Summary: | Move parsing of accentunder and accent attributes from renderer to element classes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||
Component: | MathML | Assignee: | 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
Frédéric Wang (:fredw)
2016-07-11 07:30:10 PDT
Created attachment 283986 [details]
Patch
@Brent: Can you please take a look at the patch? 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? (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. 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. Committed r203553: <http://trac.webkit.org/changeset/203553> |