Bug 85729 – Add support for menclose element

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Frédéric Wang (:fredw)]

"The menclose element renders its content inside the enclosing notation specified by its notation attribute. menclose accepts a single argument possibly being an inferred mrow of multiple children"

MathJax relies on the menclose element to implement various LaTeX commands like \enclose, \boxed, \cancel, \bcancel, \xcancel or \cancelto. Other examples include "\begin{array}{c}\hline ... \end{array}" or "\begin{array}{c}\hdashline ... \end{array}".

Some reftests:
http://devel.mathjax.org/testing/testsuite/MathMLToDisplay/Presentation/GeneralLayout/menclose/

A testcase:
http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose-cancellation1-simple.xhtml

[Jacques Distler]

Created attachment 171152 [details]
testcase

[Jacques Distler]

As Frederic notes, several common mathematical notations rely (for their MathML implementations) on menclose.

(In reply to comment #0)
> "The menclose element renders its content inside the enclosing notation specified by its notation attribute. menclose accepts a single argument possibly being an inferred mrow of multiple children"
> 
> MathJax relies on the menclose element to implement various LaTeX commands like \enclose, \boxed, \cancel, \bcancel, \xcancel or \cancelto. Other examples include "\begin{array}{c}\hline ... \end{array}" or "\begin{array}{c}\hdashline ... \end{array}".
> 
> Some reftests:
> http://devel.mathjax.org/testing/testsuite/MathMLToDisplay/Presentation/GeneralLayout/menclose/
> 
> A testcase:
> http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose-cancellation1-simple.xhtml

Hi Frederic. For menclose the main attribute we need to support is notation which has different values such as box, circle etc. So for the implementation what I thought was we can create the element and while parsing the attribute if values are like left, right, top, bottom, acturial, madruwb, rounder box we can add CSS border properties. For radical we can create RenderMathMLSquareRoot as anonymous parent and for circle, updiagonalstrike, downdiagonalstrike, verticalstrike, verticalstrike, longdiv we can take care in paint of menclose.

Please suggest?

[Frédéric Wang (:fredw)]

(In reply to comment #3)
> Hi Frederic. For menclose the main attribute we need to support is notation which has different values such as box, circle etc. So for the implementation what I thought was we can create the element and while parsing the attribute if values are like left, right, top, bottom, acturial, madruwb, rounder box we can add CSS border properties. For radical we can create RenderMathMLSquareRoot as anonymous parent and for circle, updiagonalstrike, downdiagonalstrike, verticalstrike, verticalstrike, longdiv we can take care in paint of menclose.
> 
> Please suggest?

Hi Gurpreet. I think you're correct, there are essentially three cases:

1) left, right, top, bottom, etc that can be drawn with CSS border
2) radical and longdiv that can be drawn with one stretchy char (radical and parenthesis) + a stretchy horizontal bar
3) other notations that are painted in the renderer.

Currently msqrt does 3) but I'd like to change that (bug 119038). However, 2) is currently blocked by a work-in-progress to refactor the stretchy code, so that can be for later.

I would open a new bug to start with the easiest case 1). For that one, you'll have to define the menclose element and notation attribute in mathtags.in and mathattrs.in. Map menclose to MathMLInlineContainerElement so that it behaves like an mrow. Then implement MathMLElement::collectStyleForPresentationAttribute somewhere (like in MathElement.cpp) so that it handles the case hasLocalName(mencloseTag) and name == notationAttr. This should map to the appropriate CSS property, I guess CSSPropertyBorder* or something.

[Frédéric Wang (:fredw)]

(In reply to comment #4)

Perhaps MathMLElement::collectStyleForPresentationAttribute is not really a good idea. Anyway, I'd suggest to open a but to implement the basic notations 1), prepare the implementation and continue the discussion there. One thing to note is that menclose@notation can have several values some may be repeated several times ; some are equivalent like box = top + bottom + left + right. So it would be good to have a parsing function that converts from the string to a bit mask. See http://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmencloseFrame.cpp#l86

(In reply to comment #5)
> (In reply to comment #4)
> 
> Perhaps MathMLElement::collectStyleForPresentationAttribute is not really a good idea. Anyway, I'd suggest to open a but to implement the basic notations 1), prepare the implementation and continue the discussion there. One thing to note is that menclose@notation can have several values some may be repeated several times ; some are equivalent like box = top + bottom + left + right. So it would be good to have a parsing function that converts from the string to a bit mask. See http://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmencloseFrame.cpp#l86

Hi Frederic. Thanks for the suggestion. Once you create the bug let me know. Shall start working on that.

[Frédéric Wang (:fredw)]

(In reply to comment #6)
> Hi Frederic. Thanks for the suggestion. Once you create the bug let me know. Shall start working on that.

Feel free to open the bug yourself and to submit a WIP patch there.

Created attachment 212887 [details]
Patch

[Build Bot]

Comment on attachment 212887 [details]
Patch

Attachment 212887 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2655411

[Build Bot]

Comment on attachment 212887 [details]
Patch

Attachment 212887 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2613444

Created attachment 212896 [details]
Patch

[Build Bot]

Comment on attachment 212896 [details]
Patch

Attachment 212896 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2653449

[Build Bot]

Comment on attachment 212896 [details]
Patch

Attachment 212896 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2692288

Created attachment 212897 [details]
Patch

[Build Bot]

Comment on attachment 212897 [details]
Patch

Attachment 212897 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2660514

Created attachment 212899 [details]
Patch

[Build Bot]

Comment on attachment 212899 [details]
Patch

Attachment 212899 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2654482

New failing tests:
mathml/presentation/menclose-notation-right.html
mathml/presentation/menclose-notation-actuarial.html
mathml/presentation/menclose-notation-bottom.html
mathml/presentation/menclose-notation-box.html
mathml/presentation/menclose-notation-top.html
mathml/presentation/menclose-notation-roundedbox.html
mathml/presentation/menclose-notation-left.html
mathml/presentation/menclose-notation-madruwb.html

[Build Bot]

Created attachment 212901 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212899 [details]
Patch

Attachment 212899 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2659523

New failing tests:
mathml/presentation/menclose-notation-right.html
mathml/presentation/menclose-notation-actuarial.html
mathml/presentation/menclose-notation-bottom.html
mathml/presentation/menclose-notation-box.html
mathml/presentation/menclose-notation-top.html
mathml/presentation/menclose-notation-roundedbox.html
mathml/presentation/menclose-notation-left.html
mathml/presentation/menclose-notation-madruwb.html

[Build Bot]

Created attachment 212904 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Sam Weinig]

Comment on attachment 212899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212899&action=review

> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:87
> +    if (hasLocalName(mencloseTag))
> +        return new (arena) RenderMathMLMenclose(this);

The normal model is override createRenderer in the derived class.  You should probably do that here.

> Source/WebCore/mathml/MathMLMencloseElement.cpp:47
> +// for notation = roundedbox we set the border radius (px)
> +static const String borderRadius()
> +{
> +    DEFINE_STATIC_LOCAL(const String, radius, (ASCIILiteral("5px")));
> +    return radius;
> +}

I don't think this is necessary.  Just use ASCIILiteral("5px")) where you call borderRadius().

> Source/WebCore/mathml/MathMLMencloseElement.cpp:144
> +    String padding;
> +    int fontSize = 0;
> +    String closingBrace = ")";
> +    TextRun run(closingBrace.impl(), closingBrace.length());
> +    Node* node = parentNode();
> +    if (node && node->renderer() && node->renderer()->style()) {
> +        const Font& font = node->renderer()->style()->font();
> +        fontSize = font.width(run);
> +        padding.append(String::number(fontSize));
> +        padding.append("px");
> +    }
> +    return padding;

This should use StringBuilder.

> Source/WebCore/mathml/MathMLMencloseElement.h:35
> +class MathMLMencloseElement : public MathMLInlineContainerElement {

If no one ever inherits from this class, you should mark it FINAL.

> Source/WebCore/mathml/MathMLMencloseElement.h:40
> +    virtual bool isPresentationAttribute(const QualifiedName&) const OVERRIDE;
> +    virtual void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStylePropertySet*) OVERRIDE;
> +    virtual void finishParsingChildren() OVERRIDE;

These can probably be private.

> Source/WebCore/mathml/MathMLMencloseElement.h:42
> +    Vector<String> getNotationValue() {return mNotationValues;}
> +    bool isRadical() {return mIsRadicalValue;}

You need spaces after the  { and before the }.

> Source/WebCore/mathml/MathMLMencloseElement.h:45
> +    MathMLMencloseElement(const QualifiedName& , Document&);

Extraneous space after const QualifiedName&.

> Source/WebCore/mathml/MathMLMencloseElement.h:49
> +    Vector<String> mNotationValues;
> +    bool mIsRadicalValue;

The style in WebCore is m_name, not mName.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49
> +        MathMLMencloseElement* menclose = static_cast<MathMLMencloseElement*>(element());

You should add and use a toMathMLMencloseElement() which asserts the type.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:66
> +    if (element()) {

When will there be no Element? If it is possible, you should probably check it for null in RenderMathMLMenclose::addChild as well.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:67
> +        MathMLMencloseElement* menclose = static_cast<MathMLMencloseElement*>(element());

You should add and use a toMathMLMencloseElement() which asserts the type.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:95
> +                if (equalIgnoringCase(notationVal[i], "updiagonalstrike"))
> +                    info.context->drawLine(IntPoint(left, (top+boxHeight)), IntPoint((left+boxWidth), top));
> +                else if (equalIgnoringCase(notationVal[i], "downdiagonalstrike"))
> +                    info.context->drawLine(IntPoint(left, top), IntPoint((left+boxWidth), (top+boxHeight)));
> +                else if (equalIgnoringCase(notationVal[i], "verticalstrike"))
> +                    info.context->drawLine(IntPoint((left+halfboxWidth), top), IntPoint((left+halfboxWidth), (top+boxHeight)));
> +                else if (equalIgnoringCase(notationVal[i], "horizontalstrike"))
> +                    info.context->drawLine(IntPoint(left, (top+halfboxHeight)), IntPoint((left+boxWidth), (top+halfboxHeight)));

You need spaces around your +s and you don't really need all the extra parenthesis.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:35
> +// Render sqrt(base), using radical notation.

Is this comment right?

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42
> +    virtual void paint(PaintInfo&, const LayoutPoint&) OVERRIDE;

Does this need to be protected? Can it be private?

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46
> +    bool paintdependentAttribute(Vector<String> attr, int attrSize) const;

This should probably take a const Vector<String>& to avoid a copy.  The name is also odd and should probably use camelcase.

Created attachment 212996 [details]
Patch

(In reply to comment #21)
> (From update of attachment 212899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212899&action=review
> 
> > Source/WebCore/mathml/MathMLInlineContainerElement.cpp:87
> > +    if (hasLocalName(mencloseTag))
> > +        return new (arena) RenderMathMLMenclose(this);
> 
> The normal model is override createRenderer in the derived class.  You should probably do that here.
> 
> > Source/WebCore/mathml/MathMLMencloseElement.cpp:47
> > +// for notation = roundedbox we set the border radius (px)
> > +static const String borderRadius()
> > +{
> > +    DEFINE_STATIC_LOCAL(const String, radius, (ASCIILiteral("5px")));
> > +    return radius;
> > +}
> 
> I don't think this is necessary.  Just use ASCIILiteral("5px")) where you call borderRadius().
> 
> > Source/WebCore/mathml/MathMLMencloseElement.cpp:144
> > +    String padding;
> > +    int fontSize = 0;
> > +    String closingBrace = ")";
> > +    TextRun run(closingBrace.impl(), closingBrace.length());
> > +    Node* node = parentNode();
> > +    if (node && node->renderer() && node->renderer()->style()) {
> > +        const Font& font = node->renderer()->style()->font();
> > +        fontSize = font.width(run);
> > +        padding.append(String::number(fontSize));
> > +        padding.append("px");
> > +    }
> > +    return padding;
> 
> This should use StringBuilder.
> 
> > Source/WebCore/mathml/MathMLMencloseElement.h:35
> > +class MathMLMencloseElement : public MathMLInlineContainerElement {
> 
> If no one ever inherits from this class, you should mark it FINAL.
> 
> > Source/WebCore/mathml/MathMLMencloseElement.h:40
> > +    virtual bool isPresentationAttribute(const QualifiedName&) const OVERRIDE;
> > +    virtual void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStylePropertySet*) OVERRIDE;
> > +    virtual void finishParsingChildren() OVERRIDE;
> 
> These can probably be private.
> 
> > Source/WebCore/mathml/MathMLMencloseElement.h:42
> > +    Vector<String> getNotationValue() {return mNotationValues;}
> > +    bool isRadical() {return mIsRadicalValue;}
> 
> You need spaces after the  { and before the }.
> 
> > Source/WebCore/mathml/MathMLMencloseElement.h:45
> > +    MathMLMencloseElement(const QualifiedName& , Document&);
> 
> Extraneous space after const QualifiedName&.
> 
> > Source/WebCore/mathml/MathMLMencloseElement.h:49
> > +    Vector<String> mNotationValues;
> > +    bool mIsRadicalValue;
> 
> The style in WebCore is m_name, not mName.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49
> > +        MathMLMencloseElement* menclose = static_cast<MathMLMencloseElement*>(element());
> 
> You should add and use a toMathMLMencloseElement() which asserts the type.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:66
> > +    if (element()) {
> 
> When will there be no Element? If it is possible, you should probably check it for null in RenderMathMLMenclose::addChild as well.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:67
> > +        MathMLMencloseElement* menclose = static_cast<MathMLMencloseElement*>(element());
> 
> You should add and use a toMathMLMencloseElement() which asserts the type.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:95
> > +                if (equalIgnoringCase(notationVal[i], "updiagonalstrike"))
> > +                    info.context->drawLine(IntPoint(left, (top+boxHeight)), IntPoint((left+boxWidth), top));
> > +                else if (equalIgnoringCase(notationVal[i], "downdiagonalstrike"))
> > +                    info.context->drawLine(IntPoint(left, top), IntPoint((left+boxWidth), (top+boxHeight)));
> > +                else if (equalIgnoringCase(notationVal[i], "verticalstrike"))
> > +                    info.context->drawLine(IntPoint((left+halfboxWidth), top), IntPoint((left+halfboxWidth), (top+boxHeight)));
> > +                else if (equalIgnoringCase(notationVal[i], "horizontalstrike"))
> > +                    info.context->drawLine(IntPoint(left, (top+halfboxHeight)), IntPoint((left+boxWidth), (top+halfboxHeight)));
> 
> You need spaces around your +s and you don't really need all the extra parenthesis.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:35
> > +// Render sqrt(base), using radical notation.
> 
> Is this comment right?
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42
> > +    virtual void paint(PaintInfo&, const LayoutPoint&) OVERRIDE;
> 
> Does this need to be protected? Can it be private?
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46
> > +    bool paintdependentAttribute(Vector<String> attr, int attrSize) const;
> 
> This should probably take a const Vector<String>& to avoid a copy.  The name is also odd and should probably use camelcase.

Hi Sam. Thanks for the detailed review. Uploaded new patch with the changes.

[EFL EWS Bot]

Comment on attachment 212996 [details]
Patch

Attachment 212996 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2774067

[EFL EWS Bot]

Comment on attachment 212996 [details]
Patch

Attachment 212996 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2782064

Created attachment 212998 [details]
Patch

[Build Bot]

Comment on attachment 212998 [details]
Patch

Attachment 212998 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2640243

New failing tests:
mathml/presentation/menclose-notation-right.html
mathml/presentation/menclose-notation-actuarial.html
mathml/presentation/menclose-notation-bottom.html
mathml/presentation/menclose-notation-box.html
mathml/presentation/menclose-notation-top.html
mathml/presentation/menclose-notation-roundedbox.html
mathml/presentation/menclose-notation-left.html
mathml/presentation/menclose-notation-madruwb.html

[Build Bot]

Created attachment 213004 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212998 [details]
Patch

Attachment 212998 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2695162

New failing tests:
mathml/presentation/menclose-notation-right.html
mathml/presentation/menclose-notation-actuarial.html
mathml/presentation/menclose-notation-bottom.html
mathml/presentation/menclose-notation-box.html
mathml/presentation/menclose-notation-top.html
mathml/presentation/menclose-notation-roundedbox.html
mathml/presentation/menclose-notation-left.html
mathml/presentation/menclose-notation-madruwb.html

[Build Bot]

Created attachment 213007 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212998 [details]
Patch

Attachment 212998 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2722279

New failing tests:
mathml/presentation/menclose-notation-right.html
mathml/presentation/menclose-notation-actuarial.html
mathml/presentation/menclose-notation-bottom.html
mathml/presentation/menclose-notation-box.html
mathml/presentation/menclose-notation-top.html
mathml/presentation/menclose-notation-roundedbox.html
mathml/presentation/menclose-notation-left.html
mathml/presentation/menclose-notation-madruwb.html

[Build Bot]

Created attachment 213012 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

(In reply to comment #26)
> Created an attachment (id=212998) [details]
> Patch

Some test cases are failing on mac.
In the expected.txt file there is "ab" in the same line whereas the mac results show ab one below the other hence the failure.
Can someone please help me with this? As in why this difference?

(In reply to comment #33)
> (In reply to comment #26)
> > Created an attachment (id=212998) [details] [details]
> > Patch
> 
> Some test cases are failing on mac.
> In the expected.txt file there is "ab" in the same line whereas the mac results show ab one below the other hence the failure.
> Can someone please help me with this? As in why this difference?

Hi Sam. Can you please review? Thanks

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #26)
> > > Created an attachment (id=212998) [details] [details] [details]
> > > Patch
> > 
> > Some test cases are failing on mac.
> > In the expected.txt file there is "ab" in the same line whereas the mac results show ab one below the other hence the failure.
> > Can someone please help me with this? As in why this difference?
> 
> Hi Sam. Can you please review? Thanks

Hi. Can someone please review this. Thanks

[Frédéric Wang (:fredw)]

(In reply to comment #35)
> 
> Hi. Can someone please review this. Thanks

Hi Gurpreet. I'm really willing to look at your code but don't have much time right now (working on MathJax v2.3 testing this month). I promise I'll give feedback asap. Thanks again for working on this.

[Frédéric Wang (:fredw)]

(In reply to comment #36)

Quick remarks:

- I'm wondering how relevant it is for the accessibility tools to just know about the fact that something is "menclose" without knowing the notations. Also, <menclose notation="radical"> should probably be treated as <msqrt> in the accessibility code too.

- I think the menclose notations, and most MathML attributes, are case sensitive.

- When someone uses a list of notation like "roundedbox madruwb" they should overlap around the inner child. I'm not sure that will be possible with your code, since some CSS rules are conflicting. You should probably have tests when multiple notations are used at the same time.

- RenderMathMLMenclose::paint should not draw anything when the visibility is hidden, see bug 116600. Perhaps add tests for this and for the colors of the graphics.

- The menclose width/height must be at least \sqrt(2) times as large as those of its child if you don't want overlap. Consider for example <math><menclose notation="circle"><mspace width="100px" height="50px" mathbackground="red"/></menclose></math>. Add a javascript test for that?

- Perhaps have a test that checks the equivalence <menclose notation="radical"> and <msqrt>.

[chris fleizach]

Comment on attachment 212998 [details]
Patch

I also don't think it is necessary for AX code to know about a menclose object itself. There's probably more nuanced behavior that would be needed. I'd be fine if you left the Accessibility changes out of this patch

r- for  Frédéric's comments and failing layout tests.
I also don't know exactly why the tests would be failing, but feel free to upload patches to experiment with build bots

(In reply to comment #38)
> (From update of attachment 212998 [details])
> I also don't think it is necessary for AX code to know about a menclose object itself. There's probably more nuanced behavior that would be needed. I'd be fine if you left the Accessibility changes out of this patch
> 
> r- for  Frédéric's comments and failing layout tests.
> I also don't know exactly why the tests would be failing, but feel free to upload patches to experiment with build bots

Thanks Frederic and Chris for the review. Sorry for the delayed reply. Was busy with some other work.

Frederic I had some queries

I think the menclose notations, and most MathML attributes, are case sensitive. : You mean smaller case it should apply and bigger case it should not? Is yes I observed in FF its applies for both case.

When someone uses a list of notation like "roundedbox madruwb" they should overlap around the inner child. I'm not sure that will be possible with your code, since some CSS rules are conflicting. You should probably have tests when multiple notations are used at the same time. : I got your point. Can you suggest how can I achieve this?

The menclose width/height must be at least \sqrt(2) times as large as those of its child if you don't want overlap ;Consider for example <math><menclose notation="circle"><mspace width="100px" height="50px" mathbackground="red"/></menclose></math>: You are right. Right now its overlapping. Need suggestion for this also how it can be achived? Through compute min/max pref height and width?

[Frédéric Wang (:fredw)]

(In reply to comment #39)
> Frederic I had some queries
> 
> I think the menclose notations, and most MathML attributes, are case sensitive. : You mean smaller case it should apply and bigger case it should not? Is yes I observed in FF its applies for both case.

I mean "circle" is accepted since it is defined in the spec and in the RelaxNG schema but "CIRCLE" or "CirClE" are supposed to be different notations (and thus ignored). This would just be changing equalIgnoringCase.

> When someone uses a list of notation like "roundedbox madruwb" they should overlap around the inner child. I'm not sure that will be possible with your code, since some CSS rules are conflicting. You should probably have tests when multiple notations are used at the same time. : I got your point. Can you suggest how can I achieve this?

I suspect a general solution will be to draw all elements with the graphic libraries rather than mixing them with CSS properties. However as I told you, you might just want to start with a smaller bug that implements a partial support. Overlapping notations is probably less frequent in practice so of lower priority.

> You are right. Right now its overlapping. Need suggestion for this also how it can be achived? Through compute min/max pref height and width?

I think you only need to ensure the preferred min/max width of menclose to be sqrt(2) times as large as the preferred min/max width of its content ; and similarly for the actual width/height.

(In reply to comment #40)
> (In reply to comment #39)
> > Frederic I had some queries
> > 
> > I think the menclose notations, and most MathML attributes, are case sensitive. : You mean smaller case it should apply and bigger case it should not? Is yes I observed in FF its applies for both case.
> 
> I mean "circle" is accepted since it is defined in the spec and in the RelaxNG schema but "CIRCLE" or "CirClE" are supposed to be different notations (and thus ignored). This would just be changing equalIgnoringCase.
> 
> > When someone uses a list of notation like "roundedbox madruwb" they should overlap around the inner child. I'm not sure that will be possible with your code, since some CSS rules are conflicting. You should probably have tests when multiple notations are used at the same time. : I got your point. Can you suggest how can I achieve this?
> 
> I suspect a general solution will be to draw all elements with the graphic libraries rather than mixing them with CSS properties. However as I told you, you might just want to start with a smaller bug that implements a partial support. Overlapping notations is probably less frequent in practice so of lower priority.
> 
> > You are right. Right now its overlapping. Need suggestion for this also how it can be achived? Through compute min/max pref height and width?
> 
> I think you only need to ensure the preferred min/max width of menclose to be sqrt(2) times as large as the preferred min/max width of its content ; and similarly for the actual width/height.

Thanks Fredric for the quick response. I am confused now :(
Should I continue to work on this or not ?

[Frédéric Wang (:fredw)]

(In reply to comment #41)
> Thanks Fredric for the quick response. I am confused now :(
> Should I continue to work on this or not ?

I think most of the comments will be easy to address, except perhaps handling mixing several notations and adding better accessibility support. My point is that you don't need to worry too much about these for a first implementation. They can be handled in separate bugs.

Created attachment 220506 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 220506 [details]
Patch

Attachment 220506 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5469383690289152

[EFL EWS Bot]

Comment on attachment 220506 [details]
Patch

Attachment 220506 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4658955305353216

Created attachment 220507 [details]
Patch

[EFL EWS Bot]

Comment on attachment 220507 [details]
Patch

Attachment 220507 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5386102932242432

[EFL EWS Bot]

Comment on attachment 220507 [details]
Patch

Attachment 220507 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5808315397308416

Created attachment 220508 [details]
Patch

(In reply to comment #49)
> Created an attachment (id=220508) [details]
> Patch

Hi Frederic and Chris. Sorry about the delay.
I have updated new patch with the changes as per what you both had suggested. Few things still needs to be worked on, which I can feel we can handle as separate bugs of menclose. Please review. Thanks.

[Frédéric Wang (:fredw)]

Comment on attachment 220508 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review

Thanks, I'll try to look into this asap.

Can you update the LayoutTests/mathml/presentation/inferred-mrow-* tests to take into account menclose?

> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:89
> +        return createRenderer<RenderMathMLUnderOver>(*this, std::move(style));

Not sure why it is RenderMathMLUnderOver but I guess it the menclose renderer is created in the derived class anyway...

[Frédéric Wang (:fredw)]

Comment on attachment 220508 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review

> LayoutTests/mathml/presentation/menclose-notation-attribute-set2.html:43
> +</html>

Unfortunately, you can not group mismatch tests (one mismatch of an menclose pair is enough to make the test pass)

> LayoutTests/mathml/presentation/menclose-notation-no-overlap.html:11
> +				isSuccessfullyParsed();

Perhaps you should say that these magic numbers are \sqrt(2) times the inner width/height.

> LayoutTests/mathml/presentation/menclose-notation-radical.html:8
> +            <math><menclose notation="radical"><msqrt><mspace width="100px" height="50px" mathbackground="red"/></msqrt></menclose></math>

Do you really want a mismatch test here or rather that

<menclose notation="radical"><mspace width="100px" height="50px" mathbackground="red"/></menclose>

renders the same as

<msqrt><mspace width="100px" height="50px" mathbackground="red"/></msqrt>

?

[chris fleizach]

Comment on attachment 220508 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review

> Source/WebCore/dom/Element.h:473
> +#endif

we probably don't want to put this in Element. The other MathML elements are referenced like this in MathMLElement.h

> Source/WebCore/mathml/MathMLMencloseElement.cpp:3
> + * Copyright (C) 2013 The MathJax Consortium.

if this is a new file why does it have a 2010 copyright?

> Source/WebCore/mathml/MathMLMencloseElement.cpp:70
> +    if (m_IsRadicalValue && (!firstElementChild()))

the parens around firstElementChild() seem unnecessary
can you add a comment why you need to add a child when you finish parsing

what do the 0's stand for in addChild()? if they're pointers they should be nullptr

> Source/WebCore/mathml/MathMLMencloseElement.cpp:80
> +            int attrvaluesSize = m_NotationValues.size();

size() returns a size_t

> Source/WebCore/mathml/MathMLMencloseElement.cpp:131
> +        fontSize = font.width(run);

does font.width() return an int? my guess would be that it'd return an unsigned

> Source/WebCore/mathml/MathMLMencloseElement.h:2
> + * Copyright (C) 2010 Alex Milowski ([e-mail]). All rights reserved.

ditto about copyright

> Source/WebCore/mathml/MathMLMencloseElement.h:39
> +    Vector<String>& getNotationValue() { return m_NotationValues; }

i think you can drop the "get" prefix and name it notationValues() const

> Source/WebCore/mathml/MathMLMencloseElement.h:40
> +    bool isRadical() { return m_IsRadicalValue; }

should be a const

> Source/WebCore/mathml/MathMLMencloseElement.h:52
> +    Vector<String> m_NotationValues;

this should be m_notationValues

> Source/WebCore/mathml/MathMLMencloseElement.h:53
> +    bool m_IsRadicalValue;

should be m_isRadicalValue

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:50
> +    // For radical notation creating an anonymous RenderMathMLSquareRoot.

this comment doesn't explain why you're doing this

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:56
> +            toRenderElement(firstChild())->addChild(newChild, beforeChild && beforeChild->parent() == firstChild() ? beforeChild : 0);

0 should be nullptr

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:90
> +    Vector<String>& notationVal = menclose->getNotationValue();

var name should be notationValues

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:91
> +    int notationvalSize = notationVal.size();

size_t instead of int

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:93
> +        isDefaultLongDiv = true;

seems like this should be written as

bool isDefaultLongDiv = !notatationalValueSize;

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:137
> +bool RenderMathMLMenclose::isValidNotationValue(Vector<String>& attr, int attrSize) const

passing in attrSize seems unnecessary since you're already passing in attr, which can easily find the size of itself

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:139
> +    for (int i = 0;i < attrSize; i++) {

bad spacing

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:140
> +        if (attr[i] == "updiagonalstrike" || attr[i] == "downdiagonalstrike" || attr[i] == "horizontalstrike" || attr[i] == "verticalstrike"

since you only seem to do work in the above method when the attr name matches one of these, it seems like this check is superfluous. Anything not matching one of these would cause anything to happen

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:43
> +    RenderMathMLSquareRoot* newSRoot = new RenderMathMLSquareRoot(toElement(*node), RenderStyle::createAnonymousStyleWithDisplay(&parent->style(), FLEX));

variable name should be something like squareRoot, or root

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39
> +    static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const RenderObject*);

I have a feeling this should return a RenderPtr

> LayoutTests/mathml/presentation/menclose-notation-attribute-set1.html:42
> +				isSuccessfullyParsed();

bad indentation

Created attachment 220597 [details]
Patch

Created attachment 220602 [details]
Patch

(In reply to comment #55)
> Created an attachment (id=220602) [details]
> Patch

Hi Frederic and Chris. Thanks for the review.Have made the changes as per the comments and uploaded new  patch. But the build bots is not applying the patch :(

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39
> +    static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const RenderObject*);

I have a feeling this should return a RenderPtr : Other files I checked there also its not returning RenderPtr e.g RenderMathMLRow.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:140
> +        if (attr[i] == "updiagonalstrike" || attr[i] == "downdiagonalstrike" || attr[i] == "horizontalstrike" || attr[i] == "verticalstrike"

since you only seem to do work in the above method when the attr name matches one of these, it seems like this check is superfluous. Anything not matching one of these would cause anything to happen : Not very clear and cause anything to happen like?

[Frédéric Wang (:fredw)]

(In reply to comment #56)
> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39
> > +    static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const RenderObject*);
> 
> I have a feeling this should return a RenderPtr : Other files I checked there also its not returning RenderPtr e.g RenderMathMLRow.
> 

See

https://bugs.webkit.org/show_bug.cgi?id=126631
https://bugs.webkit.org/show_bug.cgi?id=126583

for ongoing changes.

(In reply to comment #57)
> (In reply to comment #56)
> > > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39
> > > +    static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const RenderObject*);
> > 
> > I have a feeling this should return a RenderPtr : Other files I checked there also its not returning RenderPtr e.g RenderMathMLRow.
> > 
> 
> See
> 
> https://bugs.webkit.org/show_bug.cgi?id=126631
> https://bugs.webkit.org/show_bug.cgi?id=126583
> 
> for ongoing changes.

Ahh Ok :)

Created attachment 220622 [details]
Patch

Created attachment 220635 [details]
Patch

[EFL EWS Bot]

Comment on attachment 220635 [details]
Patch

Attachment 220635 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6371233004257280

[EFL EWS Bot]

Comment on attachment 220635 [details]
Patch

Attachment 220635 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6375007340986368

[Build Bot]

Comment on attachment 220635 [details]
Patch

Attachment 220635 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4963858120704000

[Build Bot]

Comment on attachment 220635 [details]
Patch

Attachment 220635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5921713137123328

[chris fleizach]

(In reply to comment #64)
> (From update of attachment 220635 [details])
> Attachment 220635 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/5921713137123328

/Volumes/Data/EWS/WebKit/Source/WebCore/mathml/MathMLMencloseElement.cpp:83:31: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
            for (int i = 0; i < attrvaluesSize; i++) {
                            ~ ^ ~~~~~~~~~~~~~~
1 error generated.

[chris fleizach]

Comment on attachment 220635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220635&action=review

> Source/WebCore/mathml/MathMLMencloseElement.cpp:44
> +    , m_notationValues(0)

i don't think m_notationValues needs to be initialized.

> Source/WebCore/mathml/MathMLMencloseElement.h:27
> +#define MATHMLMENCLOSELEMENT_H

this should not be upper cased

> Source/WebCore/mathml/MathMLMencloseElement.h:37
> +    Vector<String>& notationValue() { return m_notationValues; }

this method should be const and be called
notationValues

> Source/WebCore/mathml/MathMLMencloseElement.h:46
> +    String longDivLeftPadding();

i think this method can be const

> Source/WebCore/mathml/MathMLMencloseElement.h:48
> +    virtual bool isMathMLMencloseElement() const { return true; }

this should be OVERRIDE FINAL

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:92
> +    if ((notationalValueSize && isValidNotationValue(notationVal)) || isDefaultLongDiv) {

do you need to call isValidNotationValue() here?
if you don't call isValidNotationValue() and you have an invalid attribute, does anything bad happen?
it seems like the invalid attribute just won't cause any drawing to happen, but at least the right of the attributes will work

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:105
> +        for (int i = 0; i < notationalValueSize; i++) {

notationalValueSize is a size_t so i should be as well

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:135
> +bool RenderMathMLMenclose::isValidNotationValue(Vector<String>& attr) const

this method name should indicate that it checks all the values in the array, so something like
checkNotationalValuesValidity()

right now the name seems like it operates on a single value

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:27
> +#define RENDERMATHMLMENCLOSE_H

this should not be upper cased

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46
> +    bool isValidNotationValue(Vector<String>& attr) const;

argument name is not necessary here

[Frédéric Wang (:fredw)]

(In reply to comment #66)
> (From update of attachment 220635 [details])
> > Source/WebCore/mathml/MathMLMencloseElement.h:27
> > +#define MATHMLMENCLOSELEMENT_H
> 
> this should not be upper cased
> 

I got the same style error in bug 124838 comment 26. That seems to be a bug in the check-webkit-style script...

(In reply to comment #52)
> (From update of attachment 220508 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review
> Do you really want a mismatch test here or rather that
> 
> <menclose notation="radical"><mspace width="100px" height="50px" mathbackground="red"/></menclose>
> 
> renders the same as
> 
> <msqrt><mspace width="100px" height="50px" mathbackground="red"/></msqrt>
> 
> ? : I have reverted back to mismatch since the height of content incase of menclose is more as compared to <msqrt> test case.

Created attachment 220703 [details]
Patch

(In reply to comment #66)
> (From update of attachment 220635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220635&action=review
> 
> > Source/WebCore/mathml/MathMLMencloseElement.h:27
> > +#define MATHMLMENCLOSELEMENT_H
> 
> this should not be upper cased
> 

Got style error yesterday but today I updated my webkit code and it doesnot show the style error. So new patch its reverted.

> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:92
> > +    if ((notationalValueSize && isValidNotationValue(notationVal)) || isDefaultLongDiv) {
> 
> do you need to call isValidNotationValue() here?
> if you don't call isValidNotationValue() and you have an invalid attribute, does anything bad happen?
> it seems like the invalid attribute just won't cause any drawing to happen, but at least the right of the attributes will work

Incase attribute value is invalid then paint wont happen but unnecessary calculations of boxWidth, boxHeight etc would happen. Not required right?

[EFL EWS Bot]

Comment on attachment 220703 [details]
Patch

Attachment 220703 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6072233084583936

[kov's GTK+ EWS bot]

Comment on attachment 220703 [details]
Patch

Attachment 220703 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5445130681057280

[Frédéric Wang (:fredw)]

(In reply to comment #68)
> I have reverted back to mismatch since the height of content incase of menclose is more as compared to <msqrt> test case.

Ideally <menclose notation="radical"> and <msqrt>. If that's too hard for this bug, perhaps you can keep the match reftest, mark it as known failure and open a follow-up bug. I don't think that's a too serious MathJax has the same bug (https://github.com/mathjax/MathJax/issues/101) and everybody uses <msqrt> anyway.

Note however that in the two cases, you didn't close the <menclose notation="radical"> tag so that might explain your rendering issue.

(In reply to comment #73)
> (In reply to comment #68)
> > I have reverted back to mismatch since the height of content incase of menclose is more as compared to <msqrt> test case.
> 
> Ideally <menclose notation="radical"> and <msqrt>. If that's too hard for this bug, perhaps you can keep the match reftest, mark it as known failure and open a follow-up bug. I don't think that's a too serious MathJax has the same bug (https://github.com/mathjax/MathJax/issues/101) and everybody uses <msqrt> anyway.
> 
> Note however that in the two cases, you didn't close the <menclose notation="radical"> tag so that might explain your rendering issue.

Actually the difference is because for menclose case we set the height and width to sqrt(2) times its child content which is more as compared to msqrt case. So dont think that will be similar in any case.

[Frédéric Wang (:fredw)]

(In reply to comment #74)
> Actually the difference is because for menclose case we set the height and width to sqrt(2) times its child content which is more as compared to msqrt case. So dont think that will be similar in any case.

This should be done *only* when the circle notation is used. Otherwise this adds too much unnecessary space e.g. if you do only box or strike notations. For <menclose notation="radical"> that should not be necessary, so it could render the same as <msqrt> if menclose adds not padding/margin.

[EFL EWS Bot]

Comment on attachment 220703 [details]
Patch

Attachment 220703 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5324357945524224

[Build Bot]

Comment on attachment 220703 [details]
Patch

Attachment 220703 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5621356410437632

[Build Bot]

Comment on attachment 220703 [details]
Patch

Attachment 220703 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5058178957967360

(In reply to comment #75)
> (In reply to comment #74)
> > Actually the difference is because for menclose case we set the height and width to sqrt(2) times its child content which is more as compared to msqrt case. So dont think that will be similar in any case.
> 
> This should be done *only* when the circle notation is used. Otherwise this adds too much unnecessary space e.g. if you do only box or strike notations. For <menclose notation="radical"> that should not be necessary, so it could render the same as <msqrt> if menclose adds not padding/margin.

Ahh Ok. Right now I have done for all. Also the spec suggests
"The values "box", "roundedbox", and "circle" should enclose the contents as indicated by the values. The amount of distance between the box, roundedbox, or circle, and the contents are not specified by MathML, and is left to the renderer. In practice, paddings on each side of 0.4em in the horizontal direction and .5ex in the vertical direction seem to work well."
so in mathml.css I have added padding which is for all.

[Frédéric Wang (:fredw)]

> (In reply to comment #75)
> Ahh Ok. Right now I have done for all. Also the spec suggests
> "The values "box", "roundedbox", and "circle" should enclose the contents as indicated by the values. The amount of distance between the box, roundedbox, or circle, and the contents are not specified by MathML, and is left to the renderer. In practice, paddings on each side of 0.4em in the horizontal direction and .5ex in the vertical direction seem to work well."
> so in mathml.css I have added padding which is for all.

Well, I think this is a suggestion from Neil Soiffer as implemented in MathPlayer but not a normative a requirement (and does not work with circle BTW). Gecko adds this kind of padding (with different values) only for borders.

[Frédéric Wang (:fredw)]

Created attachment 220708 [details]
diagram and equation explaining where \sqrt{2} comes from

FYI.

(In reply to comment #81)
> Created an attachment (id=220708) [details]
> diagram and equation explaining where \sqrt{2} comes from
> 
> FYI.

Thanks Frederic for this.

For menclose padding I am keeping the value as mentioned in the spec else the box, roundedbox are very close to the child content. Please suggest?

[Frédéric Wang (:fredw)]

(In reply to comment #82)
> (In reply to comment #81)
> For menclose padding I am keeping the value as mentioned in the spec else the box, roundedbox are very close to the child content. Please suggest?

Yes, you need padding for these and other border notations. Ideally, you could do as in Gecko and add padding only for notations that need it but I don't think it's easy to do that if you rely only on mathml.css.

Created attachment 220718 [details]
Patch

[EFL EWS Bot]

Comment on attachment 220718 [details]
Patch

Attachment 220718 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6644967241940992

[Build Bot]

Comment on attachment 220718 [details]
Patch

Attachment 220718 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4523012342677504

New failing tests:
mathml/presentation/menclose-notation-radical.html
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 220724 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 220718 [details]
Patch

Attachment 220718 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4986078134009856

New failing tests:
mathml/presentation/menclose-notation-radical.html
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 220729 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 220718 [details]
Patch

Attachment 220718 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4600526436040704

New failing tests:
mathml/presentation/menclose-notation-radical.html
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 220736 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 220830 [details]
Patch

[Build Bot]

Comment on attachment 220830 [details]
Patch

Attachment 220830 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4683762499584000

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 220833 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 220830 [details]
Patch

Attachment 220830 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5255957638545408

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 220834 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 220830 [details]
Patch

Attachment 220830 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4823773601595392

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
svg/custom/conditional-processing-2.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 220836 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

I am working on the failed test cases.One of the test cases the mo operator is not stretching. And other the mspace is not aligned properly.

[Frédéric Wang (:fredw)]

(In reply to comment #99)
> I am working on the failed test cases.One of the test cases the mo operator is not stretching. And other the mspace is not aligned properly.

See https://bugs.webkit.org/show_bug.cgi?id=124841. menclose should behave like an mrow.

Created attachment 221031 [details]
Patch

[Build Bot]

Comment on attachment 221031 [details]
Patch

Attachment 221031 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5419429596758016

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 221035 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221031 [details]
Patch

Attachment 221031 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4825429579923456

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 221037 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 221043 [details]
Patch

[Build Bot]

Comment on attachment 221043 [details]
Patch

Attachment 221043 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5875198775525376

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 221046 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221043 [details]
Patch

Attachment 221043 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6283281737711616

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 221048 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

Created attachment 221107 [details]
Patch

[Build Bot]

Comment on attachment 221107 [details]
Patch

Attachment 221107 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6118968636997632

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 221108 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221107 [details]
Patch

Attachment 221107 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6650917516476416

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 221113 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 221115 [details]
Patch

[Build Bot]

Comment on attachment 221115 [details]
Patch

Attachment 221115 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5427322270253056

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 221121 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221115 [details]
Patch

Attachment 221115 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4517060725964800

New failing tests:
mathml/presentation/inferred-mrow-baseline.html
mathml/presentation/inferred-mrow-stretchy.html

[Build Bot]

Created attachment 221125 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 221127 [details]
Patch

(In reply to comment #121)
> Created an attachment (id=221127) [details]
> Patch

Finally the build is done. :) Please review.

[Frédéric Wang (:fredw)]

Comment on attachment 221127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221127&action=review

Could you please add tests for dynamic changes: adding/removing children, adding/removing/changing attribute values?

> Source/WebCore/mathml/MathMLElement.h:47
> +        return hasTagName(MathMLNames::miTag) || hasTagName(MathMLNames::mnTag) || hasTagName(MathMLNames::moTag) || hasTagName(MathMLNames::msTag) || hasTagName(MathMLNames::mtextTag) || hasTagName(MathMLNames::mencloseTag);

Why do you need that? menclose is not a MathML token.

> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:89
> +        return createRenderer<RenderMathMLMenclose>(*this, std::move(style));

Is this necessary. I believe the menclose renderer is created in MathMLMencloseElement::createElementRenderer...

> Source/WebCore/mathml/MathMLMencloseElement.cpp:2
> + * Copyright (C) 2014 The MathJax Consortium. All rights reserved.

The copyright should contain your name, I think.

> Source/WebCore/mathml/MathMLMencloseElement.cpp:68
> +    // when notation value is radical and menclose doesnot have any child then

s/doesnot/does not/

> Source/WebCore/mathml/MathMLMencloseElement.h:2
> + * Copyright (C) 2014 The MathJax Consortium. All rights reserved.

Same here about Copyright.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:2
> + * Copyright (C) 2014 The MathJax Consortium. All rights reserved.

ditto.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:2
> + * Copyright (C) 2014 The MathJax Consortium. All rights reserved.

ditto.

Created attachment 221345 [details]
Patch

> > Source/WebCore/mathml/MathMLElement.h:47
> > +        return hasTagName(MathMLNames::miTag) || hasTagName(MathMLNames::mnTag) || hasTagName(MathMLNames::moTag) || hasTagName(MathMLNames::msTag) || hasTagName(MathMLNames::mtextTag) || hasTagName(MathMLNames::mencloseTag);
> 
> Why do you need that? menclose is not a MathML token.
> 

In MathMLMencloseElement.h its being used in toMathMLMencloseElement. I check if it is menclose and then static_cast.

(In reply to comment #124)
> Created an attachment (id=221345) [details]
> Patch

Thanks Frederic for the review.

Chris and Frederic : There are some known issues of menclose like if no notation attribute then longdiv should be displayed. Its not handled yet. So if basic implementation is fine then can we go ahead with this and for the known issues handle those in different bugs? Just wanted to know your thoughts?

[Frédéric Wang (:fredw)]

(In reply to comment #125)
> In MathMLMencloseElement.h its being used in toMathMLMencloseElement. I check if it is menclose and then static_cast.

I'm not sure what is the best way to cast menclose but:

- you definitely don't want to cast a token element like mtext, mn etc to and menclose element. 

- you don't want to make isTokenElement return true for an menclose element.

http://www.w3.org/TR/MathML/chapter3.html#id.3.1.9.1

(In reply to comment #127)
> (In reply to comment #125)
> > In MathMLMencloseElement.h its being used in toMathMLMencloseElement. I check if it is menclose and then static_cast.
> 
> I'm not sure what is the best way to cast menclose but:
> 
> - you definitely don't want to cast a token element like mtext, mn etc to and menclose element. 
> 
> - you don't want to make isTokenElement return true for an menclose element.
> 
> http://www.w3.org/TR/MathML/chapter3.html#id.3.1.9.1

Actually earlier I had written isMathMLMencloseElement function which was overring the function is Element.h (previous patches) but then removed as per Chris's suggestion. (we probably don't want to put this in Element. The other MathML elements are referenced like this in MathMLElement.h)

(In reply to comment #128)
> (In reply to comment #127)
> > (In reply to comment #125)
> > > In MathMLMencloseElement.h its being used in toMathMLMencloseElement. I check if it is menclose and then static_cast.
> > 
> > I'm not sure what is the best way to cast menclose but:
> > 
> > - you definitely don't want to cast a token element like mtext, mn etc to and menclose element. 
> > 
> > - you don't want to make isTokenElement return true for an menclose element.
> > 
> > http://www.w3.org/TR/MathML/chapter3.html#id.3.1.9.1
> 
> Actually earlier I had written isMathMLMencloseElement function which was overring the function is Element.h (previous patches) but then removed as per Chris's suggestion. (we probably don't want to put this in Element. The other MathML elements are referenced like this in MathMLElement.h)

Else can add another API to check for menclose in MathMLElement.h. Your thoughts?

(In reply to comment #124)
> Created an attachment (id=221345) [details]
> Patch

Hi Chris. Can you please have a look? Thanks

Created attachment 221618 [details]
Patch

Created attachment 221716 [details]
Patch

(In reply to comment #132)
> Created an attachment (id=221716) [details]
> Patch

Patch is not being applied. All changes have been done as per the suggestions. Chris and Frederic can you please review. Thanks

[Frédéric Wang (:fredw)]

Comment on attachment 221716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221716&action=review

> LayoutTests/fast/css/resize-single-axis.html:73
> +    //}

Why these changes?

> LayoutTests/mathml/presentation/menclose-notation-attribute-remove-expected.html:5
> +            <math><mspace width="100px" height="50px" mathbackground="red"/></math>

You should have an menclose element here, otherwise the test will fail when you make "longdiv" be the default.

> LayoutTests/mathml/presentation/menclose-notation-attribute-set1.html:42
> +                isSuccessfullyParsed();

I think you could do more testing e.g. check the borders that are absent or the style of corner of roundedbox

> LayoutTests/mathml/presentation/menclose-notation-no-overlap.html:7
> +                description('Tests that menclose with notation valus as circle is sqrt(2) times the inner width/height.');

s/valus/value/

Created attachment 221840 [details]
Patch

[chris fleizach]

Comment on attachment 221840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221840&action=review

Thanks for your hard work on this. It's getting close. Maybe you need to update your local sources, which is why the patch doesn't apply

> Source/WebCore/ChangeLog:66
> +        its values liek top, left CSSBorder properties are applied and for

liek misspelled

> Source/WebCore/ChangeLog:68
> +        For radical value an anonymous RenderMathMLSquareRoot is created as

as "a" child of menclose

> Source/WebCore/mathml/MathMLMencloseElement.cpp:68
> +    // when notation value is radical and menclose does not have any child then

when should be capitalized

the sentence should say "When notation value is "a" radical and ..."

> Source/WebCore/mathml/MathMLMencloseElement.cpp:79
> +        if (!val.isEmpty()) {

i think this line should be an early return
if (val.isEmpty())
   return

> Source/WebCore/mathml/MathMLMencloseElement.cpp:81
> +            size_t attrvaluesSize = m_notationValues.size();

this variable should be named
notationValueSize

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49
> +    // For radical notation creating an anonymous RenderMathMLSquareRoot.

This comment should probably be worded like

"Allow an anonymous RenderMathMLSquareRoot to handle drawing the radical notation, rather than duplicating the code needed to paint a root."

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:70
> +    const Vector<String>& notationVal = menclose->notationValues();

I would call this variable notationValues

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:74
> +            m_minPreferredLogicalWidth = minPreferredLogicalWidth() * sqrt(float(2));

instead of calling float(2), can you just put a 2.0

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:85
> +    const Vector<String>& notationVal = menclose->notationValues();

ditto about variable name

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:89
> +            setLogicalHeight(logicalHeight() * sqrt(float(2)));

ditto about float

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:100
> +    const Vector<String>& notationVal = menclose->notationValues();

ditto about name

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117
> +            if (notationVal[i] == "updiagonalstrike")

are we missing updiagnalarrow?
https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:137
> +                midxPoint= childLeft-left;

need spaces around -
midxPoint = childLeft - left

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42
> +    virtual void addChild(RenderObject* newChild, RenderObject* beforeChild = 0) override;

beforeChild = nullptr

[Darin Adler]

Comment on attachment 221840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221840&action=review

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:74
>> +            m_minPreferredLogicalWidth = minPreferredLogicalWidth() * sqrt(float(2));
> 
> instead of calling float(2), can you just put a 2.0

sqrt(2) is also fine, but I think that float(M_SQRT2) would be even better since it wouldn’t involve a function call

Created attachment 221948 [details]
Patch

(In reply to comment #136)
> (From update of attachment 221840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221840&action=review
> 
> Thanks for your hard work on this. It's getting close. Maybe you need to update your local sources, which is why the patch doesn't apply
> 
> > Source/WebCore/ChangeLog:66
> > +        its values liek top, left CSSBorder properties are applied and for
> 
> liek misspelled
> 
> > Source/WebCore/ChangeLog:68
> > +        For radical value an anonymous RenderMathMLSquareRoot is created as
> 
> as "a" child of menclose
> 
> > Source/WebCore/mathml/MathMLMencloseElement.cpp:68
> > +    // when notation value is radical and menclose does not have any child then
> 
> when should be capitalized
> 
> the sentence should say "When notation value is "a" radical and ..."
> 
> > Source/WebCore/mathml/MathMLMencloseElement.cpp:79
> > +        if (!val.isEmpty()) {
> 
> i think this line should be an early return
> if (val.isEmpty())
>    return
> 
> > Source/WebCore/mathml/MathMLMencloseElement.cpp:81
> > +            size_t attrvaluesSize = m_notationValues.size();
> 
> this variable should be named
> notationValueSize
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49
> > +    // For radical notation creating an anonymous RenderMathMLSquareRoot.
> 
> This comment should probably be worded like
> 
> "Allow an anonymous RenderMathMLSquareRoot to handle drawing the radical notation, rather than duplicating the code needed to paint a root."
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:70
> > +    const Vector<String>& notationVal = menclose->notationValues();
> 
> I would call this variable notationValues
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:74
> > +            m_minPreferredLogicalWidth = minPreferredLogicalWidth() * sqrt(float(2));
> 
> instead of calling float(2), can you just put a 2.0
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:85
> > +    const Vector<String>& notationVal = menclose->notationValues();
> 
> ditto about variable name
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:89
> > +            setLogicalHeight(logicalHeight() * sqrt(float(2)));
> 
> ditto about float
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:100
> > +    const Vector<String>& notationVal = menclose->notationValues();
> 
> ditto about name
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117
> > +            if (notationVal[i] == "updiagonalstrike")
> 
> are we missing updiagnalarrow?
> https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:137
> > +                midxPoint= childLeft-left;
> 
> need spaces around -
> midxPoint = childLeft - left
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42
> > +    virtual void addChild(RenderObject* newChild, RenderObject* beforeChild = 0) override;
> 
> beforeChild = nullptr

Thanks Chris, Frederic and Darin for the review. So patiently you all helped me with this work.

(In reply to comment #136)
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117
> > +            if (notationVal[i] == "updiagonalstrike")
> 
> are we missing updiagnalarrow?
> https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose
> 
This seems to be newly added. Even mozilla right now doesnot support it. So I guess we can take it up later. updiagnalarrow needs some more work.

[chris fleizach]

(In reply to comment #140)
> (In reply to comment #136)
> > 
> > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117
> > > +            if (notationVal[i] == "updiagonalstrike")
> > 
> > are we missing updiagnalarrow?
> > https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose
> > 
> This seems to be newly added. Even mozilla right now doesnot support it. So I guess we can take it up later. updiagnalarrow needs some more work.

Can you adad a TODO in the code and file a bug? Thanks

[Build Bot]

Comment on attachment 221948 [details]
Patch

Attachment 221948 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6385808982409216

New failing tests:
mathml/presentation/menclose-notation-no-overlap.html

[Build Bot]

Created attachment 221951 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221948 [details]
Patch

Attachment 221948 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4582663599947776

New failing tests:
mathml/presentation/menclose-notation-no-overlap.html

[Build Bot]

Created attachment 221954 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

Created attachment 221955 [details]
Patch

(In reply to comment #141)
> (In reply to comment #140)
> > (In reply to comment #136)
> > > 
> > > > Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:117
> > > > +            if (notationVal[i] == "updiagonalstrike")
> > > 
> > > are we missing updiagnalarrow?
> > > https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose
> > > 
> > This seems to be newly added. Even mozilla right now doesnot support it. So I guess we can take it up later. updiagnalarrow needs some more work.
> 
> Can you adad a TODO in the code and file a bug? Thanks

Done.

[chris fleizach]

Comment on attachment 221955 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 221955 [details]
Patch

Rejecting attachment 221955 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 221955, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
n-no-overlap.html
patching file LayoutTests/mathml/presentation/menclose-notation-radical-expected.html
patching file LayoutTests/mathml/presentation/menclose-notation-radical.html
patching file LayoutTests/mathml/presentation/menclose-remove-children-expected.html
patching file LayoutTests/mathml/presentation/menclose-remove-children.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Chris Fleizach']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/4921392906633216

Created attachment 222409 [details]
Patch

(In reply to comment #150)
> Created an attachment (id=222409) [details]
> Patch

Hi Chris. Could you please set the r+ and cq+. Its the same patch which you reviewed earlier but since patch was a little old cq+ failed :(

[WebKit Commit Bot]

Comment on attachment 222409 [details]
Patch

Clearing flags on attachment: 222409

Committed r162933: <http://trac.webkit.org/changeset/162933>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

Mass change: add WebExposed keyword to help MDN documentation.