455839 - Small cleanup in error handling in nsCSSParser/nsCSSScanner

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Alfred Kayser]

Attachment: V1: Replace params with param

The ReportUnexpectedParams and ReportUnexpectedTokensParams accept both a list of parameters, but actually they always used with one parameter.
By changing them to accept just one parameter, the calling of these from nsCSSParser can be simplified resulting in less and cleaner code.

Comment 1

[Alfred Kayser]

Attachment: V2: Updated to current trunk

Simple reportparam cleanup/simplification.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 369091 [details] [diff] [review]
V2: Updated to current trunk

>-// @see REPORT_UNEXPECTED_EOF in nsCSSParser.cpp

You dropped this comment on the floor; please put it back next to the moved code.


Please also describe how you tested the patch.  With that description, I'd have granted review now (given that it was appropriate), but without it I'm going to wait for it until granting review.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 369091 [details] [diff] [review]
V2: Updated to current trunk

Please re-request review after describing how you tested the patch.

Comment 4

[Zack Weinberg (:zwol)]

Attachment: Alternative version (depends on bug 516091)

As long as I was messing with error reporting (see bug 516091) I thought I'd pick this one up and polish it a little.

Tested (like 516091) by running layout/style mochitests with the error console open and watching the diagnostics go by.  I also spot-checked REPORT_UNEXPECTED_TOKEN_P with <style>foo { color: hsl(1,2%,3% blah) }</style> since I didn't see that one go by.

Comment 5

[Alfred Kayser]

Even better than my attempts!

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I liked Alfred's original patch, but I think I prefer leaving "Report" in the method names and leaving the aMessage being a const char* rather than const PRUnichar* (the string conversion performance should be trivial compared to the rest of the cost of that method, so we may as well bloat the binary less and store narrow strings).  Are there other changes thrown in?

Comment 7

[Alfred Kayser]

Attachment: V3: My patch updated to trunk

Retaking. The original patch was best (IMHO).
I have included the idea from Zack to use overloading for the parameter variants.
I have also removed the EOF(char) variant, as it was used in one location only.
No other changes than that.

Tested with Mochitest (with error console), with my themes and gmail pages (especially the last generate a lot of CSS errors...)

Comment 8

[Alfred Kayser]

Note, the latest patch doesn't depend on bug 516091.

Comment 9

[Zack Weinberg (:zwol)]

Attachment: V4: My patch, addressing dbaron's comments

Re-claiming this bug again.

(In reply to comment #6)
> I liked Alfred's original patch, but I think I prefer leaving "Report" in
> the method names and leaving the aMessage being a const char* rather than
> const PRUnichar* (the string conversion performance should be trivial
> compared to the rest of the cost of that method, so we may as well bloat
> the binary less and store narrow strings).

Ok, I've put "Report" back in the method names and changed the stringbundle tag arguments back to const char*.  I have, however, kept the use of function overloads rather than a bunch of ReportUnexpectedThing variants (EOF being the only surviving one of those).

> Are there other changes thrown in?

A small number of nsSubstring -> nsAString conversions (the former is a typedef for the latter, and is obsolete according to nsStringFwd.h); AddToError is no longer an exposed method; (nsCSS)Token::IsSymbol() and ::AppendToString() are now const methods.  That last allows me to declare that all the ReportUnexpected* methods that take a token argument do not modify it.

(In reply to comment #7)
> Retaking. The original patch was best (IMHO).

I must insist that

+    REPORT_UNEXPECTED_P(PEValueParsingError, propertyName);

is aesthetically superior to

+    REPORT_UNEXPECTED_P(PEValueParsingError, propertyName.get());

> I have also removed the EOF(char) variant, as it was used in one location only.

I kept it; I really don't like having this kind of gunk in the parser

+      const PRUnichar lookingForStr[] = {
+        PRUnichar('\''), aSymbol, PRUnichar('\''), PRUnichar(0)
+      };

Also, this way all uses of PEUnexpEOF2 are in nsCSSErrorReporter.cpp.

Comment 10

[Zack Weinberg (:zwol)]

Back to depending on bug 516091.  (This is #8 in a stack that I'm sort of hoping I get to land all at once, now. :-)

Comment 11

[Zack Weinberg (:zwol)]

Random question for dbaron or whoever: Is it necessary to use nsXPIDLString rather than nsString (or possibly even nsAString) for the return from nsIStringBundle::(Get|Format)StringFromName?  How about nsIURI::GetSpec?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Used to be.  Now you can use nsString.  Maybe even nsSubstring; not sure about nsAString; I get lost in the defines.

Comment 13

[Alfred Kayser]

V4 is OK by me.

Comment 14

[Zack Weinberg (:zwol)]

Comment on attachment 423475 [details] [diff] [review]
V4: My patch, addressing dbaron's comments

I'm not sure anymore at what point this change ought to happen, so clearing review for now.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: V5

New updated version of this patch, can hopefully go in along with 543151 et seq.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: v6

Dusting this off now we're finally in a position to land bug 663291 and therefore to test CSS error reporting in detail.

No substantive changes from the previous version.  Textual dependency on bug 516091.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 661642 [details] [diff] [review]
v6

r=dbaron

Comment 18

[Zack Weinberg (:zwol)]

Attachment: v6a

This required defuzzing for the @page changes (bug 115199).

Comment 19

[Zack Weinberg (:zwol)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f01d41ab835

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f01d41ab835