Bug 159625

Summary: Move parsing of accentunder and accent attributes from renderer to element classes
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 124826    
Bug Blocks: 156536, 159623    
Attachments:
Description Flags
Patch bfulgham: review+

Frédéric Wang (:fredw)
Reported 2016-07-11 07:30:10 PDT
This is bug 156536 for accentunder/accent attributes parsed of RenderMathMLUnderOver added in bug 155756.
Attachments
Patch (30.60 KB, patch)
2016-07-19 01:41 PDT, Frédéric Wang (:fredw) 		bfulgham: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2016-07-19 01:41:08 PDT
Created attachment 283986 [details] Patch
Frédéric Wang (:fredw)
Comment 2 2016-07-21 11:52:22 PDT
@Brent: Can you please take a look at the patch?
Brent Fulgham
Comment 3 2016-07-21 12:05:09 PDT
Comment on attachment 283986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283986&action=review This patch seems fine, but I'm a little confused about the reason for the "Default" BooleanValue type. Can you clarify what that is meant to represent or what it's purpose is? > Source/WebCore/mathml/MathMLElement.cpp:530 > + if (attribute.dirty) { We prefer early return: if (!attribute.dirty) return attribute value; ... other stuff ... return attribute.value; > Source/WebCore/mathml/MathMLElement.cpp:538 > + attribute.value = BooleanValue::Default; I don't really understand the purpose of "Default" here. Is this part of the MathML specification? Maybe a better term would be "Undefined" or something?
Frédéric Wang (:fredw)
Comment 4 2016-07-21 12:14:23 PDT
(In reply to comment #3) > Comment on attachment 283986 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283986&action=review > > This patch seems fine, but I'm a little confused about the reason for the > "Default" BooleanValue type. Can you clarify what that is meant to represent > or what it's purpose is? The idea is that when you have an explicit attribute "true" or "false", you know which boolean value to set for a property. When the attribute is absent or ignored (when invalid) then a "default" or "automatic" value should be used for the property. This may depend on other parameters like the operator dictionary (as in this bug to determine whether an overscript is an accent) or some special inheritance rules (like in bug 159623 to calculate mathvariant & displaystyle). I think the exact name does not matter, it's more or less an implementation detail. Maybe "Automatic" would be more appropriate? But I'm fine with undefined too.
Brent Fulgham
Comment 5 2016-07-21 13:44:53 PDT
Comment on attachment 283986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283986&action=review r=me >> Source/WebCore/mathml/MathMLElement.cpp:538 >> + attribute.value = BooleanValue::Default; > > I don't really understand the purpose of "Default" here. Is this part of the MathML specification? > > Maybe a better term would be "Undefined" or something? Based on your explanation, I think "Default" is the right term.
Frédéric Wang (:fredw)
Comment 6 2016-07-21 22:34:08 PDT
Committed r203553: <http://trac.webkit.org/changeset/203553>
Note You need to log in before you can comment on or make changes to this bug.