Do not report some CSS errors for code valid in other browsers.

Do not report some CSS errors for code valid in other browsers.

R=pfeldman, vsevik
BUG=252105
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152886

[pfeldman, 2013-06-21 12:20:45]

I think we should filter properties on the front-end level. I'd recommend
emitting CSS.cssError structured events and generating console messages on the
front-end side. for now, we can block all arguable warnings in order to not
create noise.

https://codereview.chromium.org/17176013/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/17176013/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorCSSAgent.cpp:622: static bool
hasNonWebkitVendorSpecificPrefix(const CSSParserString& string, unsigned offset
= 0)
Do not use default values.

https://codereview.chromium.org/17176013/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorCSSAgent.cpp:635: static bool
isIgnoredPropertyName(const CSSParserString& content)
isValidPropertyName

[pfeldman, 2013-06-21 13:05:31]

lgtm

[commit-bot: I haz the power, 2013-06-21 13:11:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/17176013/4001

[commit-bot: I haz the power, 2013-06-21 14:59:05]

Change committed as 152886

[PhistucK, 2013-06-21 21:13:09]

https://codereview.chromium.org/17176013/diff/4001/LayoutTests/inspector/cons...
File LayoutTests/inspector/console/console-css-warnings.html (right):

https://codereview.chromium.org/17176013/diff/4001/LayoutTests/inspector/cons...
LayoutTests/inspector/console/console-css-warnings.html:78: background:        
linear-gradient(top,rgba(0,0,0,0) 0,rgba(0,0,0,1) 100%)    ;
This test is problematic. While linear-gradient is valid, the value for which
you test here is invalid (it should read "to top" instead of "top").
While the Developer Tools is not meant to be a general CSS validator, I think it
would be good to warn this broken value (even if only to promote standards
compliancy and help avoid future useless breakage). Is there any way to do it?
basically, maybe add another case for (regular expression)
linear-gradient[\s\t\n\r]*\([\s\t\n\r]*(top|bottom|left|right).

[apavlov, 2013-06-22 12:37:42]

On 2013/06/21 21:13:09, PhistucK wrote:
>
https://codereview.chromium.org/17176013/diff/4001/LayoutTests/inspector/cons...
> File LayoutTests/inspector/console/console-css-warnings.html (right):
> 
>
https://codereview.chromium.org/17176013/diff/4001/LayoutTests/inspector/cons...
> LayoutTests/inspector/console/console-css-warnings.html:78: background:       

> linear-gradient(top,rgba(0,0,0,0) 0,rgba(0,0,0,1) 100%)    ;
> This test is problematic. While linear-gradient is valid, the value for which
> you test here is invalid (it should read "to top" instead of "top").
> While the Developer Tools is not meant to be a general CSS validator, I think
it
> would be good to warn this broken value (even if only to promote standards
> compliancy and help avoid future useless breakage). Is there any way to do it?
> basically, maybe add another case for (regular expression)
> linear-gradient[\s\t\n\r]*\([\s\t\n\r]*(top|bottom|left|right).

Well, DevTools are not a home-made implementation of http://jigsaw.w3.org/.
We've just learned to report CSS parsing errors _from the Blink standpoint_.
They, however, are not necessarily errors on all other engines. So we try to
filter out errors that _we think_ may not be actual errors on Firefox, IE, etc.

So, if we see "linear-gradient", which we don't understand, we can either report
it as an error or say, "OK, we don't understand it, but we know others
understand property values that start with this string, so we won't report it."
Our goal in this case is not to run a specification-precise check of the value
but just to avoid as many false-positives as possible, based on the knowledge
that web developers write cross-browser code.

[PhistucK, 2013-06-22 12:43:03]

On 2013/06/22 12:37:42, apavlov wrote:
> On 2013/06/21 21:13:09, PhistucK wrote:
> >
>
https://codereview.chromium.org/17176013/diff/4001/LayoutTests/inspector/cons...
> > File LayoutTests/inspector/console/console-css-warnings.html (right):
> > 
> >
>
https://codereview.chromium.org/17176013/diff/4001/LayoutTests/inspector/cons...
> > LayoutTests/inspector/console/console-css-warnings.html:78: background:     
 
> 
> > linear-gradient(top,rgba(0,0,0,0) 0,rgba(0,0,0,1) 100%)    ;
> > This test is problematic. While linear-gradient is valid, the value for
which
> > you test here is invalid (it should read "to top" instead of "top").
> > While the Developer Tools is not meant to be a general CSS validator, I
think
> it
> > would be good to warn this broken value (even if only to promote standards
> > compliancy and help avoid future useless breakage). Is there any way to do
it?
> > basically, maybe add another case for (regular expression)
> > linear-gradient[\s\t\n\r]*\([\s\t\n\r]*(top|bottom|left|right).
> 
> Well, DevTools are not a home-made implementation of http://jigsaw.w3.org/.
> We've just learned to report CSS parsing errors _from the Blink standpoint_.
> They, however, are not necessarily errors on all other engines. So we try to
> filter out errors that _we think_ may not be actual errors on Firefox, IE,
etc.
> 
> So, if we see "linear-gradient", which we don't understand, we can either
report
> it as an error or say, "OK, we don't understand it, but we know others
> understand property values that start with this string, so we won't report
it."
> Our goal in this case is not to run a specification-precise check of the value
> but just to avoid as many false-positives as possible, based on the knowledge
> that web developers write cross-browser code.

In the specific case of linear-gradient, which is a candidate recommendation, no
browser understands the (non prefixed) "linear-gradient(top"... value, however,
many tools already emitted this wrong value (they were doing it right at the
time, before the syntax changed), which is why I think we should special case
it.

Thank you for your time!