Bug 159625 - Move parsing of accentunder and accent attributes from renderer to element classes
Summary: Move parsing of accentunder and accent attributes from renderer to element cl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 124826
Blocks: 156536 159623
  Show dependency treegraph
 
Reported: 2016-07-11 07:30 PDT by Frédéric Wang (:fredw)
Modified: 2016-07-21 22:34 PDT (History)
2 users (show)

See Also:


Attachments
Patch (30.60 KB, patch)
2016-07-19 01:41 PDT, Frédéric Wang (:fredw)
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-07-11 07:30:10 PDT
This is bug 156536 for accentunder/accent attributes parsed of RenderMathMLUnderOver added in bug 155756.
Comment 1 Frédéric Wang (:fredw) 2016-07-19 01:41:08 PDT
Created attachment 283986 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2016-07-21 11:52:22 PDT
@Brent: Can you please take a look at the patch?
Comment 3 Brent Fulgham 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?
Comment 4 Frédéric Wang (:fredw) 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.
Comment 5 Brent Fulgham 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.
Comment 6 Frédéric Wang (:fredw) 2016-07-21 22:34:08 PDT
Committed r203553: <http://trac.webkit.org/changeset/203553>