| Summary: | Move parsing of boolean operator properties into MathMLOperatorElement | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||
| Component: | MathML | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | alex, bfulgham, buildbot, commit-queue, darin, dbarton, esprehn+autocc, glenn, kondapallykalyan, rego, rniwa | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Bug Depends on: | 160034, 160239, 160241 | ||||||||||||||||||||||
| Bug Blocks: | 156537, 160301 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Frédéric Wang (:fredw)
2016-07-26 03:00:21 PDT
Created attachment 284570 [details]
Patch
Comment on attachment 284570 [details] Patch Attachment 284570 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1755711 New failing tests: mathml/opentype/large-operators-munderover.html mathml/presentation/stretchy-depth-height-symmetric.html imported/mathml-in-html5/mathml/relations/css-styling/displaystyle-1.html mathml/presentation/mo-whitespaces.html mathml/presentation/mo-movablelimits.html mathml/opentype/opentype-stretchy-horizontal.html mathml/presentation/inferred-mrow-stretchy.html mathml/presentation/mo-stretch.html mathml/presentation/stretchy-depth-height.html mathml/presentation/displaystyle-3.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-4.html mathml/opentype/opentype-stretchy.html accessibility/math-mo-fences-and-separators.html mathml/presentation/displaystyle-1.html mathml/presentation/fenced.html mathml/presentation/fenced-mi.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-2.html imported/mathml-in-html5/mathml/presentation-markup/operators/mo-axis-height-1.html mathml/presentation/attributes-accent-accentunder-dynamic.html mathml/presentation/mo-stretchy-vertical-bar.html mathml/presentation/mo-movablelimits-dynamic.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-1.html mathml/presentation/displaystyle-2.html Created attachment 284573 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 284570 [details] Patch Attachment 284570 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1755695 New failing tests: mathml/opentype/large-operators-munderover.html mathml/presentation/stretchy-depth-height-symmetric.html imported/mathml-in-html5/mathml/relations/css-styling/displaystyle-1.html mathml/presentation/mo-whitespaces.html mathml/presentation/mo-movablelimits.html mathml/opentype/opentype-stretchy-horizontal.html mathml/presentation/inferred-mrow-stretchy.html mathml/presentation/mo-stretch.html mathml/presentation/stretchy-depth-height.html mathml/presentation/displaystyle-3.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-4.html mathml/opentype/opentype-stretchy.html accessibility/math-mo-fences-and-separators.html mathml/presentation/displaystyle-1.html mathml/presentation/fenced.html mathml/presentation/fenced-mi.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-2.html imported/mathml-in-html5/mathml/presentation-markup/operators/mo-axis-height-1.html mathml/presentation/attributes-accent-accentunder-dynamic.html mathml/presentation/mo-stretchy-vertical-bar.html mathml/presentation/mo-movablelimits-dynamic.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-1.html mathml/presentation/displaystyle-2.html Created attachment 284574 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 284570 [details] Patch Attachment 284570 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1755700 New failing tests: mathml/opentype/large-operators-munderover.html mathml/presentation/stretchy-depth-height-symmetric.html imported/mathml-in-html5/mathml/relations/css-styling/displaystyle-1.html mathml/presentation/mo-whitespaces.html mathml/presentation/mo-movablelimits.html mathml/opentype/opentype-stretchy-horizontal.html mathml/presentation/inferred-mrow-stretchy.html mathml/presentation/mo-stretch.html mathml/presentation/stretchy-depth-height.html mathml/presentation/displaystyle-3.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-4.html mathml/opentype/opentype-stretchy.html mathml/presentation/displaystyle-1.html mathml/presentation/fenced.html mathml/presentation/fenced-mi.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-2.html imported/mathml-in-html5/mathml/presentation-markup/operators/mo-axis-height-1.html mathml/presentation/attributes-accent-accentunder-dynamic.html mathml/presentation/mo-stretchy-vertical-bar.html mathml/presentation/mo-movablelimits-dynamic.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-1.html mathml/presentation/displaystyle-2.html Created attachment 284575 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 284570 [details] Patch Attachment 284570 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1755720 New failing tests: mathml/opentype/large-operators-munderover.html mathml/presentation/stretchy-depth-height-symmetric.html imported/mathml-in-html5/mathml/relations/css-styling/displaystyle-1.html mathml/presentation/mo-whitespaces.html mathml/presentation/mo-movablelimits.html mathml/opentype/opentype-stretchy-horizontal.html mathml/presentation/inferred-mrow-stretchy.html mathml/presentation/mo-stretch.html mathml/presentation/stretchy-depth-height.html mathml/presentation/displaystyle-3.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-4.html mathml/opentype/opentype-stretchy.html accessibility/math-mo-fences-and-separators.html mathml/presentation/displaystyle-1.html mathml/presentation/fenced.html mathml/presentation/fenced-mi.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-2.html imported/mathml-in-html5/mathml/presentation-markup/operators/mo-axis-height-1.html mathml/presentation/attributes-accent-accentunder-dynamic.html mathml/presentation/mo-stretchy-vertical-bar.html mathml/presentation/mo-movablelimits-dynamic.html imported/mathml-in-html5/mathml/presentation-markup/scripts/underover-parameters-1.html mathml/presentation/displaystyle-2.html Created attachment 284577 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
This patch won't work because the default operator values must also be calculated in MathMLOperatorElement first (or otherwise have a third state "default" for each flag). Created attachment 284765 [details]
WIP Patch
Created attachment 284780 [details] Patch (applies after bugs 160241, 160245 and 160239) Created attachment 285102 [details]
Patch
Comment on attachment 285102 [details] Patch Attachment 285102 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1798467 New failing tests: media/track/media-element-enqueue-event-crash.html Created attachment 285105 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 285102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285102&action=review > Source/WebCore/mathml/MathMLOperatorElement.cpp:159 > +void MathMLOperatorElement::parseProperty(MathMLOperatorDictionary::Flag flag) > +{ > + ASSERT(m_properties.dirtyFlags & flag); > + > + switch (flag) { > + case Accent: > + setOperatorFlagFromAttribute(flag, accentAttr); > + break; > + case Fence: > + setOperatorFlagFromAttribute(flag, fenceAttr); > + break; > + case LargeOp: > + setOperatorFlagFromAttribute(flag, largeopAttr); > + break; > + case MovableLimits: > + setOperatorFlagFromAttribute(flag, movablelimitsAttr); > + break; > + case Separator: > + setOperatorFlagFromAttribute(flag, separatorAttr); > + break; > + case Stretchy: > + setOperatorFlagFromAttribute(flag, stretchyAttr); > + break; > + case Symmetric: > + setOperatorFlagFromAttribute(flag, symmetricAttr); > + break; > + } > +} Two more thoughts about this: 1) Could separate out a function (inline if necessary) that maps a flag to an attribute name, rather than mixing that in with the algorithm. 2) Would be nice to have an assertion if the flag is not any of the valid values. The typical design for that is to use return and ASSERT_NOT_REACHED. It’s probably clean to combine that with suggestion (1) above since that function would naturally be done with a switch and a return inside each case, so ASSERT_NOT_REACHED after the switch would be a natural thing to do. > Source/WebCore/mathml/MathMLOperatorElement.cpp:212 > + } else if (name == accentAttr) > + m_properties.dirtyFlags |= Accent; > + else if (name == fenceAttr) > + m_properties.dirtyFlags |= Fence; > + else if (name == largeopAttr) > + m_properties.dirtyFlags |= LargeOp; > + else if (name == movablelimitsAttr) > + m_properties.dirtyFlags |= MovableLimits; > + else if (name == separatorAttr) > + m_properties.dirtyFlags |= Separator; > + else if (name == stretchyAttr) > + m_properties.dirtyFlags |= Stretchy; > + else if (name == symmetricAttr) > + m_properties.dirtyFlags |= Symmetric; Could imagine here using a function that maps an attribute name to a flag and uses a boolean or Optional to indicate that the value is not one of the flag attributes. Could be cleaner than repeating the list of the attribute and flags here. Committed r204032: <http://trac.webkit.org/changeset/204032> |