RESOLVED FIXED 39482
RGB colors should be clamped to the 0-255 range
https://bugs.webkit.org/show_bug.cgi?id=39482
Summary RGB colors should be clamped to the 0-255 range
Attachments
Proposed patch (9.83 KB, patch)
2010-05-21 04:15 PDT, Andreas Kling
no flags
Proposed patch + stylefix (9.80 KB, patch)
2010-05-21 04:34 PDT, Andreas Kling
no flags
Proposed patch v2 (13.68 KB, patch)
2010-06-03 05:58 PDT, Andreas Kling
no flags
Andreas Kling
Comment 1 2010-05-21 04:15:18 PDT
Created attachment 56695 [details] Proposed patch
WebKit Review Bot
Comment 2 2010-05-21 04:17:51 PDT
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
Comment 3 2010-05-21 04:34:48 PDT
Created attachment 56696 [details] Proposed patch + stylefix
Andreas Kling
Comment 4 2010-05-21 04:36:15 PDT
As for other browsers, Firefox currently implements this behavior. Opera does not.
Eric Seidel (no email)
Comment 5 2010-05-21 12:08:57 PDT
What is IE's behavior? Have you performance tested this in any way?
Darin Adler
Comment 6 2010-05-21 12:55:07 PDT
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
Comment 7 2010-06-03 05:58:36 PDT
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
Comment 8 2010-06-03 05:59:58 PDT
(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)
Comment 9 2010-06-12 20:56:44 PDT
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
Comment 10 2010-06-13 08:57:19 PDT
(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
Comment 11 2010-06-23 14:55:50 PDT
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
Comment 12 2010-06-24 03:33:06 PDT
(In reply to comment #5) > What is IE's behavior? I'm told IE9 implements this behavior.
Oliver Hunt
Comment 13 2010-07-02 12:36:04 PDT
Comment on attachment 57759 [details] Proposed patch v2 r=me
WebKit Commit Bot
Comment 14 2010-07-02 14:16:16 PDT
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 15 2010-07-02 14:32:21 PDT
Comment on attachment 57759 [details] Proposed patch v2 Flaky test.
Andreas Kling
Comment 16 2010-07-02 14:33:32 PDT
(In reply to comment #15) > Flaky test. That was lightning fast, thanks!
WebKit Commit Bot
Comment 17 2010-07-02 17:08:20 PDT
Comment on attachment 57759 [details] Proposed patch v2 Clearing flags on attachment: 57759 Committed r62417: <http://trac.webkit.org/changeset/62417>
WebKit Commit Bot
Comment 18 2010-07-02 17:08:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.