Bug 80998 – [chromium] Fix accelerated 2D canvas in threaded compositing mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Stephen White]

[chromium] Fix accelerated 2D canvas in threaded compositing mode.

[Stephen White]

Created attachment 131633 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 131644 [details]
Patch

[Nat Duca]

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]

(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]

(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]

(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 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]

Created attachment 131894 [details]
Patch

[Stephen White]

(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 on attachment 131894 [details]
Patch

R=me

[Stephen White]

Committed r110753: <http://trac.webkit.org/changeset/110753>