Bug 160190 - Move parsing of boolean operator properties into MathMLOperatorElement
Summary: Move parsing of boolean operator properties into MathMLOperatorElement
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: 160034 160239 160241
Blocks: 156537 160301
  Show dependency treegraph
 
Reported: 2016-07-26 03:00 PDT by Frédéric Wang (:fredw)
Modified: 2016-08-02 11:48 PDT (History)
11 users (show)

See Also:


Attachments
Patch (9.86 KB, patch)
2016-07-26 03:18 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.11 MB, application/zip)
2016-07-26 04:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (1.17 MB, application/zip)
2016-07-26 04:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.36 MB, application/zip)
2016-07-26 04:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.82 MB, application/zip)
2016-07-26 04:23 PDT, Build Bot
no flags Details
WIP Patch (7.40 KB, patch)
2016-07-28 02:59 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies after bugs 160241, 160245 and 160239) (10.36 KB, patch)
2016-07-28 06:44 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (15.68 KB, patch)
2016-08-02 05:48 PDT, Frédéric Wang (:fredw)
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (737.84 KB, application/zip)
2016-08-02 06:46 PDT, Build Bot
no flags Details

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-26 03:00:21 PDT
This is again one small step for bug 156537, to handle the following attributes:

- fence
- separator
- stretchy
- symmetric
- largeop
- movablelimits
- accent

We can and probably should also handle the determination of the default dictionary values in MathMLOperatorElement but anonymous operators created by the mfenced element make this a bit tedious and we also have to take the operator form into account so I'll consider that in follow-up patches.
Comment 1 Frédéric Wang (:fredw) 2016-07-26 03:18:00 PDT
Created attachment 284570 [details]
Patch
Comment 2 Build Bot 2016-07-26 04:03:55 PDT
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
Comment 3 Build Bot 2016-07-26 04:03:59 PDT
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 4 Build Bot 2016-07-26 04:04:38 PDT
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
Comment 5 Build Bot 2016-07-26 04:04:43 PDT
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 6 Build Bot 2016-07-26 04:15:41 PDT
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
Comment 7 Build Bot 2016-07-26 04:15:45 PDT
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 8 Build Bot 2016-07-26 04:23:54 PDT
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
Comment 9 Build Bot 2016-07-26 04:23:58 PDT
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
Comment 10 Frédéric Wang (:fredw) 2016-07-27 04:48:35 PDT
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).
Comment 11 Frédéric Wang (:fredw) 2016-07-28 02:59:23 PDT
Created attachment 284765 [details]
WIP Patch
Comment 12 Frédéric Wang (:fredw) 2016-07-28 06:44:38 PDT
Created attachment 284780 [details]
Patch (applies after bugs 160241, 160245 and 160239)
Comment 13 Frédéric Wang (:fredw) 2016-08-02 05:48:40 PDT
Created attachment 285102 [details]
Patch
Comment 14 Build Bot 2016-08-02 06:46:27 PDT
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
Comment 15 Build Bot 2016-08-02 06:46:31 PDT
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 16 Darin Adler 2016-08-02 09:16:32 PDT
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.
Comment 17 Frédéric Wang (:fredw) 2016-08-02 11:48:41 PDT
Committed r204032: <http://trac.webkit.org/changeset/204032>