Bug 159623 - Move parsing of display, displaystyle and mathvariant attributes from rendering to mathml element classes
Summary: Move parsing of display, displaystyle and mathvariant attributes from renderi...
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: 108778 133845 159625
Blocks: 156536 160034
  Show dependency treegraph
 
Reported: 2016-07-11 07:14 PDT by Frédéric Wang (:fredw)
Modified: 2016-07-25 00:19 PDT (History)
7 users (show)

See Also:


Attachments
Patch (applies after bug 159625) (33.98 KB, patch)
2016-07-20 07:09 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch bug 159625 + 159623 (for EWS testing) (60.47 KB, patch)
2016-07-20 07:10 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (33.64 KB, patch)
2016-07-21 22:55 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:14:09 PDT
This is bug 156536 for the attributes parsed in rendering/mathml/MathMLStyle.cpp
Comment 1 Frédéric Wang (:fredw) 2016-07-20 07:09:16 PDT
Created attachment 284101 [details]
Patch (applies after bug 159625)
Comment 2 Frédéric Wang (:fredw) 2016-07-20 07:10:31 PDT
Created attachment 284102 [details]
Patch bug 159625 + 159623 (for EWS testing)
Comment 3 Frédéric Wang (:fredw) 2016-07-21 22:55:43 PDT
Created attachment 284307 [details]
Patch
Comment 4 Brent Fulgham 2016-07-22 09:28:03 PDT
Comment on attachment 284307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284307&action=review

> Source/WebCore/mathml/MathMLElement.cpp:589
> +void MathMLElement::getSpecifiedDisplayStyle(bool& displayStyle)

We avoid using 'get' names in WebKit. This should be called "specifiedDisplayStyle".

> Source/WebCore/mathml/MathMLElement.cpp:598
> +void MathMLElement::getSpecifiedMathVariant(MathMLElement::MathVariant& mathVariant)

Ditto. We should probably call this "specifiedMathVariant"
Comment 5 Brent Fulgham 2016-07-22 09:29:24 PDT
Comment on attachment 284307 [details]
Patch

Looks great! Thanks.
Comment 6 Darin Adler 2016-07-22 11:20:01 PDT
Comment on attachment 284307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284307&action=review

>> Source/WebCore/mathml/MathMLElement.cpp:589
>> +void MathMLElement::getSpecifiedDisplayStyle(bool& displayStyle)
> 
> We avoid using 'get' names in WebKit. This should be called "specifiedDisplayStyle".

We actually reserve the use of get for functions with out arguments. And there is an out argument here. But it’s peculiar how this works; it sets the argument to true or false, or possibly leaves the argument alone in other cases. I don’t think the name "get" makes that kind of unconventional behavior clear enough.

I think it would be clearer to use a return type of Optional<bool> for clarity even if that usage is less economical at the call site. Then the function would indeed be named specifiedDisplayStyle.

>> Source/WebCore/mathml/MathMLElement.cpp:598
>> +void MathMLElement::getSpecifiedMathVariant(MathMLElement::MathVariant& mathVariant)
> 
> Ditto. We should probably call this "specifiedMathVariant"

Same comment as above.
Comment 7 Frédéric Wang (:fredw) 2016-07-25 00:19:58 PDT
Committed r203679: <http://trac.webkit.org/changeset/203679>