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);

To add localization in MathML, see attachment 433814 [details] [diff] [review].

Another possible feature could be to send warnings when deprecated attributes are used.

As suggested in bug 427990 comment 41, it could also be interesting to send warnings for href/xlink:href when they are supported.

@fredw: I'm interested in working on this bug. I'll look over the source files you mentioned in the first comment.

(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.

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

Test case. Includes several situations with invalid markup, valid markup. The invalid markup is designed to trigger ReflowError.

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 <mprescripts/> 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

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.

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.

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: <msup><mi>x</mi></msup>, 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?

(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: > <msup><mi>x</mi></msup>, 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

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...

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 <msup><mi>x</mi></msup>: Wrong number of arguments to msup." would be far less confusing.

(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 :).

Added messages for for deprecated attributes.

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.

@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 <math> 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

(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 <math> 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?

(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!

Please ignore the previous patch submitted just a minute ago.. I made a mistake and included the wrong file.

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.

Sorry, I didn't realize marking the old patch as obsolete would cancel the feedback request.

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).

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

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?

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

(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.

(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.

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 <mprescripts/> found. > +SubSupMismatch=Subscript/Superscript mismatch in %1$S. > + > +#Warnings > +ErrorParsingValue=An error occurred while parsing the value, %3$S, of attribute %1$S, in element %2$S. The order of strings seems rather random here. Also, that you're talking about an error in a warning is odd. Maybe use "failure"?

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 %1$S is deprecated; %2$S 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 '%2$S'."

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.

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.

@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.

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.

(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?

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.

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." :)

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 %x$S 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.

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 <mprescripts/> found", perhaps explain that mprescripts should not be translated. "duplicate" can imply identical copies, so perhaps "More than one <mprescripts> found in <mmultiscripts>" 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.

@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 '<mprescripts/>' found in '<mmultiscripts>'"?

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.

(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.

Depends on patch #659552

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 '<mprescripts/>' found in '<mmultiscripts/>'. > +SubSupMismatch=Invalid markup: Subscript/Superscript mismatch in '%1$S'. > + > +# Everything after this line is logged as a warning. > +# LOCALIZATION NOTE (AttributeDeprecated): %1$S and %2$S 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 '%1$S'. > + > +# Everything after this line is logged as a warning. > +# LOCALIZATION NOTE (AttributeDeprecated): %1$S and %2$S refer to attribute names. > +AttributeParsingError=In element '%2$S': Error parsing the value, '%3$S', of attribute '%1$S'. I commented about the order here before, see comment 33.

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 '%1$S'; invalid-markup will be displayed in its place."?

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 '%1$S' Perhaps <%1$S/> 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 <%1$S> 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 '<mprescripts/>' found in '<mmultiscripts/>'. '' quotes are not really necessary here, given the </>. Was this following style from somewhere else? >+SubSupMismatch=Invalid markup: Subscript/Superscript mismatch in '%1$S'. <%1$S/> 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 '%2$S': Error parsing the value, '%3$S', of attribute '%1$S'. %2$S 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 '%1$S'. How about "Error parsing '%1$S' in MathML length attribute."? >+AttributeDeprecated='%1$S' is deprecated; superseded by '%2$S'. 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.

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.

@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.

(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.

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?

Comment on attachment 665033 [details] [diff] [review] Updated error messages. >+ChildCountIncorrect=Invalid markup: Incorrect number of children for element <%1$S/>. Swap "element" and tag as indicated in comment 51. Similarly elsewhere. >+SubSupMismatch=Invalid markup: Subscript/Superscript mismatch in element <%1$S/>. No need pass the tag as a parameter. Use literal "<mmultiscripts\>". (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 <%2$S/>, attribute '%1$S': '%3$S'. Declaration dropped. "Error in parsing value '%1$S' for '%2$S' attribute of <%3$S/>." "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 '%1$S'. Declaration dropped. It helps to mention "attribute". "Error in parsing MathML length attribute value '%1$S'." Or "Error in parsing '%1$S' for MathML length attribute."

(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.

- Added relevant localization comments, an additional error message (NoSubSup). - Changed *ParsingError to format suggested by Karl. - Changed "Declaration dropped." to "Attribute ignored."

- Changed ReportArgumentsError to ReportChildCountError() - Shortened several elongated statements. - Added new error message "NoSubSup" which fires when no subscript/superscript pair exists in <mmultiscripts> - 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.

Sorry, this is the correct patch.

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 <%1$S/>. > +DuplicateMprescripts=Invalid markup: More than one <mprescripts/> in <mmultiscripts/>. > +NoSubSup=Invalid markup: Expected at least one subscript/superscript pair in <mmultiscripts/>, found none. > +SubSupMismatch=Invalid markup: Incomplete subscript/superscript pair in <mmultiscripts/>. I think there should be only one ' ' after ':' @@ +7,5 @@ > +NoSubSup=Invalid markup: Expected at least one subscript/superscript pair in <mmultiscripts/>, found none. > +SubSupMismatch=Invalid markup: Incomplete subscript/superscript pair in <mmultiscripts/>. > + > +# LOCALIZATION NOTE: When localizing the single quotes ('), follow the conventions in css.properties for your target locale. > +AttributeParsingError=Error in parsing the value, '%1$S', for '%2$S' attribute of <%3$S/>. 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, '%1$S', for '%2$S' attribute of <%3$S/>. 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 '%1$S'. Attribute ignored. I think we should rephrase this to disambiguate. How about Error parsing attribute value '%1$S' as MathML length. Attribute ignored. Can/should we have the attribute name here? @@ +11,5 @@ > +AttributeParsingError=Error in parsing the value, '%1$S', for '%2$S' attribute of <%3$S/>. 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 '%1$S'. Attribute ignored. > +# LOCALIZATION NOTE(ScriptLevelParsingError): 'scriptlevel' should not be translated. > +ScriptLevelParsingError=Error in parsing the value, '%1$S' for 'scriptlevel' attribute. Attribute ignored. I don't think there should be a ',' after "value".

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.

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 '%1$S'. Attribute ignored. > > I think we should rephrase this to disambiguate. How about > Error parsing attribute value '%1$S' 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 '%1$S' 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 <%1$S/>. What I meant was to change the order of the words. "Incorrect number of children for <%1$S/> tag." is how this would normally be phrased in English. "tag <%1$S/>" would be used if there were if %1$S identified a single instance. >+NoSubSup=Invalid markup: Expected at least one subscript/superscript pair in <mmultiscripts/>, 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 <mmultiscripts/>. Nice.

(In reply to Karl Tomlinson (:karlt) from comment #64) > >+NoSubSup=Invalid markup: Expected at least one subscript/superscript pair in <mmultiscripts/>, 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.

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.

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.

Should these errors be added for all of the attributes in MapMathMLAttributesInto?

I wouldn't report any additional errors for deprecated attributes. Perhaps there are others that could have errors added.

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. :)

Best to move the parsing code around in a separate patch. The patch here is already fairly large.

Added AttributeParsingErrorNoTag key. Otherwise the same as last time.

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.)

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.

Added test cases for scriptsizemultiplier.

Comment on attachment 674547 [details] [diff] [review] Localization changes to support MathML error logging. >+AttributeParsingErrorNoTag=Error in parsing the value '%1$S' for '%2$S' attribute. Attribute ignored. > ValueParsingError=Error in parsing MathML attribute value '%1$S' as length. Attribute ignored. > # LOCALIZATION NOTE(ScriptLevelParsingError): 'scriptlevel' should not be translated. >-ScriptLevelParsingError=Error in parsing the value '%1$S' for 'scriptlevel' attribute. Attribute ignored. Just need to remove the LOCALIZATION NOTE(ScriptLevelParsingError) that is no longer valid/necessary.

Comment on attachment 674547 [details] [diff] [review] Localization changes to support MathML error logging. Oh, can you rename ValueParsingError to LengthParsingError too, please?

Comment on attachment 674550 [details] [diff] [review] Changes to MATHML module to support MathML error logging. Just change "ValueParsingError" in ReportLengthParseError to "LengthParsingError".

Here it is, with the modifications. :)

Here it is, with the modifications. :)

Here it is, with the modifications. :)

Newbie question: Do I need to set the review? flag on 675163?

(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.

(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?

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 <BAD!/>. I changed it so it would display properly. So should I go ahead and mark it for re-review?

(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 <BAD!/>. 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

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 '%1$S' as length. Attribute ignored. +# LOCALIZATION NOTE(ScriptLevelParsingError): 'scriptlevel' should not be translated. +ScriptLevelParsingError=Error in parsing the value '%1$S' for 'scriptlevel' attribute. Attribute ignored. by +AttributeParsingErrorNoTag=Error in parsing the value '%1$S' for '%2$S' attribute. Attribute ignored. +LengthParsingError=Error in parsing MathML attribute value '%1$S' as length. Attribute ignored.

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.

(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.

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. :)

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.

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. :/)

Updated with dynamic timer solution. The timer resets every time a message arrives in the console.

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...

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.

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. :)

(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.

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?

(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).

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.

Added check for color attribute in nsMathMLElement::ParseAttribute for special case where DeprecatedSupersededBy was not thrown for color attribute in <math/> tag. Needs re-review due to change.

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 <math> 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 <math> the same order as they appear in the javascript code. And finally copying these <math>'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"]; // <math><mroot></mroot></math> // <math><msub></msub></math> // <math><msup></msup></math> // <math><mfrac></mfrac></math> // <math><msubsup></msubsup></math> // <math><munderover></munderover></math> function rollCallChildCountIncorrect(msg) { ... } ::: layout/mathml/tests/test_bug553917.html @@ +4,5 @@ > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> > + <script type="application/javascript"> > + netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); Are the calls to netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect') really needed? I think enablePrivilege is going to be removed and the Mochitest doc says they are executed with chrome privileges. This gives me javascript warnings if I open the page directly in Firefox. @@ +99,5 @@ > + return g_unitlessValuesAreDeprecatedStatus; > + } > + > + > + function doRollCall(msg) { The test passes when all messages are intercepted and fails if some are missing. Perhaps you can make the test fail too if unknown messages are intercepted, although I'm not sure it is what you want that... Currently, I get an error about the fact that the character encoding of the HTML page is not declared. @@ +139,5 @@ > + <math><mo minsize="BADminsize">+</mo></math><BR/> > + <math><mo rspace="BADrspace">+</mo></math><BR/> > + <math><mspace width="BADwidth"/></math><BR/> > + <math><mspace height="BADheight"/></math><BR/> > + <math><mspace depth="BADdepth"/></math><BR/> What is the reason for having these <BR/>? @@ +159,5 @@ > + <math><mpadded height="BAD!"></mpadded></math> > + <math><mpadded voffset="BAD!"></mpadded></math> > + <script class="testbody" type="application/javascript"> > + netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); > + // Catch any messages _not_ caught before the listener is registered. Is it possible? I guess the MathML errors from content/ are sent during the parsing of the <math> trees. I don't know exactly if they can happen before the call to consoleService.registerListener, though.

Ready for review phase. Test passes 100% of the time on my machine. From the last patch: - Improved testing coverage for LengthParsingError. - Integrated comments above rollCall* functions as suggested by :fredw - Misc. changes About the formatting: I tried to adhere to 80 columns per line where possible without hampering readability (some lines go a few characters over. Just let me know if this is not negotiable and I will fix it.)

@Karl: did you get a chance to review Raymond's patch since last week? I think the work on this bug is almost done and it is blocking the patch for bug 411227.

Comment on attachment 682105 [details] [diff] [review] Mochitest for error logging Well done on working this out. Please find a way to avoid using UniversalXPConnect though, as we are trying to move our tests away from using that. I think you can use SpecialPowers.Cc["@mozilla.org/consoleservice;1"] .getService(SpecialPowers.Ci.nsIConsoleService); See for example dom/tests/mochitest/bugs/test_bug369306.html. If that doesn't work out then make the test a mochitest-chrome test. The boilerplate is a little different from chrome tests and they are added in the Makefile.in to MOCHITEST_CHROME_FILES instead of MOCHITEST_FILES. See for example widget/tests/Makefile.in. r+ with either of those approaches.

@karlt & fredw: Will do, right now school is bearing down on me; finals week approaches. I should be able to work on this more in about 2 weeks.

Conversion to SpecialPowers is complete. Changes: - Moved g_listener to window.g_listener - Added g_alreadyPassed variable to prevent test from running 99 times after conversion to SpecialPowers.

Comment on attachment 691918 [details] [diff] [review] Mochitest for error logging The use of SpecialPowers.wrap() is somewhat unconventional. See for example use of @mozilla.org/intl/stringbundle;1 in dom/browser-element/mochitest/browserElement_ReloadPostRequest.js Following conventions makes it easier for people to recognize patterns, and I think a little less code would be required too. I expect registerConsoleListener and postConsoleSentinel could make the nsIConsoleService usage simpler. I'm not sure, but I fear it is not quite right to call SimpleTest.finish() when further code in the test is expected to run. I wonder whether the next test might start while this one is completing. You can force page layout (and so presumably generation of the console messages) by doing something like the following in a trailing <script> block. var flush_layout = document.body.offsetHeight; postConsoleSentinel() could be called immediately after that. ok() and SimpleTest.finish() could be called from the listener when msg.message === "SENTINEL", which would allow the test to report failure without timing out.

- Removed dependency on timeouts. - Pulled the many global arrays into a JavaScript object. - Eliminated rollCall* methods. - doRollCall changed to only handle roll call. - Improved test feedback compared to original version: Now each error message counts as an individual check, so a tester can easily see exactly which error message failed to log.

Changes from comment #111: Removed g_alreadyPassed global variable. Sorry, I didn't notice it was still in there when I submitted it.

- Maintenance update: Resolved conflicts with changeset 116517 (f0e43f3770fb).

Comment on attachment 694224 [details] [diff] [review] Mochitest for error logging I like the changes you've made. Thanks!

Sent to try server: https://tbpl.mozilla.org/?tree=Try&rev=c7defd4e521e

@Raymond: can you please add a commit message to your patches? See https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

Added commit messages.

Added commit messages.

Added commit messages.

I also did a trivial update to bugfix-553917-cur.patch to resolve a conflict with its parent change set, no changes to the logic, the 'using' statements were clashing with the #include directives.

https://hg.mozilla.org/integration/mozilla-inbound/rev/169708ebc923 https://hg.mozilla.org/integration/mozilla-inbound/rev/206cc6939194 https://hg.mozilla.org/integration/mozilla-inbound/rev/6874adaa8fb0

https://hg.mozilla.org/mozilla-central/rev/169708ebc923 https://hg.mozilla.org/mozilla-central/rev/206cc6939194 https://hg.mozilla.org/mozilla-central/rev/6874adaa8fb0

This is now mentioned on https://developer.mozilla.org/en-US/docs/Firefox_20_for_developers As I see it, we don't need more notes on this. But, as always, feel free to add some details, if you want to.

Thanks Florian, that looks good to me.