WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
160339
Determine operator direction in MathMLOperatorElement and reduce relayout
https://bugs.webkit.org/show_bug.cgi?id=160339
Summary
Determine operator direction in MathMLOperatorElement and reduce relayout
Frédéric Wang (:fredw)
Reported
2016-07-29 06:16:53 PDT
After
bug 160336
, m_isVertical is the only remaining property set in RenderMathMLOperator::setOperatorProperties. We calculate it in MathMLOperatorElement and so RenderMathMLOperator::setOperatorProperties can be removed. It seems that we can then remove many needs-relayout calls without breaking tests. I'm not sure for mfenced and I think we don't really have good test coverage for this element. So for now to be safe let's kept the calls RenderMathMLFencedOperator::setOperatorProperties in various functions by overriding the RenderMathMLOperator implementations.
Attachments
Patch
(19.23 KB, patch)
2016-07-29 06:49 PDT
,
Frédéric Wang (:fredw)
darin
: review+
Details
Formatted Diff
Diff
Patch
(25.34 KB, patch)
2016-08-02 08:41 PDT
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch for EWS testing
(42.91 KB, patch)
2016-08-02 12:48 PDT
,
Frédéric Wang (:fredw)
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews112 for mac-yosemite
(2.10 MB, application/zip)
2016-08-02 13:48 PDT
,
Build Bot
no flags
Details
Patch
(25.37 KB, patch)
2016-08-02 14:18 PDT
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1
2016-07-29 06:49:27 PDT
Created
attachment 284863
[details]
Patch
Darin Adler
Comment 2
2016-07-31 21:20:42 PDT
Comment on
attachment 284863
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=284863&action=review
Change is OK, but I think we should consider some slightly different idioms here.
> Source/WebCore/mathml/MathMLOperatorElement.cpp:67 > m_operatorText = Optional<UChar>(parseOperatorText(textContent()));
I don’t think the typecast is needed here. Should be able to take out Optional<UChar>().
> Source/WebCore/mathml/MathMLOperatorElement.cpp:77 > + if (!m_operatorText) > + operatorText();
This is a messy idiom. Would be better to make the function that updates m_operatorText and m_operatorTextIsVertical separate and not just exist as a side effect of calling operatorText(). If you take my suggestion of making the OperatorText struct below then there’s another way to do it with a private function that returns a const OperatorText& and public inline functions that extract the individual data members from the structure.
> Source/WebCore/mathml/MathMLOperatorElement.h:57 > Optional<UChar> m_operatorText; > + bool m_operatorTextIsVertical;
Since these are computed together, the correct idiom would be to have them in a struct and have the struct be the Optional data member. It’s not good to have the boolean have its validity based on the presence of the other member. struct OperatorText { UChar character; bool isVertical; }; Optional<OperatorText> m_operatorText;
> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:76 > + setOperatorProperties();
Given how this is used, the function has a peculiar name. Normally we call an operator like this something like "update" not "set". Generally speaking I am concerned about this pattern where you have to remember to call this function before calling the base class. Seems error prone as we refactor the base class and possibly add additional functions. We should consider a different idiom that isn’t as fragile.
> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.h:60 > UChar m_textContent { 0 }; > + bool m_isVertical { true };
These could use the same struct as the other class above.
Frédéric Wang (:fredw)
Comment 3
2016-08-02 08:41:16 PDT
Created
attachment 285111
[details]
Patch (In reply to
comment #2
)
> > Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:76 > > + setOperatorProperties(); > > Given how this is used, the function has a peculiar name. Normally we call > an operator like this something like "update" not "set". Generally speaking > I am concerned about this pattern where you have to remember to call this > function before calling the base class. Seems error prone as we refactor the > base class and possibly add additional functions. We should consider a > different idiom that isn’t as fragile. >
OK, after rebasing all the patches before that one it turns out that the update can be simplified a lot for mfenced operators too. Essentially, the properties only depend on what was specified in the constructor and of the text content so we can update all of them in RenderMathMLFencedOperator::updateOperatorContent. It seems that the call to setOperatorProperties() were mostly necessary to resolve the LayoutUnit again (e.g. if the style changes) but this can now just be done in minSize, maxSize, leadingSpace and trailingSpace. Actually, as noted by Javier Fernández in a previous patch, minSize and maxSize are just constant for the mfenced operators since they are not specified in the operator dictionary.
Frédéric Wang (:fredw)
Comment 4
2016-08-02 12:48:29 PDT
Created
attachment 285127
[details]
Patch for EWS testing
Build Bot
Comment 5
2016-08-02 13:48:04 PDT
Comment on
attachment 285127
[details]
Patch for EWS testing
Attachment 285127
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/1800193
Number of test failures exceeded the failure limit.
Build Bot
Comment 6
2016-08-02 13:48:07 PDT
Created
attachment 285135
[details]
Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Frédéric Wang (:fredw)
Comment 7
2016-08-02 14:18:41 PDT
Created
attachment 285138
[details]
Patch
Frédéric Wang (:fredw)
Comment 8
2016-08-03 00:02:06 PDT
Committed
r204075
: <
http://trac.webkit.org/changeset/204075
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug