[chromium] Fix accelerated 2D canvas in threaded compositing mode.
Created attachment 131633 [details] Patch
Comment on attachment 131633 [details] Patch Attachment 131633 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11947606
Created attachment 131644 [details] Patch
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..
(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.
(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."
(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.
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.
Created attachment 131894 [details] Patch
(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.
Comment on attachment 131894 [details] Patch R=me
Committed r110753: <http://trac.webkit.org/changeset/110753>