1211479 - Errors when parsing substituting variables should include the generated string to help diagnose the issue

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

See bug 1211221 for an example of this. Variables may have many different values, some of which may cause parsing issues while other may work fine. If a parsing issue is encountered, the error text may read:

> Error in parsing value for 'margin-bottom' after substituting variables. Falling back to 'initial'.

It would be an improvement to make this error text read:

> Error in parsing value for 'margin-bottom' after substituting variables. Falling back to 'initial'. Generated value was 'calc(-1 * 0)'.

This would help the developer find which variable value was the cause of the parsing issue quicker.

Comment 1

[Brian Grinstead [:bgrins]]

Looks like PEValueWithVariablesParsingError is the string here.  Maybe `expandedValue` can be added to the error message at this point: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#2693.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

I came to the same conclusion! I'm testing a patch now :)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Try push, https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd5f63a896b

I had compiler issues on my local build with the ErrorReporter.cpp changes, but maybe it will be green on try (suggesting I needed a clobber build to fix some MSVC bug).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8669822 [details] [diff] [review]
Patch

You need to change the string name when you change its value in a semantically-incompatible (as in, requires new localization work) way, so the l10n tools will pick it up correctly.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8669822 [details] [diff] [review]
Patch

I'm not really familiar with ErrorReporter.h/cpp, so I'll defer to a style-system peer on this (heycam, bz, or dbaron).  --> Canceling review.

But yes, bz's comment applies -- you need a new string name here in css.properties.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8669863 [details] [diff] [review]
Patch v1.1

> +PEValueWithVariablesParsingErrorInValue=Error in parsing value for '%1$S' after substituting variables. Generated value was '%2$S'.

Probably good to document that %1$S will be the property name.

r=me

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2b2277a2a6

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad2b2277a2a6