Bug 85729 - (menclose) Add support for menclose element
(menclose)
: Add support for menclose element
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: MathML
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: gur.trio
http://www.w3.org/TR/MathML/chapter3....
: WebExposed
Depends on:
Blocks: mathml-in-mathjax
  Show dependency treegraph
 
Reported: 2012-05-06 07:35 PDT by Frédéric Wang (:fredw)
Modified: 2014-03-10 12:25 PDT (History)
29 users (show)

See Also:


Attachments
testcase (741 bytes, application/xhtml+xml)
2012-10-28 21:04 PDT, Jacques Distler
no flags Details
Patch (81.29 KB, patch)
2013-09-28 02:07 PDT, gur.trio
no flags Details | Formatted Diff | Diff
Patch (91.19 KB, patch)
2013-09-28 04:27 PDT, gur.trio
no flags Details | Formatted Diff | Diff
Patch (91.19 KB, patch)
2013-09-28 06:13 PDT, gur.trio
no flags Details | Formatted Diff | Diff
Patch (91.20 KB, patch)
2013-09-28 06:42 PDT, gur.trio
no flags 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 PDT, 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 PDT, Build Bot
no flags Details
Patch (91.88 KB, patch)
2013-09-30 08:17 PDT, gur.trio
no flags Details | Formatted Diff | Diff
Patch (91.89 KB, patch)
2013-09-30 08:37 PDT, gur.trio
no flags 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 PDT, 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 PDT, 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 PDT, Build Bot
no flags Details
Patch (68.23 KB, patch)
2014-01-07 03:18 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (68.22 KB, patch)
2014-01-07 03:33 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (68.19 KB, patch)
2014-01-07 03:51 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (68.93 KB, patch)
2014-01-07 23:51 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (69.76 KB, patch)
2014-01-08 00:02 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (70.15 KB, patch)
2014-01-08 03:36 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (71.76 KB, patch)
2014-01-08 08:17 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (72.53 KB, patch)
2014-01-09 01:52 PST, gur.trio
no flags 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 (:fredw)
no flags Details
Patch (73.81 KB, patch)
2014-01-09 05:53 PST, gur.trio
no flags 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
no flags 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
no flags 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
no flags 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
no flags 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
no flags 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
no flags Details | Formatted Diff | Diff
Patch (84.61 KB, patch)
2014-01-16 01:06 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (86.66 KB, patch)
2014-01-19 23:14 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (87.85 KB, patch)
2014-01-20 20:03 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (86.15 KB, patch)
2014-01-22 02:07 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (83.29 KB, patch)
2014-01-22 22:23 PST, gur.trio
no flags 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
no flags Details | Formatted Diff | Diff
Patch (83.37 KB, patch)
2014-01-27 23:35 PST, gur.trio
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2012-05-06 07:35:09 PDT
"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 Jacques Distler 2012-10-28 21:04:36 PDT
Created attachment 171152 [details]
testcase
Comment 2 Jacques Distler 2012-10-28 21:07:37 PDT
As Frederic notes, several common mathematical notations rely (for their MathML implementations) on menclose.
Comment 3 gur.trio 2013-09-17 20:26:58 PDT
(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 Frédéric Wang (:fredw) 2013-09-18 09:21:42 PDT
(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 Frédéric Wang (:fredw) 2013-09-18 09:34:52 PDT
(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 gur.trio 2013-09-18 20:42:30 PDT
(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 Frédéric Wang (:fredw) 2013-09-19 02:16:22 PDT
(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 gur.trio 2013-09-28 02:07:16 PDT
Created attachment 212887 [details]
Patch
Comment 9 Build Bot 2013-09-28 02:33:59 PDT
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
Comment 10 Build Bot 2013-09-28 02:48:30 PDT
Comment on attachment 212887 [details]
Patch

Attachment 212887 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2613444
Comment 11 gur.trio 2013-09-28 04:27:47 PDT
Created attachment 212896 [details]
Patch
Comment 12 Build Bot 2013-09-28 04:57:13 PDT
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
Comment 13 Build Bot 2013-09-28 05:08:48 PDT
Comment on attachment 212896 [details]
Patch

Attachment 212896 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2692288
Comment 14 gur.trio 2013-09-28 06:13:52 PDT
Created attachment 212897 [details]
Patch
Comment 15 Build Bot 2013-09-28 06:39:20 PDT
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
Comment 16 gur.trio 2013-09-28 06:42:12 PDT
Created attachment 212899 [details]
Patch
Comment 17 Build Bot 2013-09-28 08:24:19 PDT
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
Comment 18 Build Bot 2013-09-28 08:24:23 PDT
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
Comment 19 Build Bot 2013-09-28 09:51:20 PDT
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
Comment 20 Build Bot 2013-09-28 09:51:23 PDT
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
Comment 21 Sam Weinig 2013-09-28 12:16:03 PDT
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.
Comment 22 gur.trio 2013-09-30 08:17:38 PDT
Created attachment 212996 [details]
Patch
Comment 23 gur.trio 2013-09-30 08:18:27 PDT
(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.
Comment 24 EFL EWS Bot 2013-09-30 08:23:12 PDT
Comment on attachment 212996 [details]
Patch

Attachment 212996 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2774067
Comment 25 EFL EWS Bot 2013-09-30 08:24:14 PDT
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
Comment 26 gur.trio 2013-09-30 08:37:55 PDT
Created attachment 212998 [details]
Patch
Comment 27 Build Bot 2013-09-30 09:27:05 PDT
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
Comment 28 Build Bot 2013-09-30 09:27:09 PDT
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
Comment 29 Build Bot 2013-09-30 09:48:16 PDT
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
Comment 30 Build Bot 2013-09-30 09:48:19 PDT
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
Comment 31 Build Bot 2013-09-30 10:50:18 PDT
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
Comment 32 Build Bot 2013-09-30 10:50:21 PDT
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
Comment 33 gur.trio 2013-10-02 03:41:58 PDT
(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?
Comment 34 gur.trio 2013-10-04 02:21:11 PDT
(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
Comment 35 gur.trio 2013-10-09 04:03:13 PDT
(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
Comment 36 Frédéric Wang (:fredw) 2013-10-09 04:05:45 PDT
(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 Frédéric Wang (:fredw) 2013-10-09 11:44:54 PDT
(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 chris fleizach 2013-10-15 09:41:43 PDT
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
Comment 39 gur.trio 2013-11-17 23:50:58 PST
(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?
Comment 40 Frédéric Wang (:fredw) 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 gur.trio 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 Frédéric Wang (:fredw) 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 gur.trio 2014-01-07 03:18:21 PST
Created attachment 220506 [details]
Patch
Comment 44 kov's GTK+ EWS bot 2014-01-07 03:26:20 PST
Comment on attachment 220506 [details]
Patch

Attachment 220506 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5469383690289152
Comment 45 EFL EWS Bot 2014-01-07 03:29:54 PST
Comment on attachment 220506 [details]
Patch

Attachment 220506 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4658955305353216
Comment 46 gur.trio 2014-01-07 03:33:53 PST
Created attachment 220507 [details]
Patch
Comment 47 EFL EWS Bot 2014-01-07 03:39:38 PST
Comment on attachment 220507 [details]
Patch

Attachment 220507 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5386102932242432
Comment 48 EFL EWS Bot 2014-01-07 03:43:27 PST
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
Comment 49 gur.trio 2014-01-07 03:51:36 PST
Created attachment 220508 [details]
Patch
Comment 50 gur.trio 2014-01-07 05:07:50 PST
(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.
Comment 51 Frédéric Wang (:fredw) 2014-01-07 06:32:43 PST
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...
Comment 52 Frédéric Wang (:fredw) 2014-01-07 06:40:58 PST
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>

?
Comment 53 chris fleizach 2014-01-07 09:12:27 PST
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 (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 gur.trio 2014-01-07 23:51:04 PST
Created attachment 220597 [details]
Patch
Comment 55 gur.trio 2014-01-08 00:02:39 PST
Created attachment 220602 [details]
Patch
Comment 56 gur.trio 2014-01-08 02:42:15 PST
(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?
Comment 57 Frédéric Wang (:fredw) 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 gur.trio 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 gur.trio 2014-01-08 03:36:55 PST
Created attachment 220622 [details]
Patch
Comment 60 gur.trio 2014-01-08 08:17:31 PST
Created attachment 220635 [details]
Patch
Comment 61 EFL EWS Bot 2014-01-08 08:22:51 PST
Comment on attachment 220635 [details]
Patch

Attachment 220635 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6371233004257280
Comment 62 EFL EWS Bot 2014-01-08 08:26:00 PST
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
Comment 63 Build Bot 2014-01-08 08:53:45 PST
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
Comment 64 Build Bot 2014-01-08 09:06:19 PST
Comment on attachment 220635 [details]
Patch

Attachment 220635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5921713137123328
Comment 65 chris fleizach 2014-01-08 09:07:32 PST
(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.
Comment 66 chris fleizach 2014-01-08 09:15:55 PST
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
Comment 67 Frédéric Wang (:fredw) 2014-01-08 10:47:33 PST
(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...
Comment 68 gur.trio 2014-01-09 01:51:44 PST
(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.
Comment 69 gur.trio 2014-01-09 01:52:17 PST
Created attachment 220703 [details]
Patch
Comment 70 gur.trio 2014-01-09 01:55:50 PST
(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?
Comment 71 EFL EWS Bot 2014-01-09 01:56:18 PST
Comment on attachment 220703 [details]
Patch

Attachment 220703 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6072233084583936
Comment 72 kov's GTK+ EWS bot 2014-01-09 02:00:52 PST
Comment on attachment 220703 [details]
Patch

Attachment 220703 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5445130681057280
Comment 73 Frédéric Wang (:fredw) 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 gur.trio 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 Frédéric Wang (:fredw) 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 EFL EWS Bot 2014-01-09 02:15:39 PST
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
Comment 77 Build Bot 2014-01-09 02:17:31 PST
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
Comment 78 Build Bot 2014-01-09 02:39:14 PST
Comment on attachment 220703 [details]
Patch

Attachment 220703 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5058178957967360
Comment 79 gur.trio 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 Frédéric Wang (:fredw) 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 Frédéric Wang (:fredw) 2014-01-09 03:17:25 PST
Created attachment 220708 [details]
diagram and equation explaining where \sqrt{2} comes from

FYI.
Comment 82 gur.trio 2014-01-09 04:09:43 PST
(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?
Comment 83 Frédéric Wang (:fredw) 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 gur.trio 2014-01-09 05:53:25 PST
Created attachment 220718 [details]
Patch
Comment 85 EFL EWS Bot 2014-01-09 06:01:58 PST
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
Comment 86 Build Bot 2014-01-09 06:47:32 PST
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
Comment 87 Build Bot 2014-01-09 06:47:40 PST
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
Comment 88 Build Bot 2014-01-09 07:09:16 PST
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
Comment 89 Build Bot 2014-01-09 07:09:25 PST
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
Comment 90 Build Bot 2014-01-09 08:10:24 PST
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
Comment 91 Build Bot 2014-01-09 08:10:33 PST
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
Comment 92 gur.trio 2014-01-10 03:43:46 PST
Created attachment 220830 [details]
Patch
Comment 93 Build Bot 2014-01-10 04:43:17 PST
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
Comment 94 Build Bot 2014-01-10 04:43:23 PST
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
Comment 95 Build Bot 2014-01-10 05:01:42 PST
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
Comment 96 Build Bot 2014-01-10 05:01:51 PST
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
Comment 97 Build Bot 2014-01-10 06:01:31 PST
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
Comment 98 Build Bot 2014-01-10 06:01:40 PST
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
Comment 99 gur.trio 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 Frédéric Wang (:fredw) 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 gur.trio 2014-01-13 05:40:14 PST
Created attachment 221031 [details]
Patch
Comment 102 Build Bot 2014-01-13 06:35:14 PST
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
Comment 103 Build Bot 2014-01-13 06:35:21 PST
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
Comment 104 Build Bot 2014-01-13 06:57:10 PST
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
Comment 105 Build Bot 2014-01-13 06:57:19 PST
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
Comment 106 gur.trio 2014-01-13 07:56:27 PST
Created attachment 221043 [details]
Patch
Comment 107 Build Bot 2014-01-13 08:32:18 PST
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
Comment 108 Build Bot 2014-01-13 08:32:25 PST
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
Comment 109 Build Bot 2014-01-13 08:50:30 PST
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
Comment 110 Build Bot 2014-01-13 08:50:41 PST
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
Comment 111 gur.trio 2014-01-13 22:51:19 PST
Created attachment 221107 [details]
Patch
Comment 112 Build Bot 2014-01-13 23:46:35 PST
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
Comment 113 Build Bot 2014-01-13 23:46:42 PST
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
Comment 114 Build Bot 2014-01-14 00:06:12 PST
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
Comment 115 Build Bot 2014-01-14 00:06:21 PST
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
Comment 116 gur.trio 2014-01-14 00:26:59 PST
Created attachment 221115 [details]
Patch
Comment 117 Build Bot 2014-01-14 01:28:31 PST
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
Comment 118 Build Bot 2014-01-14 01:28:39 PST
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
Comment 119 Build Bot 2014-01-14 01:45:12 PST
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
Comment 120 Build Bot 2014-01-14 01:45:22 PST
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
Comment 121 gur.trio 2014-01-14 01:49:58 PST
Created attachment 221127 [details]
Patch
Comment 122 gur.trio 2014-01-14 04:38:06 PST
(In reply to comment #121)
> Created an attachment (id=221127) [details]
> Patch

Finally the build is done. :) Please review.
Comment 123 Frédéric Wang (:fredw) 2014-01-15 09:16:21 PST
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.
Comment 124 gur.trio 2014-01-16 01:06:47 PST
Created attachment 221345 [details]
Patch
Comment 125 gur.trio 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 gur.trio 2014-01-16 01:11:44 PST
(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?
Comment 127 Frédéric Wang (:fredw) 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 gur.trio 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 gur.trio 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 gur.trio 2014-01-17 05:15:47 PST
(In reply to comment #124)
> Created an attachment (id=221345) [details]
> Patch

Hi Chris. Can you please have a look? Thanks
Comment 131 gur.trio 2014-01-19 23:14:48 PST
Created attachment 221618 [details]
Patch
Comment 132 gur.trio 2014-01-20 20:03:19 PST
Created attachment 221716 [details]
Patch
Comment 133 gur.trio 2014-01-21 03:16:22 PST
(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
Comment 134 Frédéric Wang (:fredw) 2014-01-21 11:34:33 PST
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/
Comment 135 gur.trio 2014-01-22 02:07:47 PST
Created attachment 221840 [details]
Patch
Comment 136 chris fleizach 2014-01-22 09:32:56 PST
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
Comment 137 Darin Adler 2014-01-22 10:41:09 PST
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
Comment 138 gur.trio 2014-01-22 22:23:54 PST
Created attachment 221948 [details]
Patch
Comment 139 gur.trio 2014-01-22 23:00:27 PST
(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.
Comment 140 gur.trio 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 chris fleizach 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 Build Bot 2014-01-22 23:42:11 PST
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
Comment 143 Build Bot 2014-01-22 23:42:18 PST
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
Comment 144 Build Bot 2014-01-23 00:18:35 PST
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
Comment 145 Build Bot 2014-01-23 00:18:46 PST
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
Comment 146 gur.trio 2014-01-23 00:19:51 PST
Created attachment 221955 [details]
Patch
Comment 147 gur.trio 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 chris fleizach 2014-01-24 13:03:22 PST
Comment on attachment 221955 [details]
Patch

r=me
Comment 149 WebKit Commit Bot 2014-01-27 21:45:58 PST
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
Comment 150 gur.trio 2014-01-27 23:35:01 PST
Created attachment 222409 [details]
Patch
Comment 151 gur.trio 2014-01-28 01:00:09 PST
(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 :(
Comment 152 WebKit Commit Bot 2014-01-28 08:30:18 PST
Comment on attachment 222409 [details]
Patch

Clearing flags on attachment: 222409

Committed r162933: <http://trac.webkit.org/changeset/162933>
Comment 153 WebKit Commit Bot 2014-01-28 08:30:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 154 Frédéric Wang (:fredw) 2014-03-10 12:25:38 PDT
Mass change: add WebExposed keyword to help MDN documentation.