Web Inspector: Don't throw exceptions in WebInspector.Color

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[johnjbarton]

If the Color(value) constructor fails, it throws. For example it throws on "none", from border properties. The throw is not really an exception, its simply a result of passing a shortcut property value to the Color constructor.

[johnjbarton]

Created attachment 179110 [details]
Patch

[Pavel Feldman]

Comment on attachment 179110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179110&action=review

Looks good. Small style nit and annotation.

> Source/WebCore/inspector/front-end/Color.js:36
> +    this.valid = this._parse();

I'd rather introduce wrapper WebInspector.Color.parse(str) that would return nullable Color. You can still leave _parse in place and make "valid" private then. You'll simply return null when !color._valid.

[johnjbarton]

Created attachment 179780 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 179780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179780&action=review

> Source/WebCore/ChangeLog:25
> [phone]  John J. Barton  <[e-mail]>

Please remove obsolete change entry.

[Yury Semikhatsky]

Comment on attachment 179780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179780&action=review

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1744
> +                if (text !== 'none') {

No need to check explicitly for 'none' as Color.parse would return null for it.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1747
> +                if (!color) {

style: no braces around one-line blocks

[johnjbarton]

(In reply to comment #5)
> (From update of attachment 179780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179780&action=review
> 
> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1744
> > +                if (text !== 'none') {
> 
> No need to check explicitly for 'none' as Color.parse would return null for it.

Yes, however, unlike an erroneous user input, 'none' is a correct user input which is not a color. For the best user experience we should be giving correct feedback. To start on that path we should avoid sending Color.parse() values we know are not colors. However I cannot go further at this time because I don't know the complete list of correct user inputs that are not colors.

How about if I change it to a comment?

[Yury Semikhatsky]

Comment on attachment 179780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179780&action=review

>>> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1744
>>> +                if (text !== 'none') {
>> 
>> No need to check explicitly for 'none' as Color.parse would return null for it.
> 
> Yes, however, unlike an erroneous user input, 'none' is a correct user input which is not a color. For the best user experience we should be giving correct feedback. To start on that path we should avoid sending Color.parse() values we know are not colors. However I cannot go further at this time because I don't know the complete list of correct user inputs that are not colors.
> 
> How about if I change it to a comment?

A comment would probably be more clear at this point.

[johnjbarton]

Created attachment 180367 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 180367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180367&action=review

One test nit, otherwise the change looks good to me

> LayoutTests/inspector/styles/styles-invalid-color-values-expected.txt:75
> +FAIL: Error parsing color 'none'.

This should be rephrased, since the "FAIL:" prefix is reserved for test failures and should normally never occur in test expectations (and the right prefix to use here is either "SUCCESS:" or nothing at all, since none of the previous colors have this prefix)

[johnjbarton]

Created attachment 182597 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 182597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182597&action=review

> LayoutTests/inspector/styles/styles-invalid-color-values.html:39
> +            InspectorTest.addResult("invalid color '" + colorString + "' parses to null.");

I'd rather leave this function untouched and use another function (dumpInvalidColorRepresentations?) for invalid colors as they have inverted expectation compared to valid colors. With the current approach it is not clear if "invalid color 'rgba(255, 0, 0, 5)' parses to null." would be a wrong result.

[johnjbarton]

(In reply to comment #11)
> (From update of attachment 182597 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182597&action=review
> 
> > LayoutTests/inspector/styles/styles-invalid-color-values.html:39
> > +            InspectorTest.addResult("invalid color '" + colorString + "' parses to null.");
> 
> I'd rather leave this function untouched and use another function (dumpInvalidColorRepresentations?) for invalid colors as they have inverted expectation compared to valid colors. With the current approach it is not clear if "invalid color 'rgba(255, 0, 0, 5)' parses to null." would be a wrong result.

Would another wording help? There is no way for the code to determine that "invalid color 'rgba(255, 0, 0, 5)' parses to null." is a wrong result: this code tests the parser and only the parser can make this determination. A human has to understand the output and validate that such a result is incorrect. To me, such output is certainly puzzling and I would check it. Since the test expected result would list a valid output for that input, it seems unlikely to me that this error would occur.

Leaving the current function would be misleading the try/catch cannot be reached since the parser no longer throws (by design).  Better to have the test code more closely resemble the way the API is used right?

[Yury Semikhatsky]

Comment on attachment 182597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182597&action=review

>>> LayoutTests/inspector/styles/styles-invalid-color-values.html:39
>>> +            InspectorTest.addResult("invalid color '" + colorString + "' parses to null.");
>> 
>> I'd rather leave this function untouched and use another function (dumpInvalidColorRepresentations?) for invalid colors as they have inverted expectation compared to valid colors. With the current approach it is not clear if "invalid color 'rgba(255, 0, 0, 5)' parses to null." would be a wrong result.
> 
> Would another wording help? There is no way for the code to determine that "invalid color 'rgba(255, 0, 0, 5)' parses to null." is a wrong result: this code tests the parser and only the parser can make this determination. A human has to understand the output and validate that such a result is incorrect. To me, such output is certainly puzzling and I would check it. Since the test expected result would list a valid output for that input, it seems unlikely to me that this error would occur.
> 
> Leaving the current function would be misleading the try/catch cannot be reached since the parser no longer throws (by design).  Better to have the test code more closely resemble the way the API is used right?

My concern was that someone unfamiliar with the code might rebase the test with a wrong output as it would look fine. To avoid such situations it would make sense to produce either "SUCCESS: parsed invalid color to null" or "FAIL: parsed invalid color to not null" so that it is clear that the result is a failure.

You are right, there is not much sense in leaving try/catch block. My point was to split function testColors(next) into testValidColors and testInvalidColors that would dump results in different manners.

[johnjbarton]

Created attachment 183007 [details]
Patch

[johnjbarton]

Ok, now I get it, this version is better.

[johnjbarton]

review?

[WebKit Review Bot]

Comment on attachment 183007 [details]
Patch

Clearing flags on attachment: 183007

Committed r142440: <http://trac.webkit.org/changeset/142440>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.