Also mention what the value was when reporting CSS parsing errors
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Tracking
()
People
(Reporter: pbro, Assigned: nchevobbe)
References
(Blocks 1 open bug)
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?
Comment 1•4 years ago
|
||
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.
Updated•4 years ago
|
Assignee | ||
Comment 3•4 years ago
|
||
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•4 years ago
|
||
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 :)
Updated•4 years ago
|
Updated•4 months ago
|
Description
•