Closed Bug 1211479 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jaws, Assigned: jaws)

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
I came to the same conclusion! I'm testing a patch now :)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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).
Attachment #8669822 - Flags: review?(dholbert)
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 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.
Attachment #8669822 - Flags: review?(dholbert)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8669822 - Attachment is obsolete: true
Attachment #8669863 - Flags: review?(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
Attachment #8669863 - Flags: review?(bzbarsky) → review+
Attachment #8669863 - Attachment is obsolete: true
Attachment #8669936 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ad2b2277a2a6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.