Bug 85729 - (menclose) Add support for menclose element
(menclose)
: Add support for menclose element
Status: RESOLVED FIXED
: WebKit
MathML
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://www.w3.org/TR/MathML/chapter3....
: WebExposed
:
: 84019
  Show dependency treegraph
 
Reported: 2012-05-06 07:35 PST by
Modified: 2014-03-10 12:25 PST (History)


Attachments
testcase (741 bytes, application/xhtml+xml)
2012-10-28 21:04 PST, Jacques Distler
no flags Details
Patch (81.29 KB, patch)
2013-09-28 02:07 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (91.19 KB, patch)
2013-09-28 04:27 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (91.19 KB, patch)
2013-09-28 06:13 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (91.20 KB, patch)
2013-09-28 06:42 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (508.09 KB, application/zip)
2013-09-28 08:24 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (531.26 KB, application/zip)
2013-09-28 09:51 PST, Build Bot
no flags Details
Patch (91.88 KB, patch)
2013-09-30 08:17 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (91.89 KB, patch)
2013-09-30 08:37 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (518.55 KB, application/zip)
2013-09-30 09:27 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (529.00 KB, application/zip)
2013-09-30 09:48 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (558.60 KB, application/zip)
2013-09-30 10:50 PST, Build Bot
no flags Details
Patch (68.23 KB, patch)
2014-01-07 03:18 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (68.22 KB, patch)
2014-01-07 03:33 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (68.19 KB, patch)
2014-01-07 03:51 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (68.93 KB, patch)
2014-01-07 23:51 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (69.76 KB, patch)
2014-01-08 00:02 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (70.15 KB, patch)
2014-01-08 03:36 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (71.76 KB, patch)
2014-01-08 08:17 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (72.53 KB, patch)
2014-01-09 01:52 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
diagram and equation explaining where \sqrt{2} comes from (2.81 KB, text/html)
2014-01-09 03:17 PST, Frédéric Wang
no flags Details
Patch (73.81 KB, patch)
2014-01-09 05:53 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (482.25 KB, application/zip)
2014-01-09 06:47 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (513.78 KB, application/zip)
2014-01-09 07:09 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (512.27 KB, application/zip)
2014-01-09 08:10 PST, Build Bot
no flags Details
Patch (73.82 KB, patch)
2014-01-10 03:43 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (471.03 KB, application/zip)
2014-01-10 04:43 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (502.53 KB, application/zip)
2014-01-10 05:01 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (518.20 KB, application/zip)
2014-01-10 06:01 PST, Build Bot
no flags Details
Patch (73.75 KB, patch)
2014-01-13 05:40 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (472.25 KB, application/zip)
2014-01-13 06:35 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (502.38 KB, application/zip)
2014-01-13 06:57 PST, Build Bot
no flags Details
Patch (73.82 KB, patch)
2014-01-13 07:56 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (556.74 KB, application/zip)
2014-01-13 08:32 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (471.09 KB, application/zip)
2014-01-13 08:50 PST, Build Bot
no flags Details
Patch (73.76 KB, patch)
2014-01-13 22:51 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (462.58 KB, application/zip)
2014-01-13 23:46 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (489.03 KB, application/zip)
2014-01-14 00:06 PST, Build Bot
no flags Details
Patch (73.76 KB, patch)
2014-01-14 00:26 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (467.31 KB, application/zip)
2014-01-14 01:28 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (490.77 KB, application/zip)
2014-01-14 01:45 PST, Build Bot
no flags Details
Patch (73.75 KB, patch)
2014-01-14 01:49 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (84.61 KB, patch)
2014-01-16 01:06 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (86.66 KB, patch)
2014-01-19 23:14 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (87.85 KB, patch)
2014-01-20 20:03 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (86.15 KB, patch)
2014-01-22 02:07 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (83.29 KB, patch)
2014-01-22 22:23 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (489.53 KB, application/zip)
2014-01-22 23:42 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (454.18 KB, application/zip)
2014-01-23 00:18 PST, Build Bot
no flags Details
Patch (83.40 KB, patch)
2014-01-23 00:19 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff
Patch (83.37 KB, patch)
2014-01-27 23:35 PST, gur.trio@gmail.com
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-06 07:35:09 PST
"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
------- Comment #1 From 2012-10-28 21:04:36 PST -------
Created an attachment (id=171152) [details]
testcase
------- Comment #2 From 2012-10-28 21:07:37 PST -------
As Frederic notes, several common mathematical notations rely (for their MathML implementations) on menclose.
------- Comment #3 From 2013-09-17 20:26:58 PST -------
(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?
------- Comment #4 From 2013-09-18 09:21:42 PST -------
(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.
------- Comment #5 From 2013-09-18 09:34:52 PST -------
(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
------- Comment #6 From 2013-09-18 20:42:30 PST -------
(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.
------- Comment #7 From 2013-09-19 02:16:22 PST -------
(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.
------- Comment #8 From 2013-09-28 02:07:16 PST -------
Created an attachment (id=212887) [details]
Patch
------- Comment #9 From 2013-09-28 02:33:59 PST -------
(From update of attachment 212887 [details])
Attachment 212887 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2655411
------- Comment #10 From 2013-09-28 02:48:30 PST -------
(From update of attachment 212887 [details])
Attachment 212887 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2613444
------- Comment #11 From 2013-09-28 04:27:47 PST -------
Created an attachment (id=212896) [details]
Patch
------- Comment #12 From 2013-09-28 04:57:13 PST -------
(From update of attachment 212896 [details])
Attachment 212896 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2653449
------- Comment #13 From 2013-09-28 05:08:48 PST -------
(From update of attachment 212896 [details])
Attachment 212896 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2692288
------- Comment #14 From 2013-09-28 06:13:52 PST -------
Created an attachment (id=212897) [details]
Patch
------- Comment #15 From 2013-09-28 06:39:20 PST -------
(From update of attachment 212897 [details])
Attachment 212897 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2660514
------- Comment #16 From 2013-09-28 06:42:12 PST -------
Created an attachment (id=212899) [details]
Patch
------- Comment #17 From 2013-09-28 08:24:19 PST -------
(From update of attachment 212899 [details])
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
------- Comment #18 From 2013-09-28 08:24:23 PST -------
Created an attachment (id=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
------- Comment #19 From 2013-09-28 09:51:20 PST -------
(From update of attachment 212899 [details])
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
------- Comment #20 From 2013-09-28 09:51:23 PST -------
Created an attachment (id=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
------- Comment #21 From 2013-09-28 12:16:03 PST -------
(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.
------- Comment #22 From 2013-09-30 08:17:38 PST -------
Created an attachment (id=212996) [details]
Patch
------- Comment #23 From 2013-09-30 08:18:27 PST -------
(In reply to comment #21)
> (From update of attachment 212899 [details] [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.
------- Comment #24 From 2013-09-30 08:23:12 PST -------
(From update of attachment 212996 [details])
Attachment 212996 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2774067
------- Comment #25 From 2013-09-30 08:24:14 PST -------
(From update of attachment 212996 [details])
Attachment 212996 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2782064
------- Comment #26 From 2013-09-30 08:37:55 PST -------
Created an attachment (id=212998) [details]
Patch
------- Comment #27 From 2013-09-30 09:27:05 PST -------
(From update of attachment 212998 [details])
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
------- Comment #28 From 2013-09-30 09:27:09 PST -------
Created an attachment (id=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
------- Comment #29 From 2013-09-30 09:48:16 PST -------
(From update of attachment 212998 [details])
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
------- Comment #30 From 2013-09-30 09:48:19 PST -------
Created an attachment (id=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
------- Comment #31 From 2013-09-30 10:50:18 PST -------
(From update of attachment 212998 [details])
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
------- Comment #32 From 2013-09-30 10:50:21 PST -------
Created an attachment (id=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
------- Comment #33 From 2013-10-02 03:41:58 PST -------
(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?
------- Comment #34 From 2013-10-04 02:21:11 PST -------
(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
------- Comment #35 From 2013-10-09 04:03:13 PST -------
(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #26)
> > > Created an attachment (id=212998) [details] [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
------- Comment #36 From 2013-10-09 04:05:45 PST -------
(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.
------- Comment #37 From 2013-10-09 11:44:54 PST -------
(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>.
------- Comment #38 From 2013-10-15 09:41:43 PST -------
(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
------- Comment #39 From 2013-11-17 23:50:58 PST -------
(In reply to comment #38)
> (From update of attachment 212998 [details] [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?
------- Comment #40 From 2013-11-18 00:14:46 PST -------
(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.
------- Comment #41 From 2013-11-18 00:22:25 PST -------
(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 ?
------- Comment #42 From 2013-11-18 00:33:51 PST -------
(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.
------- Comment #43 From 2014-01-07 03:18:21 PST -------
Created an attachment (id=220506) [details]
Patch
------- Comment #44 From 2014-01-07 03:26:20 PST -------
(From update of attachment 220506 [details])
Attachment 220506 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5469383690289152
------- Comment #45 From 2014-01-07 03:29:54 PST -------
(From update of attachment 220506 [details])
Attachment 220506 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4658955305353216
------- Comment #46 From 2014-01-07 03:33:53 PST -------
Created an attachment (id=220507) [details]
Patch
------- Comment #47 From 2014-01-07 03:39:38 PST -------
(From update of attachment 220507 [details])
Attachment 220507 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5386102932242432
------- Comment #48 From 2014-01-07 03:43:27 PST -------
(From update of attachment 220507 [details])
Attachment 220507 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5808315397308416
------- Comment #49 From 2014-01-07 03:51:36 PST -------
Created an attachment (id=220508) [details]
Patch
------- Comment #50 From 2014-01-07 05:07:50 PST -------
(In reply to comment #49)
> Created an attachment (id=220508) [details] [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.
------- Comment #51 From 2014-01-07 06:32:43 PST -------
(From update of attachment 220508 [details])
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...
------- Comment #52 From 2014-01-07 06:40:58 PST -------
(From update of attachment 220508 [details])
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>

?
------- Comment #53 From 2014-01-07 09:12:27 PST -------
(From update of attachment 220508 [details])
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 (alex@milowski.com). 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
------- Comment #54 From 2014-01-07 23:51:04 PST -------
Created an attachment (id=220597) [details]
Patch
------- Comment #55 From 2014-01-08 00:02:39 PST -------
Created an attachment (id=220602) [details]
Patch
------- Comment #56 From 2014-01-08 02:42:15 PST -------
(In reply to comment #55)
> Created an attachment (id=220602) [details] [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?
------- Comment #57 From 2014-01-08 02:47:46 PST -------
(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.
------- Comment #58 From 2014-01-08 02:51:00 PST -------
(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 :)
------- Comment #59 From 2014-01-08 03:36:55 PST -------
Created an attachment (id=220622) [details]
Patch
------- Comment #60 From 2014-01-08 08:17:31 PST -------
Created an attachment (id=220635) [details]
Patch
------- Comment #61 From 2014-01-08 08:22:51 PST -------
(From update of attachment 220635 [details])
Attachment 220635 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6371233004257280
------- Comment #62 From 2014-01-08 08:26:00 PST -------
(From update of attachment 220635 [details])
Attachment 220635 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6375007340986368
------- Comment #63 From 2014-01-08 08:53:45 PST -------
(From update of attachment 220635 [details])
Attachment 220635 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4963858120704000
------- Comment #64 From 2014-01-08 09:06:19 PST -------
(From update of attachment 220635 [details])
Attachment 220635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5921713137123328
------- Comment #65 From 2014-01-08 09:07:32 PST -------
(In reply to comment #64)
> (From update of attachment 220635 [details] [details])
> Attachment 220635 [details] [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.
------- Comment #66 From 2014-01-08 09:15:55 PST -------
(From update of attachment 220635 [details])
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
------- Comment #67 From 2014-01-08 10:47:33 PST -------
(In reply to comment #66)
> (From update of attachment 220635 [details] [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...
------- Comment #68 From 2014-01-09 01:51:44 PST -------
(In reply to comment #52)
> (From update of attachment 220508 [details] [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.
------- Comment #69 From 2014-01-09 01:52:17 PST -------
Created an attachment (id=220703) [details]
Patch
------- Comment #70 From 2014-01-09 01:55:50 PST -------
(In reply to comment #66)
> (From update of attachment 220635 [details] [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?
------- Comment #71 From 2014-01-09 01:56:18 PST -------
(From update of attachment 220703 [details])
Attachment 220703 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6072233084583936
------- Comment #72 From 2014-01-09 02:00:52 PST -------
(From update of attachment 220703 [details])
Attachment 220703 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5445130681057280
------- Comment #73 From 2014-01-09 02:03:00 PST -------
(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.
------- Comment #74 From 2014-01-09 02:08:45 PST -------
(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.
------- Comment #75 From 2014-01-09 02:15:04 PST -------
(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.
------- Comment #76 From 2014-01-09 02:15:39 PST -------
(From update of attachment 220703 [details])
Attachment 220703 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5324357945524224
------- Comment #77 From 2014-01-09 02:17:31 PST -------
(From update of attachment 220703 [details])
Attachment 220703 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5621356410437632
------- Comment #78 From 2014-01-09 02:39:14 PST -------
(From update of attachment 220703 [details])
Attachment 220703 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5058178957967360
------- Comment #79 From 2014-01-09 03:06:27 PST -------
(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.
------- Comment #80 From 2014-01-09 03:11:08 PST -------
> (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.
------- Comment #81 From 2014-01-09 03:17:25 PST -------
Created an attachment (id=220708) [details]
diagram and equation explaining where \sqrt{2} comes from

FYI.
------- Comment #82 From 2014-01-09 04:09:43 PST -------
(In reply to comment #81)
> Created an attachment (id=220708) [details] [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?
------- Comment #83 From 2014-01-09 04:15:59 PST -------
(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.
------- Comment #84 From 2014-01-09 05:53:25 PST -------
Created an attachment (id=220718) [details]
Patch
------- Comment #85 From 2014-01-09 06:01:58 PST -------
(From update of attachment 220718 [details])
Attachment 220718 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6644967241940992
------- Comment #86 From 2014-01-09 06:47:32 PST -------
(From update of attachment 220718 [details])
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
------- Comment #87 From 2014-01-09 06:47:40 PST -------
Created an attachment (id=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
------- Comment #88 From 2014-01-09 07:09:16 PST -------
(From update of attachment 220718 [details])
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
------- Comment #89 From 2014-01-09 07:09:25 PST -------
Created an attachment (id=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
------- Comment #90 From 2014-01-09 08:10:24 PST -------
(From update of attachment 220718 [details])
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
------- Comment #91 From 2014-01-09 08:10:33 PST -------
Created an attachment (id=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
------- Comment #92 From 2014-01-10 03:43:46 PST -------
Created an attachment (id=220830) [details]
Patch
------- Comment #93 From 2014-01-10 04:43:17 PST -------
(From update of attachment 220830 [details])
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
------- Comment #94 From 2014-01-10 04:43:23 PST -------
Created an attachment (id=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
------- Comment #95 From 2014-01-10 05:01:42 PST -------
(From update of attachment 220830 [details])
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
------- Comment #96 From 2014-01-10 05:01:51 PST -------
Created an attachment (id=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
------- Comment #97 From 2014-01-10 06:01:31 PST -------
(From update of attachment 220830 [details])
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
------- Comment #98 From 2014-01-10 06:01:40 PST -------
Created an attachment (id=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
------- Comment #99 From 2014-01-10 06:24:05 PST -------
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.
------- Comment #100 From 2014-01-10 06:46:33 PST -------
(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.
------- Comment #101 From 2014-01-13 05:40:14 PST -------
Created an attachment (id=221031) [details]
Patch
------- Comment #102 From 2014-01-13 06:35:14 PST -------
(From update of attachment 221031 [details])
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
------- Comment #103 From 2014-01-13 06:35:21 PST -------
Created an attachment (id=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
------- Comment #104 From 2014-01-13 06:57:10 PST -------
(From update of attachment 221031 [details])
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
------- Comment #105 From 2014-01-13 06:57:19 PST -------
Created an attachment (id=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
------- Comment #106 From 2014-01-13 07:56:27 PST -------
Created an attachment (id=221043) [details]
Patch
------- Comment #107 From 2014-01-13 08:32:18 PST -------
(From update of attachment 221043 [details])
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
------- Comment #108 From 2014-01-13 08:32:25 PST -------
Created an attachment (id=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
------- Comment #109 From 2014-01-13 08:50:30 PST -------
(From update of attachment 221043 [details])
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
------- Comment #110 From 2014-01-13 08:50:41 PST -------
Created an attachment (id=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
------- Comment #111 From 2014-01-13 22:51:19 PST -------
Created an attachment (id=221107) [details]
Patch
------- Comment #112 From 2014-01-13 23:46:35 PST -------
(From update of attachment 221107 [details])
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
------- Comment #113 From 2014-01-13 23:46:42 PST -------
Created an attachment (id=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
------- Comment #114 From 2014-01-14 00:06:12 PST -------
(From update of attachment 221107 [details])
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
------- Comment #115 From 2014-01-14 00:06:21 PST -------
Created an attachment (id=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
------- Comment #116 From 2014-01-14 00:26:59 PST -------
Created an attachment (id=221115) [details]
Patch
------- Comment #117 From 2014-01-14 01:28:31 PST -------
(From update of attachment 221115 [details])
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
------- Comment #118 From 2014-01-14 01:28:39 PST -------
Created an attachment (id=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
------- Comment #119 From 2014-01-14 01:45:12 PST -------
(From update of attachment 221115 [details])
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
------- Comment #120 From 2014-01-14 01:45:22 PST -------
Created an attachment (id=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
------- Comment #121 From 2014-01-14 01:49:58 PST -------
Created an attachment (id=221127) [details]
Patch
------- Comment #122 From 2014-01-14 04:38:06 PST -------
(In reply to comment #121)
> Created an attachment (id=221127) [details] [details]
> Patch

Finally the build is done. :) Please review.
------- Comment #123 From 2014-01-15 09:16:21 PST -------
(From update of attachment 221127 [details])
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.
------- Comment #124 From 2014-01-16 01:06:47 PST -------
Created an attachment (id=221345) [details]
Patch
------- Comment #125 From 2014-01-16 01:07:57 PST -------
> > 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.
------- Comment #126 From 2014-01-16 01:11:44 PST -------
(In reply to comment #124)
> Created an attachment (id=221345) [details] [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?
------- Comment #127 From 2014-01-16 01:22:55 PST -------
(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
------- Comment #128 From 2014-01-16 01:33:52 PST -------
(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)
------- Comment #129 From 2014-01-16 03:31:42 PST -------
(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?
------- Comment #130 From 2014-01-17 05:15:47 PST -------
(In reply to comment #124)
> Created an attachment (id=221345) [details] [details]
> Patch

Hi Chris. Can you please have a look? Thanks
------- Comment #131 From 2014-01-19 23:14:48 PST -------
Created an attachment (id=221618) [details]
Patch
------- Comment #132 From 2014-01-20 20:03:19 PST -------
Created an attachment (id=221716) [details]
Patch
------- Comment #133 From 2014-01-21 03:16:22 PST -------
(In reply to comment #132)
> Created an attachment (id=221716) [details] [details]
> Patch

Patch is not being applied. All changes have been done as per the suggestions. Chris and Frederic can you please review. Thanks
------- Comment #134 From 2014-01-21 11:34:33 PST -------
(From update of attachment 221716 [details])
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/
------- Comment #135 From 2014-01-22 02:07:47 PST -------
Created an attachment (id=221840) [details]
Patch
------- Comment #136 From 2014-01-22 09:32:56 PST -------
(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
------- Comment #137 From 2014-01-22 10:41:09 PST -------
(From update of attachment 221840 [details])
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
------- Comment #138 From 2014-01-22 22:23:54 PST -------
Created an attachment (id=221948) [details]
Patch
------- Comment #139 From 2014-01-22 23:00:27 PST -------
(In reply to comment #136)
> (From update of attachment 221840 [details] [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.
------- Comment #140 From 2014-01-22 23:18:38 PST -------
(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.
------- Comment #141 From 2014-01-22 23:24:09 PST -------
(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
------- Comment #142 From 2014-01-22 23:42:11 PST -------
(From update of attachment 221948 [details])
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
------- Comment #143 From 2014-01-22 23:42:18 PST -------
Created an attachment (id=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
------- Comment #144 From 2014-01-23 00:18:35 PST -------
(From update of attachment 221948 [details])
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
------- Comment #145 From 2014-01-23 00:18:46 PST -------
Created an attachment (id=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
------- Comment #146 From 2014-01-23 00:19:51 PST -------
Created an attachment (id=221955) [details]
Patch
------- Comment #147 From 2014-01-23 00:20:57 PST -------
(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.
------- Comment #148 From 2014-01-24 13:03:22 PST -------
(From update of attachment 221955 [details])
r=me
------- Comment #149 From 2014-01-27 21:45:58 PST -------
(From update of attachment 221955 [details])
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
------- Comment #150 From 2014-01-27 23:35:01 PST -------
Created an attachment (id=222409) [details]
Patch
------- Comment #151 From 2014-01-28 01:00:09 PST -------
(In reply to comment #150)
> Created an attachment (id=222409) [details] [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 :(
------- Comment #152 From 2014-01-28 08:30:18 PST -------
(From update of attachment 222409 [details])
Clearing flags on attachment: 222409

Committed r162933: <http://trac.webkit.org/changeset/162933>
------- Comment #153 From 2014-01-28 08:30:31 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #154 From 2014-03-10 12:25:38 PST -------
Mass change: add WebExposed keyword to help MDN documentation.