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+
Patch (25.34 KB, patch)
2016-08-02 08:41 PDT, Frédéric Wang (:fredw)
no flags
Patch for EWS testing (42.91 KB, patch)
2016-08-02 12:48 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
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
Patch (25.37 KB, patch)
2016-08-02 14:18 PDT, Frédéric Wang (:fredw)
no flags
Frédéric Wang (:fredw)
Comment 1 2016-07-29 06:49:27 PDT
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
Frédéric Wang (:fredw)
Comment 8 2016-08-03 00:02:06 PDT
Note You need to log in before you can comment on or make changes to this bug.