187007 - make css errors localizable

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

css errors should be localizable before they are turned on

Comment 1

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

See ReportToConsole in nsCSSLoader.cpp, which could be moved and used here.

My main concern is that the actual code to trigger the errors (the stuff that's
spread through nsCSSParser) be very easy to write.  I suspect some advanced
features of nsTextFormatter will be necessary (%$n or something).

If you really want to go ahead and do this, I'll warn you that I might be rather
picky about the names for the strings...

Comment 2

[David G King]

Well, it missed 1.4beta, so changing milestone.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

I'd rather not have people change the milestones of my bugs...

Comment 4

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

I'm probably going to do this soon, so taking bug.  Or did you make any progress
on it?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #4)
> I'm probably going to do this soon, so taking bug.  Or did you make any progress
> on it?

No, sorry. Feel free to take this.

Comment 6

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

Attachment: preliminary refactoring

This compiles, but is untested.

Comment 7

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

Comment on attachment 158839 [details] [diff] [review]
preliminary refactoring

The error messages affected here are those added for bug 154942, bug 223799,
and bug 221669.

Comment 8

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

Comment on attachment 158839 [details] [diff] [review]
preliminary refactoring

I've tested the testcases on all three bugs to make sure the errors still show
up.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158839 [details] [diff] [review]
preliminary refactoring

>Index: base/public/nsContentUtils.h

>+   *   @param aParamLength Length of aParams.

aParamsLength

>+                                  const nsCSubstring& aURI,

Any reason for this not to be an nsIURI?  All the callers seem to have
nsIURIs...

Other than that, r+sr=bzbarsky.

Comment 10

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

Comment on attachment 158839 [details] [diff] [review]
preliminary refactoring

Checked in to trunk (with comments addressed -- although ReportToConsole has to
null-check the URI because nsCSSLoader apparently needs to), 2004-09-14 10:26
-0700.

Comment 11

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

Attachment: work in progress

So this didn't actually benefit from attachment 158839 [details] [diff] [review], but anyway...

I've cross-checked the strings in 3 ways:
 * there are no duplicate string names
 * there are no duplicate string values
 * the list of strings used is the same as the list of string stored

I haven't actually compiled this yet.

Comment 12

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

Attachment: patch

This one compiles; not tested yet.

Comment 13

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

Attachment: testcase exercising all the macros (but not all the errors!)

Comment 14

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

Comment on attachment 159139 [details] [diff] [review]
patch

This seems to work fine.

Comment 15

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

I didn't mean to include the nsCSSScanner.h changes.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 159139 [details] [diff] [review]
patch

>Index: content/html/style/src/css.properties

>+PEUnexpEOF=Unexpected end of file while searching for %1$S.

>+PEUnknownProperty=Unknown property %1$S.

The property name used to be in single quotes.	Was this a purposeful change?

>+PEPropertyParsingError=Error in parsing value for property %1$S.

Same here.

>+PEUnknownAtRule=Unrecognized at-rule or error parsing at-rule %1$S.

The token string used to be in single quotes.

>+PEAtNSNoPrefix=namespace prefix in @namespace rule
>+PEAtNSNoURI=namespace URI in @namespace rule

Was there a reason for not using "EOF" in the names of these string names? 
They're passed to REPORT_UNEXPECTED_EOF, and you seem to use "EOF" at the ends
of names of things that get passed to that, for the most part.

>+PESkipDeclNoBraceFound=closing } of declaration block
>+PESkipRSNoBraceFound=closing } of invalid rule set

Same.

>+PESelectorGroupNoCombinator=Dangling combinator.

This would be better named "PESelectorGroupExtraCombinator" or something.

>+PEAttSelUnexpected=Unexpected token in attribute selector:  '%1$S'.

Why two spaces after the ':'?

>+PEAttSelCloseEOF=] to end attribute selector

Put single quotes around the ']'?

>+PEPseudoSelNewStyleOnly=This pseudo-element must use the \"::\" form:  '%1$S'.

I don't think you need to backslash the quotes in a properties file.  Also, why
two spaces after the ':'?

>+PEPseudoSelTrailing=Found trailing token after pseudo-element, which must be the last part of a selector:  '%1$S'.

Why two spaces after the ':'?

>+PELangArgNotIdent=Expected identifier for lang pseudo-class parameter '%1$S'.

This is missing a "but found" before the '%1$S'.

>Index: content/html/style/src/nsCSSParser.cpp

>+#define ENSURE_STRINGBUNDLE if (!InitStringBundle()) return;

I think I'd prefer a ENSURE_STRINGBUNDLE macro that requires a ';' after it to
compile... having it in the functions without the ';' is sorta weird.  I don't
feel very strongly about this, though, so if you do, go with the way you like.

>+static void ReportUnexpectedEOF(nsCSSScanner *sc, const char* aMessage)

"aSearchTarget" may be a better arg name here than "aMessage"... it's not great
either, though.  Basically, it would be good to indicate that the const char*
is supposed to say what we were looking for when the EOF happened.  It's not a
high priority, though, so if nothing comes to mind, just leave it as is.

>+// aMessage must take 1 parameter

add " -- the string representation of the unexpected token"

The rest looks good (minus the nsCSSScanner.h change, for now).

r+sr=bzbarsky with the nits.

Comment 17

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

(In reply to comment #16)
> >+PEPseudoSelTrailing=Found trailing token after pseudo-element, which must be
the last part of a selector:  '%1$S'.
> 
> Why two spaces after the ':'?

That wasn't a change, but I fixed it anyway.

Comment 18

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

Attachment: patch

Comment 19

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

Fix checked in to trunk, 2004-09-20 12:38 -0700.

Comment 20

[Alexander L. Slovesnik]

It looks like there is typo in this patch in
http://lxr.mozilla.org/mozilla/source/dom/locales/en-US/chrome/layout/css.properties#127:

PEBadDeclOrRuleEnd=Expected ';' or '}' to terminate declaration but found"))

I guess that it should be:

PEBadDeclOrRuleEnd=Expected ';' or '}' to terminate declaration but found '%1$S'.

Comment 21

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

Fixed.  Thanks for pointing that out.