Bug 160339

Created attachment 284863 [details] Patch

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.

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.

Created attachment 285127 [details] Patch for EWS testing

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.

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

Created attachment 285138 [details] Patch

Committed r204075: <http://trac.webkit.org/changeset/204075>