Bug 124572

Created attachment 217283 [details] Patch V1

Comment on attachment 217283 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=217283&action=review This is OK, but I see room for improvement, especially on test coverage. > Source/WebCore/mathml/MathMLElement.cpp:81 > + if (name == mathbackgroundAttr || name == mathsizeAttr || name == mathcolorAttr || name == fontsizeAttr || name == backgroundAttr || name == colorAttr || name == fontstyleAttr || name == fontweightAttr || name == fontfamilyAttr || name == dirAttr) I would be more comfortable with this if statement if the attributes were in some obvious order, say alphabetical. Also would be nice if there was a test that would fail if each of these was removed. I am betting that I could remove one of these 10 without any test failing. > Source/WebCore/mathml/MathMLElement.cpp:110 > + if (hasTagName(mathTag) || hasTagName(mstyleTag) || hasTagName(mrowTag) || hasTagName(mtextTag) || hasTagName(msTag) || hasTagName(moTag) || hasTagName(miTag) || hasTagName(mnTag)) I would be more comfortable with this if statement if the tag names were in some obvious order, say alphabetical. Also would be nice if there was a test that would fail if each of these was removed. I am betting that I could remove one of these 8 without any test failing. Also, this might be a good candidate for a named function (can be inline so the code generated is the same), since it’s not obvious to me what these elements have in common. Think of the function name as a good way to comment what this set of tags represents. > Source/WebCore/mathml/mathtags.in:11 > +mstyle interfaceName=MathMLElement I’d like to see test coverage for this. We have tests that check if elements get the right kind of wrapper, and we should cover this tag in that kind of test.

Comment on attachment 217283 [details] Patch V1 Clearing flags on attachment: 217283 Committed r159504: <http://trac.webkit.org/changeset/159504>

All reviewed patches have been landed. Closing bug.

Frédéric: Would you mind posting a secondary patch to incorporate Darin's suggestions for improvements? We can reopen this bug to do so.

OK, I will come back to this tomorrow. Regarding the first set, there should already test coverage for these attributes and test for dir is added here. Regarding the second set, new tests are added here for each element. However, perhaps I should add a test verifying that dir has no effect on other elements like e.g. msqrt.

(In reply to comment #6) > I’d like to see test coverage for this. We have tests that check if elements get the right kind of wrapper, and we should cover this tag in that kind of test. Can you give me an example of such a test so that I can do that for mstyle?

Created attachment 217415 [details] Follow up V1 > if (name == mathbackgroundAttr || name == mathsizeAttr || name == mathcolorAttr || name == fontsizeAttr || name == backgroundAttr || name == colorAttr || name == fontstyleAttr || name == fontweightAttr || name == fontfamilyAttr || name == dirAttr) These were tested by mathml/presentation/attributes-mathvariant, mathsize and background-color. The dir attribute is tested by the test added in the first patch. > if (hasTagName(mathTag) || hasTagName(mstyleTag) || hasTagName(mrowTag) || hasTagName(mtextTag) || hasTagName(msTag) || hasTagName(moTag) || hasTagName(miTag) || hasTagName(mnTag)) These are tested by the new tests, especially direction-overall and direction-token. I've added a test to direction-overall to check that the dir attribute is not applied to <msqrt>. > I’d like to see test coverage for this. We have tests that check if elements get the right kind of wrapper, and we should cover this tag in that kind of test. I have not done that yet.

(In reply to comment #7) > (In reply to comment #6) > > I’d like to see test coverage for this. We have tests that check if elements get the right kind of wrapper, and we should cover this tag in that kind of test. > > Can you give me an example of such a test so that I can do that for mstyle? fast/dom/wrapper-classes.html

Created attachment 217469 [details] Follow up V2 Mmm, MathML does not have a DOM so I'm not really sure I can access the exact class from Javascript. I tried something but it seems to just returns "Element".

(In reply to comment #10) > Mmm, MathML does not have a DOM so I'm not really sure I can access the exact class from Javascript. I tried something but it seems to just returns "Element". I see. I did not know that. We’ll have to test this indirectly by checking for the different behavior caused by the internal DOM class, not by looking at wrappers at all.

Created attachment 217541 [details] Follow up V3

(In reply to comment #11) > I see. I did not know that. > > We’ll have to test this indirectly by checking for the different behavior caused by the internal DOM class, not by looking at wrappers at all. OK, I've removed that test. Although removing one of the presentation attribute would have broken the test for the token elements, there were nothing to check the presentation attributes on <mstyle> so I've added a test for that.

Reopening per comment 5.

(In reply to comment #5) > Frédéric: Would you mind posting a secondary patch to incorporate Darin's suggestions for improvements? We can reopen this bug to do so. Brent, why did you suggest reopening this bug? I thought the best practice for a case like this is to use a new bug report.

(In reply to comment #15) > (In reply to comment #5) > > Frédéric: Would you mind posting a secondary patch to incorporate Darin's suggestions for improvements? We can reopen this bug to do so. > > Brent, why did you suggest reopening this bug? I thought the best practice for a case like this is to use a new bug report. I thought he just had a couple of small changes to incorporate some suggestions you had on his original patch. I thought it would be easier to track this in the same bug, rather than have multiple bugs (and possibly Radars) that track a small set of changes focused on the same topic. But in the future I'll have people leave the bug closed and open a new one.

Comment on attachment 217541 [details] Follow up V3 View in context: https://bugs.webkit.org/attachment.cgi?id=217541&action=review > Source/WebCore/mathml/MathMLElement.cpp:128 > +bool MathMLElement::isMathMLToken() const > +{ > + return hasTagName(miTag) || hasTagName(mnTag) || hasTagName(moTag) || hasTagName(msTag) || hasTagName(mtextTag); > +} Are you sure this is actually getting inlined? In older compilers we had to put the function higher in the file than the call sites, and also mark it inline here, not just in the declaration.

Comment on attachment 217541 [details] Follow up V3 Clearing flags on attachment: 217541 Committed r159680: <http://trac.webkit.org/changeset/159680>

All reviewed patches have been landed. Closing bug.

Mass change: add WebExposed keyword to help MDN documentation.