Also mention what the value was when reporting CSS parsing errors

NEW
Assigned to

Status

()

enhancement
P3
normal
5 months ago
24 days ago

People

(Reporter: pbro, Assigned: nchevobbe)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

In bug 1528426, we're trying to make CSS errors more actionable for our users. In particular for the WebCompat team to diagnose problems quicker.

One type of CSS errors happens when values can't be parsed.
These errors read Error in parsing value for ‘word-wrap’. Declaration dropped

This is a feature request to change the wording to also mention the value that couldn't be parsed. The errors would therefore read:

Error in parsing value ‘break-all’ for ‘word-wrap’. Declaration dropped.

Is this doable? Are there cases where this might actually not be wanted?

Perhaps a bit more readable:
‘break-all’ is not a valid value for ‘word-wrap’. Declaration dropped.

Are there cases where this might actually not be wanted?

Maybe we should only include the value if its length is small?
I wouldn't be surprised if there are errors like
‘word-wrap: <10k random chars here>’ out there...

Also, some properties have values that are naturally rather long,
e.g. ‘grid-template-areas’. Including such values would probably
make the error message less readable.

Priority: -- → P3
Whiteboard: [layout:triage-discuss]

I'll give this a try :)

Assignee: nobody → nchevobbe

Okay, so let check if I get the "what needs to be done" part right.

The error string, PEValueParsingError, is defined in dom/locales/en-US/chrome/layout/css.properties.
We'd like to change it to something like:

PEValueParsingError=‘%1$S’ is not a valid value for ‘%2$S’.

The string gets picked up in servo/ports/geckolib/error_reporter.rs by to_gecko_message.

ErrorReporter's report function is then calling Gecko_ReportUnexpectedCSSError with the string name, and 1 parameter (the property currently).

In Gecko_ReportUnexpectedCSSError, we then check that we have the param, and call ReportUnexpectedUnescaped which will put the parameter in the error string:

  if (param) {
    nsDependentCSubstring paramValue(param, paramLen);
    nsAutoString wideParam = NS_ConvertUTF8toUTF16(paramValue);
    reporter.ReportUnexpectedUnescaped(message, wideParam);
  } else {
    reporter.ReportUnexpected(message);
  }

Now, we want to actually pass 2 params, the property and its value.
So I'd change Gecko_ReportUnexpectedCSSError's signature to accept an array of strings (params), instead of a single one (param).

Then I guess it implies that we need to transform paramLen into an array as well?

In ErrorReporter, we need to pass such array to Gecko_ReportUnexpectedCSSError.
I think we need to change:

struct ErrorParams<'a> {
    prefix_param: Option<ErrorString<'a>>,
    main_param: Option<ErrorString<'a>>,
}

so that we have an array of ErrorString instead of a single one (main_param -> main_params)?

And then handle that in all the places where we create ErrorParams objects.


Emilio, does this analysis looks correct to you?

Flags: needinfo?(emilio)

Sorry for the lag here. Yeah, that looks right.

Most generic way would be to turn main_param -> main_params, and use an SmallVec<[ErrorString<'a>; 2> (to avoid
unnecessarily allocating).

I think

You'd need to change Gecko_ReportUnexpectedCSSError to take something like:

  const char** mainParamsStrings,
  size_t* mainParamLengths,
  size_t paramCount,

You could go with something more fancy, like an ErrorParam struct shared by Rust and C++:

#[repr(C)]
pub struct ErrorParam {
    string: *const u8,
    length: usize,
}
  const StyleErrorParam* mainParams,
  size_t mainParamCount,

If you want to do that, it should be a matter of adding ErrorParam to this list of types, and this other list.

Might be overkill for a single use, though we may use it to also clean up prefixParam too... your call :)

Flags: needinfo?(emilio)
Whiteboard: [layout:triage-discuss]
Duplicate of this bug: 1569160
You need to log in before you can comment on or make changes to this bug.