Better filtering CSS errors.

• Component: CSS
• Status: RESOLVED WONTFIX

[Ryosuke Niwa]

We should probably merge
https://chromium.googlesource.com/chromium/blink/+/c655fe1e72ca6b35e680b069410f320e51ca6aae

[Glenn Adams]

Created attachment 201760 [details]
Patch

[Benjamin Poulain]

This is odd layering. Joseph, Tim, what do you think?

[Glenn Adams]

(In reply to comment #2)
> This is odd layering. Joseph, Tim, what do you think?

I rather thought so myself, but I stayed (fairly) faithful to the merge set source. Please suggest changes as you prefer.

[Benjamin Poulain]

> I rather thought so myself, but I stayed (fairly) faithful to the merge set source. Please suggest changes as you prefer.

I would not take this one patch. I think this change makes the architecture worse.

Maybe the Inspectors gurus will disagree, lets see what they say.

[Joseph Pecoraro]

I thought our Inspector CSS parsing error support was incomplete. I think the Blink split when it was partially done, so we don't get any CSS parsing errors in the inspector right now. So I'm not sure how useful it would be to take the patch unless someone wants to fully merge reporting CSS errors.

As for the patch itself, I agree it seems a bit weird to have the "Inspector" filter errors in the CSS Parser like that.

[Benjamin Poulain]

Comment on attachment 201760 [details]
Patch

Looks like we should not take this one.

Still, thank you Glenn for looking for blink patches that could be good for WebKit.

[Glenn Adams]

(In reply to comment #6)
> (From update of attachment 201760 [details])
> Looks like we should not take this one.
> 
> Still, thank you Glenn for looking for blink patches that could be good for WebKit.

Actually, Ryosuke suggested this be merged, and I was just back filling. :)

As you can see from https://bugs.webkit.org/attachment.cgi?id=201760&action=review, there is already filtering occurring in InspectorConsoleAgent::addMessageToConsole in a kludgey fashion:

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:-191
> - // Ignore errors like "*property=value". This trick is used for IE7: http://stackoverflow.com/questions/4563651/what-does-an-asterisk-do-in-a-css-property-name
> - if (source == CSSMessageSource && message.contains(": *"))
> -     return;
> -

which seems equally out of place (as a layering violation). For whatever reason, it looks like the blink patch writer chose to reinforce that violation by keeping the filter in the inspector.

A better, non-layer-violating solution would simply be to place the filtering function in or around CSSParser::syntaxError, and not move CSSParser::Location to WebCore::CSSParserLocation.

How about I update the patch accordingly to take this better approach?

[Ryosuke Niwa]

I think we should simply ignore the inspector change since we don't enable that feature in WebKit anyway, and someone has to finish implementing the feature or remove it.

[Glenn Adams]

The blink merge set violates layers and appears only to handle a special IE7 compatibility case anyway. So let's not merge this one. I will instead file a new bug to remove the special IE7 code from InspectorConsoleAgent.