Bug 39482 – RGB colors should be clamped to the 0-255 range

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

HTML5 spec:
http://www.whatwg.org/specs/web-apps/current-work/#colors

This change will make WebKit pass the following tests:
http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.rgb-clamp-3.html
http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.rgb-clamp-4.html
http://philip.html5.org/tests/canvas/suite/tests/2d.fillStyle.parse.rgb-clamp-5.html

[Andreas Kling]

Created attachment 56695 [details]
Proposed patch

[WebKit Review Bot]

Attachment 56695 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSParser.cpp:3671:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/css/CSSParser.cpp:3664:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Created attachment 56696 [details]
Proposed patch + stylefix

[Andreas Kling]

As for other browsers, Firefox currently implements this behavior. Opera does not.

[Eric Seidel (no email)]

What is IE's behavior?

Have you performance tested this in any way?

[Darin Adler]

Comment on attachment 56696 [details]
Proposed patch + stylefix

> +ctx.shadowColor = "rgb(0,0,256)";
> +shouldBe("ctx.shadowColor", "'#0000ff'");

Please write the test so you can see what is being tested in the test output. For example:

    shouldBe("ctx.shadowColor = 'rgb(0,0,256)'; ctx.shadowColor", "'#0000ff'");

[Andreas Kling]

Created attachment 57759 [details]
Proposed patch v2

LayoutTests updated to show what's being tested.
Also unskipped 3 of the imported canvas/philip tests that pass with this change.

[Andreas Kling]

(In reply to comment #5)
> Have you performance tested this in any way?

Not really. What kind of performance test did you have in mind?

[Eric Seidel (no email)]

I would suspect that color parsing would be hot code.  The PLT would be one way to test, but running it is basically impossible unless you work at Apple (or Google if you're running it in Chrome).

You could write a little js page which parsed various colors in a loop using the CSS DOM or by calling  style.backgroundColor =  or setAttribute("color", ...).  You'd have to use shark or sample to verify that your benchmark actually hit the color code.  If so, then that would easily prove or disprove that this did or did not change performance of color parsing. :)  Whether changing color parsing performance would change the PLT is another quesiton, but if you didn't change color parsing perf then you obviously didn't change the PLT. :)

We used a similar approach for testing the HTML5 parser.  See WebCore/benchmarks/parsing/

[Andreas Kling]

(In reply to comment #9)
> I would suspect that color parsing would be hot code.  The PLT would be one way to test, but running it is basically impossible unless you work at Apple (or Google if you're running it in Chrome).

Any chance I could get some help running it for this change then?

I'll hack up the JS page either way, once I clear my queue. Thanks :)

[Andreas Kling]

Finally got around to this, sorry about the delay.

This patch incurs a slight performance hit, circa 0.72% slowdown for parsing of typical rgba() color strings.

I'm not sure this would have a measurable effect on your PLT timings.

[Andreas Kling]

(In reply to comment #5)
> What is IE's behavior?

I'm told IE9 implements this behavior.

[Oliver Hunt]

Comment on attachment 57759 [details]
Proposed patch v2

r=me

[WebKit Commit Bot]

Comment on attachment 57759 [details]
Proposed patch v2

Rejecting patch 57759 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
ing Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19281 test cases.
fast/loader/recursive-before-unload-crash.html -> failed

Exiting early after 1 failures. 14193 tests run.
211.67s total testing time

14192 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3355219

[Eric Seidel (no email)]

Comment on attachment 57759 [details]
Proposed patch v2

Flaky test.

[Andreas Kling]

(In reply to comment #15)
> Flaky test.

That was lightning fast, thanks!

[WebKit Commit Bot]

Comment on attachment 57759 [details]
Proposed patch v2

Clearing flags on attachment: 57759

Committed r62417: <http://trac.webkit.org/changeset/62417>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.