Bug 104835 - Web Inspector: Don't throw exceptions in WebInspector.Color
Summary: Web Inspector: Don't throw exceptions in WebInspector.Color
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-12 12:54 PST by johnjbarton
Modified: 2013-02-11 01:41 PST (History)
9 users (show)

See Also:


Attachments
Patch (3.95 KB, patch)
2012-12-12 13:01 PST, johnjbarton
no flags Details | Formatted Diff | Diff
Patch (8.42 KB, patch)
2012-12-17 12:33 PST, johnjbarton
no flags Details | Formatted Diff | Diff
Patch (8.48 KB, patch)
2012-12-20 10:51 PST, johnjbarton
no flags Details | Formatted Diff | Diff
Patch (8.65 KB, patch)
2013-01-14 10:23 PST, johnjbarton
no flags Details | Formatted Diff | Diff
Patch (9.52 KB, patch)
2013-01-16 10:55 PST, johnjbarton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description johnjbarton 2012-12-12 12:54:58 PST
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.
Comment 1 johnjbarton 2012-12-12 13:01:53 PST
Created attachment 179110 [details]
Patch
Comment 2 Pavel Feldman 2012-12-14 00:02:21 PST
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.
Comment 3 johnjbarton 2012-12-17 12:33:11 PST
Created attachment 179780 [details]
Patch
Comment 4 Yury Semikhatsky 2012-12-17 20:41:47 PST
Comment on attachment 179780 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +2012-12-12  John J. Barton  <johnjbarton@chromium.org>

Please remove obsolete change entry.
Comment 5 Yury Semikhatsky 2012-12-17 20:46:02 PST
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
Comment 6 johnjbarton 2012-12-18 09:22:00 PST
(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?
Comment 7 Yury Semikhatsky 2012-12-19 03:26:55 PST
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.
Comment 8 johnjbarton 2012-12-20 10:51:06 PST
Created attachment 180367 [details]
Patch
Comment 9 Alexander Pavlov (apavlov) 2013-01-14 06:00:38 PST
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)
Comment 10 johnjbarton 2013-01-14 10:23:54 PST
Created attachment 182597 [details]
Patch
Comment 11 Yury Semikhatsky 2013-01-15 00:50:16 PST
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.
Comment 12 johnjbarton 2013-01-15 09:22:24 PST
(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?
Comment 13 Yury Semikhatsky 2013-01-16 05:52:00 PST
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.
Comment 14 johnjbarton 2013-01-16 10:55:41 PST
Created attachment 183007 [details]
Patch
Comment 15 johnjbarton 2013-01-16 10:56:06 PST
Ok, now I get it, this version is better.
Comment 16 johnjbarton 2013-01-28 12:01:39 PST
review?
Comment 17 WebKit Review Bot 2013-02-11 01:41:10 PST
Comment on attachment 183007 [details]
Patch

Clearing flags on attachment: 183007

Committed r142440: <http://trac.webkit.org/changeset/142440>
Comment 18 WebKit Review Bot 2013-02-11 01:41:15 PST
All reviewed patches have been landed.  Closing bug.