Bug 553917 - Send MathML parsing errors to the Error Console
 Summary: Send MathML parsing errors to the Error Console
 Status: RESOLVED FIXED [mentor=fredw][lang=c++] dev-doc-complete, student-project Core Components MathML (show other bugs) Trunk All All P5 enhancement with 2 votes (vote) mozilla20 rfw2nd 411227 Show dependency tree / graph

Reported: 2010-03-21 11:22 PDT by Frédéric Wang (:fredw)
Modified: 2013-03-02 10:35 PST (History)
6 users (show)
ryanvm: in‑testsuite+
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Attachments
Errors to console, not final patch, just a progress report. (17.84 KB, patch)
2012-06-04 12:26 PDT, rfw2nd
no flags Details | Diff | Review
Test case for printing mathml errors to console. (1.65 KB, text/plain)
2012-06-04 12:27 PDT, rfw2nd
no flags Details
Updated testcase with better coverage. (2.19 KB, text/plain)
2012-06-04 12:43 PDT, rfw2nd
no flags Details
Patch, round 2. (21.96 KB, patch)
2012-06-06 03:05 PDT, rfw2nd
fred.wang: feedback+
Details | Diff | Review
Initial support for deprecated attribute warnings. (35.58 KB, patch)
2012-06-09 20:37 PDT, rfw2nd
fred.wang: feedback+
Details | Diff | Review
See bugfix-553917-localization.patch as well (39.10 KB, patch)
2012-06-26 17:41 PDT, rfw2nd
no flags Details | Diff | Review
See bugfix-553917-localization.patch as well (34.85 KB, patch)
2012-06-26 17:43 PDT, rfw2nd
karlt: review-
Details | Diff | Review
Patch for changes to localization to support MathML error logging. (4.30 KB, patch)
2012-06-26 17:46 PDT, rfw2nd
karlt: review-
l10n: review-
wgianopoulos: feedback+
Details | Diff | Review
Localization changes to support MathML error logging. (4.02 KB, patch)
2012-09-09 00:22 PDT, rfw2nd
l10n: review-
karlt: review-
Details | Diff | Review
Changes to MathML module to support error logging. (35.08 KB, patch)
2012-09-09 00:24 PDT, rfw2nd
karlt: review-
Details | Diff | Review
Updated error messages. (1.36 KB, patch)
2012-09-26 10:13 PDT, rfw2nd
no flags Details | Diff | Review
Test cases for error logging (4.11 KB, text/plain)
2012-09-26 11:20 PDT, rfw2nd
no flags Details
Localization changes to support MathML error logging. (3.16 KB, patch)
2012-09-27 22:53 PDT, rfw2nd
karlt: review-
karlt: feedback+
Details | Diff | Review
Changes to MATHML module to support MathML error logging. (34.87 KB, patch)
2012-09-27 23:02 PDT, rfw2nd
no flags Details | Diff | Review
Changes to MATHML module to support MathML error logging. (25.19 KB, patch)
2012-09-27 23:08 PDT, rfw2nd
no flags Details | Diff | Review
Changes to MathML module to support error logging. (39.18 KB, patch)
2012-10-05 00:09 PDT, rfw2nd
karlt: review-
karlt: feedback+
Details | Diff | Review
Localization changes to support error logging. (4.25 KB, patch)
2012-10-05 00:10 PDT, rfw2nd
l10n: review+
karlt: review+
Details | Diff | Review
Localization changes to support MathML error logging. (4.32 KB, patch)
2012-10-23 22:22 PDT, rfw2nd
karlt: review+
Details | Diff | Review
Changes to MATHML module to support MathML error logging. (39.35 KB, patch)
2012-10-23 22:29 PDT, rfw2nd
karlt: review+
Details | Diff | Review
Test cases for MathML error logging. (5.01 KB, text/plain)
2012-10-23 22:34 PDT, rfw2nd
no flags Details
Localization changes to support error logging. (4.24 KB, patch)
2012-10-25 09:29 PDT, rfw2nd
l10n: review+
Details | Diff | Review
Changes to MathML module to support error logging. (39.35 KB, patch)
2012-10-25 09:31 PDT, rfw2nd
no flags Details | Diff | Review
Changes to MathML module to support error logging. (39.35 KB, patch)
2012-10-25 09:32 PDT, rfw2nd
fred.wang: review+
Details | Diff | Review
Mochitest for error logging (4.87 KB, patch)
2012-10-30 11:40 PDT, rfw2nd
no flags Details | Diff | Review
Mochitest for error logging (5.21 KB, patch)
2012-10-30 13:11 PDT, rfw2nd
no flags Details | Diff | Review
Mochitest for error logging (6.44 KB, patch)
2012-10-31 10:25 PDT, rfw2nd
no flags Details | Diff | Review
Mochitest for error logging (8.64 KB, patch)
2012-11-07 10:06 PST, rfw2nd
fred.wang: feedback+
Details | Diff | Review
Changes to MathML module to support error logging. (39.53 KB, patch)
2012-11-07 10:12 PST, rfw2nd
karlt: review+
Details | Diff | Review
Mochitest for error logging (10.69 KB, patch)
2012-11-15 11:50 PST, rfw2nd
karlt: review+
Details | Diff | Review
Mochitest for error logging (10.97 KB, patch)
2012-12-13 11:32 PST, rfw2nd
karlt: review-
Details | Diff | Review
Mochitest for error logging (7.54 KB, patch)
2012-12-19 21:30 PST, rfw2nd
no flags Details | Diff | Review
Mochitest for error logging (7.45 KB, patch)
2012-12-19 21:33 PST, rfw2nd
karlt: review+
Details | Diff | Review
Changes to MathML module to support error logging. (39.58 KB, patch)
2012-12-19 21:38 PST, rfw2nd
no flags Details | Diff | Review
Changes to MathML module to support error logging. (39.67 KB, patch)
2012-12-26 16:07 PST, rfw2nd
no flags Details | Diff | Review
Localization changes to support error logging. (4.33 KB, patch)
2012-12-26 16:08 PST, rfw2nd
no flags Details | Diff | Review
Mochitest for error logging (7.54 KB, patch)
2012-12-26 16:09 PST, rfw2nd
no flags Details | Diff | Review

 Frédéric Wang (:fredw) 2010-03-21 11:22:52 PDT In various places of the MathML layout engine, "invalid-markup" is displayed without additional information. It could be useful to send a description to the Error Console such that "too many/few arguments in element %1$S". The work is essentially to add some messages in dom/locales/en-US/chrome/mathml/mathml.properties and to call nsContentUtils::ReportToConsole each time nsMathMLContainerFrame::ReflowError is used. Below is the result of a bash command that looks for ReflowError. It seems that nsMathMLContainerFrame::ReflowError may sometimes display "invalid-markup" even when the markup is valid (for example failure of measurement in nsMathMLContainerFrame::MeasureForWidth). fred@localhost:~/mozilla/src/layout/mathml$ grep ReflowError *.cpp nsMathMLContainerFrame.cpp:nsMathMLContainerFrame::ReflowError(nsIRenderingContext& aRenderingContext, nsMathMLContainerFrame.cpp: ReflowError(*aRenderingContext, desiredSize); nsMathMLmfracFrame.cpp: return ReflowError(aRenderingContext, aDesiredSize); nsMathMLmmultiscriptsFrame.cpp: return ReflowError(aRenderingContext, aDesiredSize); nsMathMLmmultiscriptsFrame.cpp: return ReflowError(aRenderingContext, aDesiredSize); nsMathMLmoverFrame.cpp: return ReflowError(aRenderingContext, aDesiredSize); nsMathMLmrootFrame.cpp: rv = ReflowError(renderingContext, aDesiredSize); nsMathMLmrootFrame.cpp: ReflowError(*aRenderingContext, desiredSize); nsMathMLmsubFrame.cpp: return aFrame->ReflowError(aRenderingContext, aDesiredSize); nsMathMLmsubsupFrame.cpp: return aFrame->ReflowError(aRenderingContext, aDesiredSize); nsMathMLmsupFrame.cpp: return aFrame->ReflowError(aRenderingContext, aDesiredSize); nsMathMLmunderFrame.cpp: return ReflowError(aRenderingContext, aDesiredSize); nsMathMLmunderoverFrame.cpp: return ReflowError(aRenderingContext, aDesiredSize); Frédéric Wang (:fredw) 2010-03-22 05:08:44 PDT To add localization in MathML, see attachment 433814 [details] [diff] [review]. Frédéric Wang (:fredw) 2010-04-04 05:11:51 PDT Another possible feature could be to send warnings when deprecated attributes are used. Frédéric Wang (:fredw) 2011-04-12 03:20:01 PDT As suggested in bug 427990 comment 41, it could also be interesting to send warnings for href/xlink:href when they are supported. rfw2nd 2012-06-02 00:28:22 PDT @fredw: I'm interested in working on this bug. I'll look over the source files you mentioned in the first comment. Frédéric Wang (:fredw) 2012-06-03 13:28:06 PDT (In reply to rfw2nd from comment #4) > @fredw: I'm interested in working on this bug. I'll look over the source > files you mentioned in the first comment. Thank you. I think you'll have to make the error messages localizable, so comment 1 may also give you an idea on how to do that. If you need help for this bug, you can use IRC (https://wiki.mozilla.org/IRC). I'm generally on the #mathml canal. rfw2nd 2012-06-04 12:26:20 PDT Created attachment 629879 [details] [diff] [review] Errors to console, not final patch, just a progress report. Not a final patch; just wanted to post some progress to be evaluated. What I've done so far is to add a helper method, ReportErrorToConsole to nsMathMLContainerFrame, which calls nsContentUtils::ReportToConsole and passes some information common to each error (such as the document URI). Also, error messages which can be localized have been added to mathml.properties rfw2nd 2012-06-04 12:27:42 PDT Created attachment 629880 [details] Test case for printing mathml errors to console. Test case. Includes several situations with invalid markup, valid markup. The invalid markup is designed to trigger ReflowError. rfw2nd 2012-06-04 12:43:56 PDT Created attachment 629889 [details] Updated testcase with better coverage. Frédéric Wang (:fredw) 2012-06-04 13:49:24 PDT Comment on attachment 629879 [details] [diff] [review] Errors to console, not final patch, just a progress report. Review of attachment 629879 [details] [diff] [review]: ----------------------------------------------------------------- Thank you Raymond. The general idea of the patch looks good, so I'm only giving small remarks. Note that for work-in-progress patches, you can use the feedback flag. When you think the patch is ready you may ask a review to Karl Tomlinson (owner of the MathML module). You'll probably need another review from a i18n peer. I'd suggest you to have a look at Mozilla's coding style guide: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide In particular: - don't include whitespace changes in your patch - use spaces instead of tabs and verify horizontal alignment - use line length of 80 characters or less - verify spacing around operators Here are other ideas for this bug: * generate warnings for deprecated features: - xlink:href (bug 427990 comment 41) - http://www.w3.org/TR/MathML3/chapter3.html#presm.deprecatt (in content/mathml/content/src/nsMathMLElement.cpp) - use of unitless attribute values http://www.w3.org/TR/MathML3/chapter2.html#fund.units (in nsMathMLElement::ParseNumericValue) * generate warnings for parsing failures: - In nsMathMLmpaddedFrame::ParseAttribute - In nsMathMLFrame::ParseNumericValue * maybe more... I'm wondering if we should make the invalid-markup message localizable. Bug 307830 was resolved WONTFIX, but as a comparison I know that MathJax devs are planning to make error messages localizable... ::: dom/locales/en-US/chrome/mathml/mathml.properties @@ +1,4 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > +TooManyFewArgumentsInElement=Wrong number of arguments to element %1\$S I would call this WrongNumberOfArguments ::: layout/mathml/nsMathMLContainerFrame.cpp @@ +60,5 @@ > aRenderingContext.SetFont(fm); > > // bounding metrics > nsAutoString errorMsg; errorMsg.AssignLiteral("invalid-markup"); > + unnecessary whitespace change ::: layout/mathml/nsMathMLmfracFrame.cpp @@ +202,5 @@ > + const PRUnichar* argv[] ={mContent->NodeInfo()->NodeName().get()}; > + ReportErrorToConsole("TooManyFewArgumentsInElement", > + argv, > + 1); > + Maybe here and below, you can do the same as in nsMathMLmmultiscriptsFrame::Place ::: layout/mathml/nsMathMLmmultiscriptsFrame.cpp @@ +250,5 @@ > if (childFrame->GetContent()->Tag() == nsGkAtoms::mprescripts_) { > if (mprescriptsFrame) { > // duplicate found > // report an error, encourage people to get their markups in order > + ReportErrorToConsole( "DuplicateMprescripts"); for example, here you don't need a space after the left parenthesis rfw2nd 2012-06-06 03:05:17 PDT Created attachment 630487 [details] [diff] [review] Patch, round 2. I went through the patch and eliminated any whitespace changes I could find. Also removed "too many children in mroot" in favor of "wrong number of arguments to mroot." based on your suggestion. :) I'm moving forward on getting the Parse Error warnings for invalid attributes as well. Right now, it is outputting a parse error to the console when an invalid numerical value is parsed. Frédéric Wang (:fredw) 2012-06-06 04:24:21 PDT Comment on attachment 630487 [details] [diff] [review] Patch, round 2. Review of attachment 630487 [details] [diff] [review]: ----------------------------------------------------------------- Thank you Raymond. You're making good progress. I'm looking forward to seeing warnings for deprecated features. Below are stylistic remarks (mostly). ::: layout/mathml/nsMathMLContainerFrame.cpp @@ +1528,5 @@ > + errorMsgId, > + argv, > + argc, > + mContent->GetDocument()->GetDocumentURI()); > +} You have to indent correctly to align the arguments passed to the function. So that should be + return nsContentUtils::ReportToConsole(nsIScriptError::errorFlag, + "MathML", Idem for other places in the code. @@ +1537,5 @@ > +{ > + const PRUnichar * argv[] = > + {attribute, > + elemName, > + value}; I'm not really sure what is the best way to pass arguments to this fonction, what is most appropriate string format and the best way to create this PRUnichar* table. It would be better to ask Karl. @@ +1544,5 @@ > + nsContentUtils::eMATHML_PROPERTIES, > + "ErrorParsingValue", > + argv, > + 3, > + mContent->GetDocument()->GetDocumentURI()); two spaces indent. ::: layout/mathml/nsMathMLContainerFrame.h @@ +262,5 @@ > + */ > + nsresult > + ReportErrorToConsole(const char *errorMsgId, > + const PRUnichar **argv=nsnull, > + PRUint32 argc=0); I think Mozilla's style is one space before and after the equal sign ::: layout/mathml/nsMathMLmfracFrame.cpp @@ +198,5 @@ > if (frameNum) > frameDen = frameNum->GetNextSibling(); > if (!frameNum || !frameDen || frameDen->GetNextSibling()) { > // report an error, encourage people to get their markups in order > + const PRUnichar* argv[] ={mContent->NodeInfo()->NodeName().get()}; I think Mozilla's convention is one space before and after the braces. I'm not sure why here and in some places you use a table with a single element element, while in nsMathMLmmultiscriptsFrame::Place you simply use a pointer to this element (the latter seems simpler to me). ::: layout/mathml/nsMathMLmsupFrame.cpp @@ +111,5 @@ > supScriptFrame = baseFrame->GetNextSibling(); > if (!baseFrame || !supScriptFrame || supScriptFrame->GetNextSibling()) { > // report an error, encourage people to get their markups in order > + const PRUnichar* argv[] = {aFrame->GetContent()->NodeInfo()->NodeName().get()}; > + aFrame->ReportErrorToConsole("WrongNumberOfArguments",argv,1); There are still tabs here. rfw2nd 2012-06-08 15:06:19 PDT I'm running into an interesting problem: It looks like nsMathMLContentFrame & its descendents not only get instantiated multiple times for each element, but Place() gets called multiple times, which makes the error messages appear more than once for each invalid-markup error. For example, if I put: x, I'll get about 6 "Wrong number of arugments..." errors in the console. Considering your typical use case will probably be more complex than a single element, it could confuse the user into thinking they have many more errors than actually exist. The only solution coming to my mind right now is putting in an extra validation step in nsMathMLElement... which could catch the invalid markup before it gets pushed to layout. Am I on the right track? rfw2nd 2012-06-08 15:07:39 PDT (In reply to rfw2nd from comment #12) > I'm running into an interesting problem: It looks like nsMathMLContentFrame > & its descendents not only get instantiated multiple times for each element, > but Place() gets called multiple times, which makes the error messages > appear more than once for each invalid-markup error. For example, if I put: > x, I'll get about 6 "Wrong number of arugments..." > errors in the console. Considering your typical use case will probably be > more complex than a single element, it could confuse the user into thinking > they have many more errors than actually exist. The only solution coming to > my mind right now is putting in an extra validation step in > nsMathMLElement... which could catch the invalid markup before it gets > pushed to layout. Am I on the right track? edit: nsMathMLContentFrame -> nsMathMLContainerFrame Frédéric Wang (:fredw) 2012-06-09 01:42:18 PDT Maybe you can check aPlaceOrigin to reduce a bit this number, but there are still many situations where a reflow can happen. Not sure it's worth moving the checking on the content/ side... rfw2nd 2012-06-09 13:39:15 PDT Hmm.. Maybe a medium between the two, checking aPlaceOrigin to reduce the number of erorrs provided, and providing some extra contextual information; like a string for which the user can search. Right now it just says "Wrong number of arguments to msup.", but, something like "Error in x: Wrong number of arguments to msup." would be far less confusing. rfw2nd 2012-06-09 19:50:17 PDT (In reply to Frédéric Wang (:fredw) from comment #14) > Maybe you can check aPlaceOrigin to reduce a bit this number, but there are > still many situations where a reflow can happen. Not sure it's worth moving > the checking on the content/ side... I agree; it would introduce more complexity later, when support for new mathml fetures are added, because then new code to validate them would have to be added. I tried to see if I could get the original markup that caused the error so it could be displayed, but.. that information gets discarded by the time it reaches the layout/ stage. Adding aPlaceOrigin reduced the amount of duplicate error messages to about 2 per incident. (assuming the user doesn't do anything that would require Reflow, such as resizing the browser window.) In other news, deprecation warnings are now in place, I'll update the patch shortly :). rfw2nd 2012-06-09 20:37:25 PDT Created attachment 631717 [details] [diff] [review] Initial support for deprecated attribute warnings. Added messages for for deprecated attributes. Frédéric Wang (:fredw) 2012-06-10 00:40:55 PDT Comment on attachment 631717 [details] [diff] [review] Initial support for deprecated attribute warnings. Review of attachment 631717 [details] [diff] [review]: ----------------------------------------------------------------- Can you please move the changes relevant to i18n reviewers (mathml.properties and jar.mn) in a separate patch? Numeric values parsed on the layout/ side are treated differently than those parsed on the content/ side. Also, other attribute values are not verified at all on the layout/ side. I don't think we need to add a detailed error for each attribute parsing failure. That would be adding too much code in my opinion and we will have to add a new error message each time we implement a new attribute. For numeric attributes, you can simplify your code by adding an intermediate nsMathMLElement::ParseNumericValueWithLogging function that calls nsMathMLElement::ParseNumericValue and do the logging if the parsing fails. You can then remove the changes from nsMathMLFrame::ParseNumericValue and nsMathMLElement::MapMathMLAttributesInto. mpadded is a bit special, so I'm fine with keeping different errors for each attribute. It's not a problem for deprecated attributes either, because we will hopefully eventually remove them. So I think what remains to do is: - a warning in nsMathMLElement::ParseNumericValue the fact that unitless values are deprecated - a warning in nsMathMLElement::IsLink about the fact that XLink's href is deprecated in favor of MathML3's href. Then I think you will be ready to ask a first review to Karl. ::: content/mathml/content/src/nsMathMLElement.cpp @@ +881,5 @@ > + const nsString& aFavoredAttribute, > + nsIDocument* aDocument) > +{ > + const PRUnichar *argv[] = {aDeprecatedAttribute.get(), > + aFavoredAttribute.get()}; There should be a space after "{" and before "}". Idem elsewhere ::: layout/mathml/nsMathMLmsupFrame.cpp @@ +113,5 @@ > // report an error, encourage people to get their markups in order > + if (aPlaceOrigin) { > + const PRUnichar* arg = > + aFrame->GetContent()->NodeInfo()->NodeName().get(); > + aFrame->ReportErrorToConsole("WrongNumberOfArguments",&arg,1); need one space after the commas. Idem elsewhere. ::: layout/mathml/nsMathMLmsupFrame.h @@ +37,5 @@ > nscoord aScriptSpace); > > protected: > nsMathMLmsupFrame(nsStyleContext* aContext) : nsMathMLContainerFrame(aContext) {} > + Please remove the whitespace changes in this file. rfw2nd 2012-06-14 23:55:10 PDT @fredw: I noticed that some of the values in MapMathMLAttributesInto are non-numeric (such as color names and such). Would it be worthwhile in your opinion to add Parse...WithLogging() helper methods for those values too, since we are logging parse errors in the case of numeric values? Also: I think I have a fair understanding of the codebase now, but, I'm having trouble with locating exactly where the "mode", and "display" attributes of the tag are parsed and discovered. I wanted to add a deprecated attribute warning for "mode", since it is deprecated in favor of "display".. however... I can find places in the code where it appears to be using values derived from those attributes, but nothing that actually looks for them and parses them, at least under the content/mathml and layout/mathml areas. XRef: https://developer.mozilla.org/en/MathML/Element/math#Attributes Frédéric Wang (:fredw) 2012-06-15 01:06:56 PDT (In reply to rfw2nd from comment #19) > @fredw: I noticed that some of the values in MapMathMLAttributesInto are > non-numeric (such as color names and such). Would it be worthwhile in your > opinion to add Parse...WithLogging() helper methods for those values too, > since we are logging parse errors in the case of numeric values? In my opinion, you can just ignore them. There are plenty of non-numeric values on the layout side too, and I don't think we want to add a verification for each of these values. > > Also: I think I have a fair understanding of the codebase now, but, I'm > having trouble with locating exactly where the "mode", and "display" > attributes of the [itex] tag are parsed and discovered. I wanted to add a > deprecated attribute warning for "mode", since it is deprecated in favor of > "display".. however... I can find places in the code where it appears to be > using values derived from those attributes, but nothing that actually looks > for them and parses them, at least under the content/mathml and > layout/mathml areas. > XRef: https://developer.mozilla.org/en/MathML/Element/math#Attributes These attributes (as well as fontstyle and fontweight) are simply implemented via the CSS stylesheet layout/mathml/mathml.css. Maybe you can add warnings in nsMathMLElement::ParseAttribute? Bill Gianopoulos [:WG9s] 2012-06-17 11:43:20 PDT (In reply to rfw2nd from comment #17) > Created attachment 631717 [details] [diff] [review] > Initial support for deprecated attribute warnings. > > Added messages for for deprecated attributes. I am not sure what you are looking for from feedback from me. I am not really a peer in this or any related code module. What I can do is now that you have feedback+ from Fred, add this to my nightly build patches and do some testing on my own and report back results. I am not sure that is what the feedback flag is meant for, but if that is what you were asking for I can certainly oblige! rfw2nd 2012-06-26 17:41:34 PDT Created attachment 636951 [details] [diff] [review] See bugfix-553917-localization.patch as well rfw2nd 2012-06-26 17:43:35 PDT Created attachment 636952 [details] [diff] [review] See bugfix-553917-localization.patch as well Please ignore the previous patch submitted just a minute ago.. I made a mistake and included the wrong file. rfw2nd 2012-06-26 17:46:53 PDT Created attachment 636953 [details] [diff] [review] Patch for changes to localization to support MathML error logging. Bill Gianopoulos [:WG9s] 2012-06-26 18:44:41 PDT So I finally got your previous patch passed the merge conflicts with Fred's patch queue so you cancelled the feedback request? I think give up on trying to include this in my builds. rfw2nd 2012-06-26 20:07:24 PDT Sorry, I didn't realize marking the old patch as obsolete would cancel the feedback request. Mark Banner (:standard8) 2012-06-27 00:51:40 PDT Comment on attachment 636953 [details] [diff] [review] Patch for changes to localization to support MathML error logging. Sorry, I'm not an appropriate reviewer for mathml. According to https://wiki.mozilla.org/Modules/Core#MathML you want Karl or Robert, redirecting review. You will want to add localisation notes on those strings though, so that localisers know what the variables are replaced by (look in other .properties files for examples of LOCALIZATION NOTE). Frédéric Wang (:fredw) 2012-06-27 05:11:26 PDT Comment on attachment 636952 [details] [diff] [review] See bugfix-553917-localization.patch as well Review of attachment 636952 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Raymond! I just add a comment below. Also, this link may be useful e.g. to format your commit mesage: https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in ::: content/mathml/content/src/nsMathMLElement.cpp @@ +105,5 @@ > + WarnDeprecated(NS_LITERAL_STRING("mode"), > + NS_LITERAL_STRING("display"), > + OwnerDoc()); > + } > + Here you might want to add a warning for "fontstyle" and "fontweight" too. http://www.w3.org/TR/MathML3/chapter3.html#presm.deprecatt Bill Gianopoulos [:WG9s] 2012-06-27 16:09:52 PDT Comment on attachment 636953 [details] [diff] [review] Patch for changes to localization to support MathML error logging. Review of attachment 636953 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I have builds available for testing at http://www.wg9s.com/mozilla/firefox/ for Linux, Windows and Android that include this patch. Would it be possible to include the line number of the source file? Only issue I see is that I have no idea how to display the error console on and Android native build. As far as I can figure out the product as built can not do it and no extension exists either. I guess they figure people using tablets don;t care about we development? Bill Gianopoulos [:WG9s] 2012-06-27 16:49:11 PDT  My testing of this has uncovered a different issue. I seem to get multiple sets of errors from a single load of a test page. It would seem that perhaps we parse MathML elements multiple times to display the same page? Not an issue with this patch, but would seem to be an issue in any case. I got 3 sets of errors from loading this testcase: https://bugzilla.mozilla.org/attachment.cgi?id=629889 Bill Gianopoulos [:WG9s] 2012-06-27 17:29:53 PDT (In reply to Bill Gianopoulos [:WG9s] from comment #29) y issue I see is that I have no idea how to display the error console on > and Android native build. As far as I can figure out the product as built > can not do it and no extension exists either. I guess they figure people > using tablets don;t care about we development? It seems these messages go to the log so I need to use catlog to see them so I can make sure that is working. The fact that there is no web or error console on native android is the subject of another bug I will file. Frédéric Wang (:fredw) 2012-06-29 02:58:53 PDT (In reply to Bill Gianopoulos [:WG9s] from comment #30) > My testing of this has uncovered a different issue. I seem to get multiple > sets of errors from a single load of a test page. It would seem that > perhaps we parse MathML elements multiple times to display the same page? > Not an issue with this patch, but would seem to be an issue in any case. > > I got 3 sets of errors from loading this testcase: > > https://bugzilla.mozilla.org/attachment.cgi?id=629889 Yes, that's what we discussed above. There are two kinds of errors: those that happen during parsing (on the content/ side) and those that happen during rendering (on the layout/ side). I guess the latter are displayed only once (or when the DOM tree is changed) while the former are displayed each time we reflow the page. Personally, when I debug a web page, I often see the same error repeated in the console. Duplicate errors are grouped together. I don't really worry how many there are, I just try to fix the page, clear the console and reload the page to see if they happen again. We can move everything on the content/ side, but I think some (non-MathML) errors are already raised from the layout/ side. Also, I think it's best to raise the errors near the place where they happen. As for the line numbers, I don't think it's always possible to get them (and certainly not on the layout side). In most places of the Mozilla source code where errors are sent to the console, line numbers are not passed. Axel Hecht [:Pike] 2012-07-23 05:31:12 PDT Comment on attachment 636953 [details] [diff] [review] Patch for changes to localization to support MathML error logging. Review of attachment 636953 [details] [diff] [review]: ----------------------------------------------------------------- As Mark already said, please add localization notes that describe the paramaters. There's some brief documentation on those on https://developer.mozilla.org/en/Localization_notes. Also, usually en-US has their paramaters ordered as they appear in the string. See below. If you want a more semantic ordering, I'd go element, attr name, attr value. ::: dom/locales/en-US/chrome/mathml/mathml.properties @@ +7,5 @@ > +DuplicateMprescripts=Duplicate found. > +SubSupMismatch=Subscript/Superscript mismatch in %1S. > + > +#Warnings > +ErrorParsingValue=An error occurred while parsing the value, %3S, of attribute %1S, in element %2S. The order of strings seems rather random here. Also, that you're talking about an error in a warning is odd. Maybe use "failure"? Karl Tomlinson (ni?:karlt) 2012-08-03 01:30:06 PDT Comment on attachment 636953 [details] [diff] [review] Patch for changes to localization to support MathML error logging. Sorry about the delay getting back to you here. These are quite large changes for a first contribution, thank you. (In reply to Axel Hecht [:Pike] from comment #33) Also, that you're talking about an error in a warning is odd. CSS has set a precedent here with "Error in parsing value for 'ATTR'. Declaration dropped, for example. nsCSSScanner::OutputError() also uses nsIScriptError::warningFlag, perhaps because it is a generic method. However, I wonder whether these should actually be "errors" for MathML. I don't see the spec making a distinction between unexpected attributes and unexpected numbers of elements. The only relevant text I found was the following. http://www.w3.org/TR/MathML3/chapter2.html#fund.attval talks about "allowed values" for attributes. http://www.w3.org/TR/MathML3/chapter2.html#interf.error says "If a MathML-input-conformant application receives input containing one or more elements with an illegal number or type of attributes or child schemata, it should nonetheless attempt to render all the input in an intelligible way, i.e., to render normally those parts of the input that were valid, and to render error messages (rendered as if enclosed in an merror element) in place of invalid expressions." I expect warning level is appropriate for deprecated features, but I wonder whether all others, including unrecognized attribute values, should be errors? Note the '' quotes that CSS uses for its warnings. I think they would be useful in these messages also. "Error in parsing value for" could be used here too I think. It is a little more concise. >+AttributeDeprecated=The attribute %1S is deprecated; %2S should be used in its place. Can this be modified a bit to make it clearer that %2 is not necessarily a direct replacement. Perhaps "deprecated, superseded by '%2S'." Karl Tomlinson (ni?:karlt) 2012-08-03 01:32:35 PDT Comment on attachment 636952 [details] [diff] [review] See bugfix-553917-localization.patch as well > Added some warnings for deprecated attributes; began adding more parse error warnings, converage is as of yet incomplete. Include the bug number in the first line of the commit message. No need to say "converage is as of yet incomplete". >For also see bugfix-553917-localization.patch Once these patches are committed, they will no longer have names, so no point including them in the commit message. The localization patch should land first, as this patch depends on it. >+ if (aAttribute == nsGkAtoms::mode) { >+ WarnDeprecated(NS_LITERAL_STRING("mode"), >+ NS_LITERAL_STRING("display"), >+ OwnerDoc()); >+ } Can this warning be issued only if Tag() is math? >+bool >+nsMathMLElement::ParseNumericValueWithLogging(const nsString& aString, >+ nsCSSValue& aCSSValue, >+ PRUint32 aFlags, >+ nsIDocument* aDocument) >+{ >+ bool rv = ParseNumericValue(aString,aCSSValue,aFlags); >+ if (!rv) ParseNumericValue can log (if PARSE_ALLOW_UNITLESS) even when not called via the -WithLogging interface, so I assume the two methods exist just because the nsIDocument is not available in some situations (rather than because in some cases we want to avoid logging). If so, then I think ParseNumericValue can have the additional nsIDocument parameter, but that can be set to nullptr where necessary. If it helps to have separate functions to handle the multiple return points, then use a local function that is not on the public interface. The name |rv| is usually only used for nsresult types. Here it could be something like |success|. >+ /** >+ * Helper method to log a warning to the console for deprecated attributes. >+ * @param aDeprecatedAttribute The deprecated attribute's name. >+ * @param aFavoredAttribute The new attribute >+ * @param aDocument The document in which the deprecated attribute was used. >+ */ >+ static nsresult WarnDeprecated(const nsString& aDeprecatedAttribute, >+ const nsString& aFavoredAttribute, >+ nsIDocument* aDocument = nsnull); >+ >+ /** >+ * Helper method to log a warning to the console for parse errors. >+ * @param aValue The value for which the parse error occured. >+ * @param aDocument The document in which the parse error occured. >+ */ >+ static nsresult WarnParseError(const nsString& aValue, >+ nsIDocument* aDocument = nsnull); Looks like these are only used with nsMathMLElement. If so, better to make these internal-linkage file-static functions. > nsMathMLContainerFrame(nsStyleContext* aContext) : nsContainerFrame(aContext) {} >- > NS_DECL_QUERYFRAME Avoid whitespace changes. >+ WarnParseError(const PRUnichar* aAttribute, >+ const PRUnichar* aValue, >+ const PRUnichar* aElemName); Alignment. Similarly elsewhere. >+ /** >+ Outputs a warning to the console in the event of a parse error while >+ parsing attribute/value pairs. >+ @param aAttribute The attribute for which the parse error occured. >+ @param aValue The value for which the parse error occured. >+ @param aElemName The name of the element for which the error is relevant. >+ @return NS_OK if successful, an error code corresponding to any error that occurs. >+ */ >+ nsresult >+ WarnParseError(const PRUnichar* aAttribute, >+ const PRUnichar* aValue, >+ const PRUnichar* aElemName); >+ >+ /** >+ helper to call ReportToConsole when an error occurs. >+ @param aParams see nsContentUtils::ReportToConsole >+ @param aParams see nsContentUtils::ReportToConsole >+ */ Inconsistent comment format. Duplicate lines. >+nsresult nsMathMLContainerFrame::ReportErrorToConsole(const char *errorMsgId, >+ const PRUnichar **aParams, >+ PRUint32 aParamCount) >+{ >+ return nsContentUtils::ReportToConsole(nsIScriptError::errorFlag, >+ "MathML", mContent->GetDocument(), >+ nsContentUtils::eMATHML_PROPERTIES, >+ errorMsgId, >+ aParams, >+ aParamCount, >+ mContent >+ ->GetDocument()->GetDocumentURI()); >+} >+ >+nsresult nsMathMLContainerFrame::WarnParseError(const PRUnichar* aAttribute, >+ const PRUnichar* aValue, >+ const PRUnichar* aElemName) >+{ >+ >+ const PRUnichar * argv[] = { aAttribute,aElemName,aValue }; >+ >+ return nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, >+ "MathML", mContent->GetDocument(), >+ nsContentUtils::eMATHML_PROPERTIES, >+ "ErrorParsingValue", >+ argv, >+ 3, >+ mContent->GetDocument()->GetDocumentURI()); Usually the function name starts on a new line. That will help with aligning the function parameter list. nsIContent::GetDocument says it is deprecated, and it looks like it could return NULL. I guess you want OwnerDoc(), which shouldn't return NULL. No need to pass mContent->GetDocument()->GetDocumentURI as well as aDocument because "If aURI is null, then aDocument->GetDocumentURI() is used." Less code wins over explicitness here. Similarly elsewhere. Is these should both be error rather than warning, then use nsContainerFrame::ReportErrorToConsole from WarnParseError (and rename WarnParseError). > if (!nsMathMLElement::ParseNumericValue(aString, cssValue, aFlags)) { > // Invalid attribute value. aLengthValue remains unchanged, so the default >- // length value is used. >+ // length value is used. Extra whitespace shouldn't be added here. >+ nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, >+ "MathML", >+ aPresContext->Document(), >+ nsContentUtils::eMATHML_PROPERTIES, >+ "InvalidNumericValue", I don't think InvalidNumericValue is defined. >+ (const PRUnichar**) &aString, reinterpret_casts like this are making assumptions that may not be valid. .get() can get the PRUnichar* and that can be assigned to a local array. >+ const PRUnichar* arg= mContent->NodeInfo()->NodeName().get(); Need a space before '='. mContent->NodeName().get() is simpler. Similarly elsewhere. >- return ReflowError(aRenderingContext, aDesiredSize); >+ if (aPlaceOrigin) { >+ const PRUnichar* arg = mContent->NodeInfo()->NodeName().get(); >+ ReportErrorToConsole("SubSupMismatch", >+ &arg, >+ 1); >+ return ReflowError(aRenderingContext, aDesiredSize); >+ } I assume ReflowError needs to be returned even when !aPlaceOrigin, so as to request the correct size/space for the frame. Can you collapse the parameters in new function calls please, except where necessary to wrap to stay with 80 columns or to aid legibility. Sometimes it can be helpful to choose the break points to keep related arguments (such as array pointer and length) together, or to make boolean tests involving function calls easier to read. You can also break after :: if that helps. >+ WarnParseError(NS_LITERAL_STRING("lspace").get(), >+ value.get(), >+ NS_LITERAL_STRING("mpadded").get()); Align parameters with the first parameter. i.e. inside the '('. I think nsGkAtoms::lspace_->GetUTF16String() would return a PRUnichar, which may be a little prettier than NS_LITERAL_STRING().get(). >+ if (!ParseAttribute(value, mVerticalOffsetSign, mVerticalOffset, >+ mVerticalOffsetPseudoUnit)) { >+ WarnParseError(NS_LITERAL_STRING("voffset").get(),value.get(),NS_LITERAL_STRING("mpadded").get()); >+ } Need to wrap arguments here. There are also a number of function calls that need spaces after the commas. >+ const PRUnichar* arg = mContent->NodeInfo()->NodeName().get(); >+ const PRUnichar * arg = aFrame->GetContent()->NodeInfo()->NodeName().get(); The first form with only one space beside the "*" is preferred. >+ const PRUnichar* arg = >+ aFrame->GetContent()->NodeInfo()->NodeName().get(); Indent the second line here (if it doesn't fit on the same line now). > if (!baseFrame || !underFrame || underFrame->GetNextSibling()) { > // report an error, encourage people to get their markups in order >+ if (aPlaceOrigin) { >+ const PRUnichar* arg = mContent->NodeInfo()->NodeName().get(); >+ ReportErrorToConsole("WrongNumberOfArguments", >+ &arg, >+ 1); >+ } > return ReflowError(aRenderingContext, aDesiredSize); > } > } > if (tag == nsGkAtoms::mover_) { > if (!baseFrame || !overFrame || overFrame->GetNextSibling()) { > // report an error, encourage people to get their markups in order >+ if (aPlaceOrigin) { >+ const PRUnichar* arg = mContent->NodeInfo()->NodeName().get(); >+ ReportErrorToConsole("WrongNumberOfArguments", >+ &arg, >+ 1); >+ } > return ReflowError(aRenderingContext, aDesiredSize); > } > } > if (tag == nsGkAtoms::munderover_) { > if (!baseFrame || !underFrame || !overFrame || overFrame->GetNextSibling()) { > // report an error, encourage people to get their markups in order >+ if (aPlaceOrigin) { >+ const PRUnichar* arg = mContent->NodeInfo()->NodeName().get(); >+ ReportErrorToConsole("WrongNumberOfArguments", >+ &arg, >+ 1); >+ } > return ReflowError(aRenderingContext, aDesiredSize); > } > } Indentation issues here. There's repeated code here, so instead set a bool haveError variable based on baseFrame underFrame etc, and have just one block to report the error. Actually, this pattern is repeated enough that its probably better to have a single ReportArgumentsError function on nsMathMLContainerFrame that gets the node name and calls ReportErrorToConsole. Karl Tomlinson (ni?:karlt) 2012-08-03 01:37:17 PDT The best way to merge the patch with the changes from Bug 626472 is to s/\bnsnull\b/nullptr/g through the whole patch before reapplying. rfw2nd 2012-08-09 16:27:09 PDT @karlt: Thank you for the detailed review! One question, regarding ParseNumericValueWithLogging: "ParseNumericValue can log (if PARSE_ALLOW_UNITLESS) even when not called via the -WithLogging interface, so I assume the two methods exist just because the nsIDocument is not available in some situations (rather than because in some cases we want to avoid logging). If so, then I think ParseNumericValue can have the additional nsIDocument parameter, but that can be set to nullptr where necessary." ==> Maybe we can remove ParseNumericValueWithLogging entirely, and just add a |bool logging| parameter to turn logging on that defaults to false in addition to the nsIDocument* parameter? Since it's kind of misleading to have a |ParseNumericValue| and a |ParseNumericValueWithLogging| because it gives the assumption that |ParseNumericValue| does not log. Karl Tomlinson (ni?:karlt) 2012-08-09 17:07:45 PDT Are there any callers that really don't want to log messages? My first preference would be for a single Method without the |logging| parameter. rfw2nd 2012-08-10 19:22:03 PDT (In reply to Karl Tomlinson (:karlt) from comment #38) > Are there any callers that really don't want to log messages? > > My first preference would be for a single Method without the |logging| > parameter. There are some instances where it would make sense not to log. For instance, when parsing attributes that can have either a numerical value or a string value, it depends on the result of ParseNumericValue to determine what type of value it is. We could, instead of having us log with the extra parameter, just not log when the nsIDocument* parameter is null? Karl Tomlinson (ni?:karlt) 2012-08-11 01:08:52 PDT Yes, that's fine if the nsIDocument is always available in other situations and the effect of the null parameter is documented, or another option is add a PARSE_SUPPRESS_WARNINGS flag. rfw2nd 2012-08-11 17:32:22 PDT I like the idea of the PARSE_SUPPRESS_WARNINGS flag also. It'll entail some more changes, but the behaviour is communicated at a glance, instead of having to read the documentation and remember that "null here means no warnings will be displayed." :) rfw2nd 2012-08-17 22:18:03 PDT Ok. made some changes to the code side.. still need to clean up the patch a bit before I submit it. On the localization side I'm a bit confused as to what comments I should be leaving. I read the description on the web, but.. are the comments supposed to detail what each %xS represents? Or are they just supposed to provide general info to help better translate the message? I'm wondering now if it would be best just to copy the CSS warnings nearly verbaitm.. just for consistency? Any thoughts on that idea? Thanks, sorry if any questions sound dumb or obvious. Karl Tomlinson (ni?:karlt) 2012-08-19 13:42:42 PDT Consistency with the CSS warnings sounds good. The extra information in these MathML warnings is helpful though so I wouldn't suggest removing that. Re localization notes, I don't know what the norm is (CSS warnings have no notes), but the goal is that someone doing the translation, who may know nothing about MathML, can translate without looking up where the strings are used. Perhaps a header note explaining that these messages show up in the Error Console to give feedback to MathML authors. "arguments" has more than one meaning in English, so perhaps mention that "children of" would also be appropriate, or otherwise explain. Re "Duplicate found", perhaps explain that mprescripts should not be translated. "duplicate" can imply identical copies, so perhaps "More than one found in " might be better. Some strings may be clear enough to be self documenting. Can "Invalid Value" be more specific? "Invalid numeric value for attribute" or similar perhaps. rfw2nd 2012-08-22 07:28:52 PDT @karlt: Would it also be better to mark specifically the errors that cause 'invalid-markup' to be displayed. Perhaps with a prefix, "Invalid Markup: More than one '' found in ''"? Karl Tomlinson (ni?:karlt) 2012-08-22 15:10:09 PDT Possibly better. I don't really have a preference. If doing that then I think "markup" should not be capitalized. I notice that we don't localize the 'invalid-markup' message, but that need not influence the decision here. Frédéric Wang (:fredw) 2012-08-22 15:39:14 PDT (In reply to Karl Tomlinson (:karlt) from comment #45) > I notice that we don't localize the 'invalid-markup' message, but that need > not influence the decision here. See bug 307830 that was resolved WONTFIX. However, I think if we add these MathML parsing errors and make them localizable, that would make sense to make the "invalid-markup" message localalizable too. rfw2nd 2012-09-09 00:22:37 PDT Created attachment 659552 [details] [diff] [review] Localization changes to support MathML error logging. rfw2nd 2012-09-09 00:24:08 PDT Created attachment 659553 [details] [diff] [review] Changes to MathML module to support error logging. Depends on patch #659552 Axel Hecht [:Pike] 2012-09-13 07:16:36 PDT Comment on attachment 659552 [details] [diff] [review] Localization changes to support MathML error logging. Review of attachment 659552 [details] [diff] [review]: ----------------------------------------------------------------- Many of the notes in comment 33 still stand, also see below. In general, I think these messages could be more descriptive, in particular mention "MathML" every now and then. Authors might have a document with all kinds of html5 content, with just some fragments of mathml in them. ::: dom/locales/en-US/chrome/mathml/mathml.properties @@ +3,5 @@ > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +# These messages appear in the error console in order to assist MathML authors > + > +# LOCALIZATION NOTE: "Invalid markup:" should not be localized. This is misunderstanding the other bug. This isn't as much jargon as the "invalid-markup" prefix was. @@ +10,5 @@ > +DuplicateMprescripts=Invalid markup: More than one '' found in ''. > +SubSupMismatch=Invalid markup: Subscript/Superscript mismatch in '%1S'. > + > +# Everything after this line is logged as a warning. > +# LOCALIZATION NOTE (AttributeDeprecated): %1S and %2S refer to attribute names. Put the localization note next to the entity it's for, please. @@ +11,5 @@ > +SubSupMismatch=Invalid markup: Subscript/Superscript mismatch in '%1S'. > + > +# Everything after this line is logged as a warning. > +# LOCALIZATION NOTE (AttributeDeprecated): %1S and %2S refer to attribute names. > +AttributeParsingError=In element '%2S': Error parsing the value, '%3S', of attribute '%1S'. I commented about the order here before, see comment 33. rfw2nd 2012-09-13 09:53:46 PDT I'll be submitting for feedback before the next review as there is more work to be done on this patch. It might be a while before I can get around to resubmitting again as school is in session, but my workload so far has been moderate. @karlt: What's your opinion, in order to provide more context information in the parse errors, of adding a new struct? Say ParseErrorContextInfo, with nsIDocument*, and nsGkAtom members for the element and attribute names, then having ParseNumericValue take that as a parameter, instead of nsIDocument*? Either that, or just adding the extra nsGkAtom parameters to ParseNumericValue with sensible default values? That way I can eliminate "ValueParsingError", which is honestly the most vague of the error messages. @Pike: * Should I be specifying the elements, and attributes, are MathML elements? Should the word 'MathML' appear in every error message? * Is the prefix 'Invalid markup: ' sufficient, and would it be better to replace it with something more descriptive, such as: "Too many children in '%1S'; invalid-markup will be displayed in its place."? Karl Tomlinson (ni?:karlt) 2012-09-13 21:26:56 PDT Comment on attachment 659552 [details] [diff] [review] Localization changes to support MathML error logging. (In reply to rfw2nd from comment #50) > @karlt: What's your opinion, in order to provide more context information > in the parse errors, of adding a new struct? There needs to be compromise here between including every piece of identifying information, and not adding copious code to pass around information that will rarely be used. I think the patch as it is now is a good compromise. Let's just to try to be as clear as we can with the information we have. Messages that specify the tag name don't need to explicitly mention MathML. It would probably be helpful to do so in other messages. >+WrongNumberOfArguments=Invalid markup: Wrong number of arguments to element '%1S' Perhaps <%1S/> instead of '' quotes to clarify that this is a tag, which would also be useful to translators. Change the order of words I think. "to <%1S> element.", because the parameter is a generic tag rather than a specific id. There should be a full stop '.' for consistency. >+DuplicateMprescripts=Invalid markup: More than one '' found in ''. '' quotes are not really necessary here, given the . Was this following style from somewhere else? >+SubSupMismatch=Invalid markup: Subscript/Superscript mismatch in '%1S'. <%1S/> instead of '' quotes, I think. This is only for mmultiscripts, so no need to pass the tag as a parameter. Perhaps a localization note to explain that the a subscript is expected for each superscript and vice versa could help. >+# Everything after this line is logged as a warning. What do you think about what I said in comment 34 about making most of these messages errors? >+AttributeParsingError=In element '%2S': Error parsing the value, '%3S', of attribute '%1S'. %2S is not an id either so I'd move it to before the word "element" as above. The clause should be separated with a ',' rather than a ':', and so "error" would be regularly spaced and not capitalized. > "ValueParsingError", which is honestly the most vague of the error messages. >+ValueParsingError=Error in parsing the value '%1S'. How about "Error parsing '%1S' in MathML length attribute."? >+AttributeDeprecated='%1S' is deprecated; superseded by '%2S'. Use a comma rather than semi-colon to separate a participle clause such as here. > +UnitlessValuesAreDeprecated=Unitless values are deprecated in MathML 3. > --- a/dom/locales/jar.mn > +++ b/dom/locales/jar.mn There's something odd about this patch. Each other modified file has a corresponding "diff [...]" line. Karl Tomlinson (ni?:karlt) 2012-09-13 21:28:15 PDT Comment on attachment 659553 [details] [diff] [review] Changes to MathML module to support error logging. This is looking close. >+WarnDeprecated(const PRUnichar* aDeprecatedAttribute, >+ const PRUnichar* aFavoredAttribute, >+ nsIDocument* aDocument) Inconsistent alignment >+ if (!(aFlags & PARSE_SUPPRESS_WARNINGS)) { >+ WarnParseError(aString,aDocument); >+ } Need a space after ',' for all these WarnParseError calls, and some other calls. >- ParseNumericValue(value->GetStringValue(), *scriptMinSize, 0); >+ ParseNumericValue(value->GetStringValue(), >+ *scriptMinSize, >+ 0, >+ aData->mPresContext->Document()); I don't want to split each function call argument onto separate lines as it spreads out the code unnecessarily reducing the amount of code that can be viewed together. See comment 35 re collapsing parameters, but I didn't describe that very clearly. > static bool ParseNumericValue(const nsString& aString, > nsCSSValue& aCSSValue, >- uint32_t aFlags); >+ uint32_t aFlags, >+ nsIDocument* aDocument = nullptr); If the last parameter were required (i.e. the default nullptr were removed), how many call sites would need to add the nullptr argument? The advantage of being more explicit hear would be that if someone copied a function call to another place in the code, it would be clearer that they could fill in that parameter if available. >+ const PRUnichar* argv[] = { aAttribute, >+ mContent->Tag()->GetUTF16String(), >+ aValue }; Alignment. >+} > //========================== Leave the blank line here. >+ /** >+ Outputs a warning to the console in the event of a parse error while >+ /** >+ Helper function that reports "Wrong Number of Arguments" errors. >+ /** >+ helper to call ReportToConsole when an error occurs. Inconsistent comment format. > if (!nsMathMLElement::ParseNumericValue(aString, cssValue, aFlags)) { > // Invalid attribute value. aLengthValue remains unchanged, so the default > // length value is used. >+ const PRUnichar* argv[] = {aString.get()}; >+ nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, >+ "MathML", >+ aPresContext->Document(), >+ nsContentUtils::eMATHML_PROPERTIES, >+ "ValueParsingError", >+ argv, >+ 1); Wouldn't nsMathMLElement::ParseNumericValue issue exactly the same message, if the nsIDocument were provided? > return ReflowError(aRenderingContext, aDesiredSize); >+ > } No extra blank line here, please. rfw2nd 2012-09-22 22:58:09 PDT @karlt: My thought was that error level would be for the invalid-markup messages because content cannot be displayed at all. Whereas, warning level would suffice for errors in parsing attribute values, etc because content would still be displayed (incorrectly of course). I see your point though. It's a bit counter-intuitive to have the word "error" in a warning-level message. In all honesty, I'm neutral on the issue, code-wise it's just a matter of changing one flag. As far as the ValueParsingError... It's tricky to add extra information as to what type of value it tried to parse. When the error "Failed to parse the value 'blah'" is shown, has trickled down through various attempts to parse the value before finally failing. (i.e. first it tries to parse it as a number, then a string when that fails.) In these situations the difficult part is to capture whether the user intended to provide a numerical or string value. Karl Tomlinson (ni?:karlt) 2012-09-23 00:43:04 PDT (In reply to rfw2nd from comment #53) > @karlt: My thought was that error level would be for the invalid-markup > messages because content cannot be displayed at all. Whereas, warning level > would suffice for errors in parsing attribute values, etc because content > would still be displayed (incorrectly of course). I see your point though. > It's a bit counter-intuitive to have the word "error" in a warning-level > message. In all honesty, I'm neutral on the issue, code-wise it's just a > matter of changing one flag. As far as I am aware, it seems that the choice to prevent content being displayed at all for argument errors, but to continue to display something for attribute errors, is simply a decision made in the writing of Gecko's implementation. I'm not aware of the spec making such a distinction. An advantage of using error-level for nsMathMLContainerFrame::WarnParseError is that it can then use nsMathMLContainerFrame::ReportErrorToConsole to reduce the amount of code a little. > As far as the ValueParsingError... It's tricky to add extra information as > to what type of value it tried to parse. When the error "Failed to parse > the value 'blah'" is shown, has trickled down through various attempts to > parse the value before finally failing. (i.e. first it tries to parse it as > a number, then a string when that fails.) In these situations the difficult > part is to capture whether the user intended to provide a numerical or > string value. I had assumed that ValueParsingError was always a length value, whether a numeric or string length, and so the suggestion in comment 51. However, I see there is also scriptlevel. How about having a separate warning message for scriptlevel, so that the length parsing error messages can specify that the error is in a MathML length attribute? That would make the messages a little more specific, without needing to pass any additional info. rfw2nd 2012-09-26 10:13:42 PDT Created attachment 665033 [details] [diff] [review] Updated error messages. How are these for the error messages? Also: (layout/mathml/nsMathMLmpaddedFrame.cpp) ---Line #187-190--- #ifdef DEBUG printf("mpadded: attribute with bad numeric value: %s\n", NS_LossyConvertUTF16toASCII(aString).get()); #endif ------- Should this, and similar statements be removed, since they're basically just echoing the same information displayed in the error console? rfw2nd 2012-09-26 11:20:14 PDT Created attachment 665064 [details] Test cases for error logging Karl Tomlinson (ni?:karlt) 2012-09-26 22:46:16 PDT Comment on attachment 665033 [details] [diff] [review] Updated error messages. >+ChildCountIncorrect=Invalid markup: Incorrect number of children for element <%1S/>. Swap "element" and tag as indicated in comment 51. Similarly elsewhere. >+SubSupMismatch=Invalid markup: Subscript/Superscript mismatch in element <%1S/>. No need pass the tag as a parameter. Use literal "". (comment 51) And a localization note to explain that the a subscript is expected for each superscript and vice versa could help. (comment 51) >+AttributeParsingError=Error in parsing the value for <%2S/>, attribute '%1S': '%3S'. Declaration dropped. "Error in parsing value '%1S' for '%2S' attribute of <%3S/>." "Declaration dropped." is a bit CSS specific. Probably "Attribute ignored". I expect CSS has well defined behavior for unrecognized attributes etc. MathML just calls these out as errors and doesn't define behavior AFAIK. >+ValueParsingError=Error in parsing the MathML length value '%1S'. Declaration dropped. It helps to mention "attribute". "Error in parsing MathML length attribute value '%1S'." Or "Error in parsing '%1S' for MathML length attribute." Karl Tomlinson (ni?:karlt) 2012-09-26 22:47:28 PDT (In reply to rfw2nd from comment #55) > (layout/mathml/nsMathMLmpaddedFrame.cpp) > ---Line #187-190--- > #ifdef DEBUG > printf("mpadded: attribute with bad numeric value: %s\n", > NS_LossyConvertUTF16toASCII(aString).get()); > #endif > ------- > Should this, and similar statements be removed, since they're basically just > echoing the same information displayed in the error console? Yes, please. rfw2nd 2012-09-27 22:53:47 PDT Created attachment 665788 [details] [diff] [review] Localization changes to support MathML error logging. - Added relevant localization comments, an additional error message (NoSubSup). - Changed *ParsingError to format suggested by Karl. - Changed "Declaration dropped." to "Attribute ignored." rfw2nd 2012-09-27 23:02:18 PDT Created attachment 665789 [details] [diff] [review] Changes to MATHML module to support MathML error logging. - Changed ReportArgumentsError to ReportChildCountError() - Shortened several elongated statements. - Added new error message "NoSubSup" which fires when no subscript/superscript pair exists in - Moved deprecation warning about XLink:href from IsLink to SetAttr (to prevent the warning from firing every time one hovers over the link.) - Corrected existing parameter alignment error in nsMathMLElement::ParseNumericValue definition (see nsMathMLElement.h) - Updated array assignments to follow existing convention. rfw2nd 2012-09-27 23:08:06 PDT Created attachment 665792 [details] [diff] [review] Changes to MATHML module to support MathML error logging. Sorry, this is the correct patch. Axel Hecht [:Pike] 2012-10-01 05:59:04 PDT Comment on attachment 665788 [details] [diff] [review] Localization changes to support MathML error logging. Review of attachment 665788 [details] [diff] [review]: ----------------------------------------------------------------- I'm no native speaker, but I found a few nits and rephrased one entry. Feel free to overrule that by native speakers. ::: dom/locales/en-US/chrome/mathml/mathml.properties @@ +4,5 @@ > + > +ChildCountIncorrect=Invalid markup: Incorrect number of children for tag <%1S/>. > +DuplicateMprescripts=Invalid markup: More than one in . > +NoSubSup=Invalid markup: Expected at least one subscript/superscript pair in , found none. > +SubSupMismatch=Invalid markup: Incomplete subscript/superscript pair in . I think there should be only one ' ' after ':' @@ +7,5 @@ > +NoSubSup=Invalid markup: Expected at least one subscript/superscript pair in , found none. > +SubSupMismatch=Invalid markup: Incomplete subscript/superscript pair in . > + > +# LOCALIZATION NOTE: When localizing the single quotes ('), follow the conventions in css.properties for your target locale. > +AttributeParsingError=Error in parsing the value, '%1S', for '%2S' attribute of <%3S/>. Attribute ignored. I wouldn't use both ',' and ''' to separate the value, but just '''. I think we're frowning on too many commas in mozilla land. @@ +9,5 @@ > + > +# LOCALIZATION NOTE: When localizing the single quotes ('), follow the conventions in css.properties for your target locale. > +AttributeParsingError=Error in parsing the value, '%1S', for '%2S' attribute of <%3S/>. Attribute ignored. > +# LOCALIZATION NOTE (ValueParsingError): "length attribute" is not to be interpreted as "an attribute named 'length'". It is a certain type of value. > +ValueParsingError=Error in parsing the MathML length attribute value '%1S'. Attribute ignored. I think we should rephrase this to disambiguate. How about Error parsing attribute value '%1S' as MathML length. Attribute ignored. Can/should we have the attribute name here? @@ +11,5 @@ > +AttributeParsingError=Error in parsing the value, '%1S', for '%2S' attribute of <%3S/>. Attribute ignored. > +# LOCALIZATION NOTE (ValueParsingError): "length attribute" is not to be interpreted as "an attribute named 'length'". It is a certain type of value. > +ValueParsingError=Error in parsing the MathML length attribute value '%1S'. Attribute ignored. > +# LOCALIZATION NOTE(ScriptLevelParsingError): 'scriptlevel' should not be translated. > +ScriptLevelParsingError=Error in parsing the value, '%1S' for 'scriptlevel' attribute. Attribute ignored. I don't think there should be a ',' after "value". Karl Tomlinson (ni?:karlt) 2012-10-03 22:39:40 PDT Comment on attachment 665792 [details] [diff] [review] Changes to MATHML module to support MathML error logging. Can you attach a patch with 8 lines of context and function names, please? https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_do_I_check_stuff_in.3F describes how mercurial can be set up to do this automatically. Karl Tomlinson (ni?:karlt) 2012-10-03 23:11:41 PDT Comment on attachment 665788 [details] [diff] [review] Localization changes to support MathML error logging. (In reply to Axel Hecht [:Pike] from comment #62) > > +# LOCALIZATION NOTE (ValueParsingError): "length attribute" is not to be interpreted as "an attribute named 'length'". It is a certain type of value. > > +ValueParsingError=Error in parsing the MathML length attribute value '%1S'. Attribute ignored. > > I think we should rephrase this to disambiguate. How about > Error parsing attribute value '%1S' as MathML length. Attribute ignored. > > Can/should we have the attribute name here? "MathML length" is perhaps not quite right, because what is a valid length is not consistent across all MathML elements. I'd suggest "Error in parsing MathML attribute value '%1S' as length." Getting the attribute name will overly complicate the code for the value added. Otherwise, Axel's suggestions look good to me. I think different people would put a different number of spaces after a colon where the next clause is a complete sentence, but a quick look through other properties files suggests a single space is more the norm. >+ChildCountIncorrect=Invalid markup: Incorrect number of children for tag <%1S/>. What I meant was to change the order of the words. "Incorrect number of children for <%1S/> tag." is how this would normally be phrased in English. "tag <%1S/>" would be used if there were if %1S identified a single instance. >+NoSubSup=Invalid markup: Expected at least one subscript/superscript pair in , found none. Here "found none" is not a participle clause, but a separate statement like "Attribute dropped". A semicolon would be fine here or it might be more normal to use a full stop '.' and capitalize. >+SubSupMismatch=Invalid markup: Incomplete subscript/superscript pair in . Nice. Karl Tomlinson (ni?:karlt) 2012-10-03 23:17:17 PDT (In reply to Karl Tomlinson (:karlt) from comment #64) > >+NoSubSup=Invalid markup: Expected at least one subscript/superscript pair in , found none. > > Here "found none" is not a participle clause, Well maybe it kind of is, but it doesn't describe a noun in the previous clause. It is a separate statement. rfw2nd 2012-10-05 00:09:03 PDT Created attachment 668351 [details] [diff] [review] Changes to MathML module to support error logging. rfw2nd 2012-10-05 00:10:13 PDT Created attachment 668352 [details] [diff] [review] Localization changes to support error logging. Axel Hecht [:Pike] 2012-10-08 06:15:36 PDT Comment on attachment 668352 [details] [diff] [review] Localization changes to support error logging. Review of attachment 668352 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for hanging in there, looks good to me string wise. Karl Tomlinson (ni?:karlt) 2012-10-10 18:00:35 PDT Comment on attachment 668351 [details] [diff] [review] Changes to MathML module to support error logging. Can you rename ValueParsingError to LengthParsingError, and rename nsMathMLElement::ReportParseError to ReportLengthParseError or similar please? Otherwise we may get someone using the same code for non-length value errors. > float floatValue = str.ToFloat(&errorCode); > // Negative scriptsizemultipliers are not parsed > if (NS_SUCCEEDED(errorCode) && floatValue >= 0.0f) { > scriptSizeMultiplier->SetFloatValue(floatValue, eCSSUnit_Number); >+ } else { >+ ReportParseError(str, aData->mPresContext->Document()); > } Such as here. scriptsizemultiplier is not really a length. Perhaps ScriptLevelParsingError can be generalized (and renamed) to include a parameter for the attribute name. Perhaps "AttributeParsingErrorNoTag". >+} > //========================== Leave the blank line before the //==. >+ /** >+ Helper to call ReportErrorToConsole for parse errors involving >+ /** >+ Helper to call ReportErrorToConsole for ChildCountIncorrect >+ /** >+ Helper to call ReportToConsole when an error occurs. Inconsistent alignment of these comment blocks. Looks like these kind of comments in this file at least are moving towards /** with '*'s down the left hand side of the block. rfw2nd 2012-10-18 16:37:52 PDT Should these errors be added for all of the attributes in MapMathMLAttributesInto? Karl Tomlinson (ni?:karlt) 2012-10-18 16:51:23 PDT I wouldn't report any additional errors for deprecated attributes. Perhaps there are others that could have errors added. rfw2nd 2012-10-22 09:56:09 PDT Looking over it, there are some areas where the code can be cleaned up too. I'll submit a patch with updated testcases.html to see what you think of it as soon as possible (I have midterm exams this week, so it'll probably be sometime next week). I'm thinking: Move some of the parsing work out of MapMathMLAttributes into to their own specific Parse*Value functions. Specifically: The huge block of code for parsing the mathsize attribute with the named values can be moved to "ParseFontSizeValue" (or similar), with error reports added. Right now I'm working on doing the same for the mathcolor and mathbackground blocks: ParseColorValue. As an added plus, it'll make things easier to test and if another attribute which requires a font size argument or a color argument pops up say in MathML 4 or 5, it'll be quite trivial to add it in without copying and pasting code. :) Karl Tomlinson (ni?:karlt) 2012-10-22 14:45:38 PDT Best to move the parsing code around in a separate patch. The patch here is already fairly large. rfw2nd 2012-10-23 22:22:10 PDT Created attachment 674547 [details] [diff] [review] Localization changes to support MathML error logging. Added AttributeParsingErrorNoTag key. Otherwise the same as last time. rfw2nd 2012-10-23 22:29:07 PDT Created attachment 674550 [details] [diff] [review] Changes to MATHML module to support MathML error logging. Refactoring: in nsMathMLElement.cpp: ReportParseError ==> ReportLengthParseError Added: ReportParseErrorNoTag helper to call ReportParseErrorNoTag where appropriate. nsMathMLContainerFrame: Fixed comments to obey convention, removed @return annotations as no other methods have them (and they didn't convey any truely useful information.) rfw2nd 2012-10-23 22:32:25 PDT Comment on attachment 674550 [details] [diff] [review] Changes to MATHML module to support MathML error logging. The only caveat here is that we're not reporting errors for mathfont or mathbackground+mathcolor. I plan to add those messages in the patch with the refactor I spoke of in #72. rfw2nd 2012-10-23 22:34:05 PDT Created attachment 674551 [details] Test cases for MathML error logging. Added test cases for scriptsizemultiplier. Karl Tomlinson (ni?:karlt) 2012-10-24 13:31:41 PDT Comment on attachment 674547 [details] [diff] [review] Localization changes to support MathML error logging. >+AttributeParsingErrorNoTag=Error in parsing the value '%1S' for '%2S' attribute. Attribute ignored. > ValueParsingError=Error in parsing MathML attribute value '%1S' as length. Attribute ignored. > # LOCALIZATION NOTE(ScriptLevelParsingError): 'scriptlevel' should not be translated. >-ScriptLevelParsingError=Error in parsing the value '%1S' for 'scriptlevel' attribute. Attribute ignored. Just need to remove the LOCALIZATION NOTE(ScriptLevelParsingError) that is no longer valid/necessary. Karl Tomlinson (ni?:karlt) 2012-10-24 13:39:41 PDT Comment on attachment 674547 [details] [diff] [review] Localization changes to support MathML error logging. Oh, can you rename ValueParsingError to LengthParsingError too, please? Karl Tomlinson (ni?:karlt) 2012-10-24 13:44:54 PDT Comment on attachment 674550 [details] [diff] [review] Changes to MATHML module to support MathML error logging. Just change "ValueParsingError" in ReportLengthParseError to "LengthParsingError". rfw2nd 2012-10-25 09:29:20 PDT Created attachment 675158 [details] [diff] [review] Localization changes to support error logging. Here it is, with the modifications. :) rfw2nd 2012-10-25 09:31:28 PDT Created attachment 675160 [details] [diff] [review] Changes to MathML module to support error logging. Here it is, with the modifications. :) rfw2nd 2012-10-25 09:32:59 PDT Created attachment 675163 [details] [diff] [review] Changes to MathML module to support error logging. Here it is, with the modifications. :) rfw2nd 2012-10-25 10:15:32 PDT Newbie question: Do I need to set the review? flag on 675163? Bill Gianopoulos [:WG9s] 2012-10-25 11:41:07 PDT (In reply to rfw2nd from comment #84) > Newbie question: Do I need to set the review? flag on 675163? If you already have an r+ that says as long as you make this change and that is the only change in the new patch, then the answer is you do not need to ask for review again. If you made any other change than you probably do need to ask for a new review. Frédéric Wang (:fredw) 2012-10-26 12:06:06 PDT (With Autoland, which is no longer available at the moment, you need to get r+ from someone who has the right to push to the try server. I'm not sure that the bot is clever enough to detect that the new patch is a small modification of the former one) Anyway, what Bill says is true. Here it seems that in addition to renaming ValueParsingError you have modified the order of arguments in nsMathMLContainerFrame::ReportParseError. Is it intentional? rfw2nd 2012-10-26 18:05:59 PDT Yes, the arguments were ordered incorrectly at first. My apologies, I didn't catch it in the testcase before. It would display in the testcase as: Error parsing the value 'width' for attribute 'mpadded' of . I changed it so it would display properly. So should I go ahead and mark it for re-review? Frédéric Wang (:fredw) 2012-10-27 01:54:32 PDT (In reply to rfw2nd from comment #87) > Yes, the arguments were ordered incorrectly at first. My apologies, I > didn't catch it in the testcase before. It would display in the testcase > as: Error parsing the value 'width' for attribute 'mpadded' of . I > changed it so it would display properly. So should I go ahead and mark it > for re-review? Normally if you add other changes, you must ask for a review again. But here it is just a small change and I verified it, so no need for a review again. To catch this kind of error during the development and to detect possible regressions in the future, maybe you should write a javascript-based unit test to verify which errors are sent to the console. I guess you can do that with nsIConsoleListener https://developer.mozilla.org/en-US/docs/Console_service#Application_developer_point_of_view via a xpcshell test or perhaps mochitest https://developer.mozilla.org/en-US/docs/Mozilla_automated_testing Frédéric Wang (:fredw) 2012-10-27 02:20:37 PDT Regarding the localization changes, Karl already gave r+ and the only changes since the latest r+ by Axel are to replace +ValueParsingError=Error in parsing MathML attribute value '%1S' as length. Attribute ignored. +# LOCALIZATION NOTE(ScriptLevelParsingError): 'scriptlevel' should not be translated. +ScriptLevelParsingError=Error in parsing the value '%1S' for 'scriptlevel' attribute. Attribute ignored. by +AttributeParsingErrorNoTag=Error in parsing the value '%1S' for '%2S' attribute. Attribute ignored. +LengthParsingError=Error in parsing MathML attribute value '%1S' as length. Attribute ignored. rfw2nd 2012-10-29 10:43:17 PDT The blocker I'm running into for the automated tests is: If I'm testing the logged string for correctness, how do I make sure the test doesn't explode in a different locale? (Without having to manually program the error messages from the other locale into the test.) I looked at the interfaces nsIConsoleMessage and nsIScrptError, and into the source code for some test cases that were using the console service and couldn't find anything useful in this aspect. Frédéric Wang (:fredw) 2012-10-29 10:52:06 PDT (In reply to rfw2nd from comment #90) > The blocker I'm running into for the automated tests is: If I'm testing the > logged string for correctness, how do I make sure the test doesn't explode > in a different locale? (Without having to manually program the error > messages from the other locale into the test.) I looked at the interfaces > nsIConsoleMessage and nsIScrptError, and into the source code for some test > cases that were using the console service and couldn't find anything useful > in this aspect. Good point. I see three options: - Try to access the localization files from the Javascript and insert the arguments in the right places (not sure these localization files are available from the test framework). - Use the English version, hoping that the tests are only executed in that version. - Match the console output against regular expressions that test only the argument values passed (which are not localized). It seems that the third option would be the easiest. rfw2nd 2012-10-29 15:08:08 PDT Option 1 seems to be possible via nsIStringBundleService. :) I can format the string with the correct order of arguments, then test that the string coming from the console is equal. I think I have a solution for the problem that the console logging is async. :) My workload with school should be fairly light this week, so I might be able to get the testcases done this week. :) rfw2nd 2012-10-30 11:40:26 PDT Created attachment 676688 [details] [diff] [review] Mochitest for error logging I am still having problems with false positives. It looks like the choreography between the console updates and the checking for the message being in the console is imperfect. :( it looks like there is no way to make the console block. The code for the tests still needs to be formatted properly. This is my first mochitest so a lot of it is patched together from other tests. rfw2nd 2012-10-30 13:09:09 PDT I have a solution for the async issue; I'm not sure if I like it, but I am not seeing much in the way of alternatives: - Layout module logs duplicate reflow errors, so counting the error message is out. (we can't say, expect there to be 18 errors. Because of reflow, there are duplicates.) - A static timer would probably cause a lot of false negatives if the system is under load. doTest(); is held back until a timer is up. Initially, the timer has 3 seconds of countdown. Each time a message is added to the error console, the countdown resets. Eventually it will time out, and the test will run with whatever happens to be in the error console at the time. Since we're just checking for the existence of the error messages. My question is: How good is this solution for testing servers, which may be under load? Is 3 seconds a realistic timeout? (I honestly don't like the idea of a test relying on something like an arbitrary timeout. :/) rfw2nd 2012-10-30 13:11:24 PDT Created attachment 676738 [details] [diff] [review] Mochitest for error logging Updated with dynamic timer solution. The timer resets every time a message arrives in the console. Frédéric Wang (:fredw) 2012-10-30 13:49:48 PDT I'm not familiar with the console service or Mochitest, but would it be better to to use a counter incremented in "observe" to determine how many MathML error messages have been sent and only call doTest() once all these messages are sent? If all the expected messages are not sent, SimpleTest.finish() will never be called and I guess the testing framework has some timeout mechanism that will detect the test failure... rfw2nd 2012-10-31 10:25:17 PDT Created attachment 677058 [details] [diff] [review] Mochitest for error logging Made test only run once, added tests for mmultiscripts. Replaced magic numbers with g_timeOut. Added some info messages to describe: Under which condition the test may give a false positive, and a message indicating when the test actually begins. rfw2nd 2012-10-31 10:42:14 PDT fredw: Awesome idea, thanks. It started me thinking that I can rewrite it to do the roll call within the nsIConsoleListener. Inspect each error message that comes in, and update an array of boolean values (a simple counter won't work due to multiple instances of the same reflow error being logged). At the end, a loop iterates through the boolean values, and if they are all true: Call SimpleTest.finish();.. Else, it just keeps going until the test runner times it out. :) Frédéric Wang (:fredw) 2012-11-01 05:57:34 PDT (In reply to rfw2nd from comment #98) > fredw: Awesome idea, thanks. It started me thinking that I can rewrite it > to do the roll call within the nsIConsoleListener. Inspect each error > message that comes in, and update an array of boolean values (a simple > counter won't work due to multiple instances of the same reflow error being > logged). At the end, a loop iterates through the boolean values, and if > they are all true: Call SimpleTest.finish();.. Else, it just keeps going > until the test runner times it out. :) I wanted to propose you to inspect error directly in "observe" but I was not sure how to deal with the order or repetitions of error messages and I thought relying on your existing doTest function was easier. I guess you could determine the actual number of messages, including repetitions, but using a table of boolean as you proposed even just sounds better and more reliable. Axel Hecht [:Pike] 2012-11-01 06:56:55 PDT Comment on attachment 675158 [details] [diff] [review] Localization changes to support error logging. Review of attachment 675158 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks. This also has the changes mentioned in comment 89, right? Frédéric Wang (:fredw) 2012-11-01 07:13:15 PDT (In reply to Axel Hecht [:Pike] from comment #100) > Looks good to me, thanks. This also has the changes mentioned in comment 89, > right? Yes, actually what I meant is that they are exactly the changes added since your last review (I verified with a diff on the two versions of the patch). rfw2nd 2012-11-07 10:06:37 PST Created attachment 679220 [details] [diff] [review] Mochitest for error logging Changed the rollcall to run within the listener. If the test finds all of the error messages, it will pass. Otherwise, it will eventually be stopped by the test runner and thus, fail. rfw2nd 2012-11-07 10:12:32 PST Created attachment 679230 [details] [diff] [review] Changes to MathML module to support error logging. Added check for color attribute in nsMathMLElement::ParseAttribute for special case where DeprecatedSupersededBy was not thrown for color attribute in tag. Needs re-review due to change. Frédéric Wang (:fredw) 2012-11-07 13:15:05 PST `Comment on attachment 679220 [details] [diff] [review] Mochitest for error logging Review of attachment 679220 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, I haven't checked all the tests, but maybe you can make the association between the arrays, the roll* functions and the [itex] trees clearer so that it will be easier to read (for review and in the future if people want to analyze test failures). Perhaps putting each pair of arrays above the corresponding roll* function that handles them. If possible, ordering the [itex] the same order as they appear in the javascript code. And finally copying these [itex]'s in javascript comments. So basically something like: var g_childCountIncorrectStatus = [false, false, false, false, false, false]; var g_childCountIncorrect = ["mroot", "msub", "msup", "mfrac", "msubsup", "munderover"]; // [itex] //  //  //  //  //  function rollCallChildCountIncorrect(msg) { ... } ::: layout/mathml/tests/test_bug553917.html @@ +4,5 @@ > + > + > + > +

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