1538103 - Also mention what the value was when reporting CSS parsing errors

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

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?

Comment 1

[Mats Palmgren (inactive)]

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.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

I'll give this a try :)

Comment 3

[Nicolas Chevobbe [: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?

Comment 4

[Emilio Cobos Álvarez (: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 :)