RESOLVED FIXED 80998
[chromium] Fix accelerated 2D canvas in threaded compositing mode.
https://bugs.webkit.org/show_bug.cgi?id=80998
Summary [chromium] Fix accelerated 2D canvas in threaded compositing mode.
Stephen White
Reported 2012-03-13 09:02:06 PDT
[chromium] Fix accelerated 2D canvas in threaded compositing mode.
Attachments
Patch (4.66 KB, patch)
2012-03-13 09:17 PDT, Stephen White
no flags
Patch (5.25 KB, patch)
2012-03-13 09:57 PDT, Stephen White
no flags
Patch (10.11 KB, patch)
2012-03-14 11:47 PDT, Stephen White
jamesr: review+
Stephen White
Comment 1 2012-03-13 09:17:00 PDT
WebKit Review Bot
Comment 2 2012-03-13 09:23:42 PDT
Comment on attachment 131633 [details] Patch Attachment 131633 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11947606
Stephen White
Comment 3 2012-03-13 09:57:52 PDT
Nat Duca
Comment 4 2012-03-13 10:07:05 PDT
Comment on attachment 131644 [details] Patch I'd really like unit tests for these cases. I don't think its okay for us to be fixing non-visual bugs and calling them covered by unit tests, personally. TreeSynchronizerTests seems a good place for scrollbars. Canvas2DLayerChromium, I'm not sure how to unit test. Enne might have an idea..
Stephen White
Comment 5 2012-03-13 10:54:02 PDT
(In reply to comment #4) > (From update of attachment 131644 [details]) > I'd really like unit tests for these cases. I don't think its okay for us to be fixing non-visual bugs and calling them covered by unit tests, personally. Totally agreed, although I would like at least one layout test that runs with threaded compositing and canvas2D, just to make sure we're at least we don't regress on basic functionality. I was thinking of adding a flag for threaded compositing in Settings so that it could be set from LayoutTestController in JS, similar to what we do for other things (such as accelerated painting), then we could have a basic "draw a green square" test. What do you think? > TreeSynchronizerTests seems a good place for scrollbars. I'll look into that. > Canvas2DLayerChromium, I'm not sure how to unit test. Enne might have an idea.. It does seem to have its own unit test (which I just broke). I'll look into adding more cases.
Nat Duca
Comment 6 2012-03-13 11:15:36 PDT
(In reply to comment #5) > Totally agreed, although I would like at least one layout test that runs with threaded compositing and canvas2D, just to make sure we're at least we don't regress on basic functionality. Yeah totally fair. We have --threaded-compositing right now. That will eventually be automatically set on bots where we use threaded compositing --- so it wont give you blanket coverage on all platforms, but it will give you relatively good coverage. I'm a little worried about making this a test controller flag. We dont currently support setting the proxy type at runtime --- you might get away with it if you set it before accelerated compositing turns on, but then thats racey, right? > > Canvas2DLayerChromium, I'm not sure how to unit test. Enne might have an idea.. > > It does seem to have its own unit test (which I just broke). I'll look into adding more cases. Thanks! Sorry to be "that guy."
Adrienne Walker
Comment 7 2012-03-13 12:17:37 PDT
(In reply to comment #6) > (In reply to comment #5) > > Totally agreed, although I would like at least one layout test that runs with threaded compositing and canvas2D, just to make sure we're at least we don't regress on basic functionality. > Yeah totally fair. > > We have --threaded-compositing right now. That will eventually be automatically set on bots where we use threaded compositing --- so it wont give you blanket coverage on all platforms, but it will give you relatively good coverage. > > I'm a little worried about making this a test controller flag. We dont currently support setting the proxy type at runtime --- you might get away with it if you set it before accelerated compositing turns on, but then thats racey, right? Could we just assert or CRASH if you ever call this new test-only function "too late"? It seems useful to be able to have a threaded compositing smoke test.
James Robinson
Comment 8 2012-03-13 14:19:58 PDT
Comment on attachment 131644 [details] Patch Code changes look good. Agree that tests would be nice - we have unit tests for tree sync already that should cover updateScrollbarLayerPointersRecursive(), looks like we're just missing a step. I'd like to just run all layout tests we currently run through the compositing path through the threaded path.
Stephen White
Comment 9 2012-03-14 11:47:23 PDT
Stephen White
Comment 10 2012-03-14 11:59:06 PDT
(In reply to comment #9) > Created an attachment (id=131894) [details] > Patch Added/fixed unit tests. I'll leave the layout tests to the threaded compositor switchover, as James describes.
James Robinson
Comment 11 2012-03-14 12:04:23 PDT
Comment on attachment 131894 [details] Patch R=me
Stephen White
Comment 12 2012-03-14 14:29:04 PDT
Note You need to log in before you can comment on or make changes to this bug.