Bug 80998 - [chromium] Fix accelerated 2D canvas in threaded compositing mode.
Summary: [chromium] Fix accelerated 2D canvas in threaded compositing mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-13 09:02 PDT by Stephen White
Modified: 2012-03-14 14:29 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.66 KB, patch)
2012-03-13 09:17 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2012-03-13 09:57 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2012-03-14 11:47 PDT, Stephen White
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2012-03-13 09:02:06 PDT
[chromium] Fix accelerated 2D canvas in threaded compositing mode.
Comment 1 Stephen White 2012-03-13 09:17:00 PDT
Created attachment 131633 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Stephen White 2012-03-13 09:57:52 PDT
Created attachment 131644 [details]
Patch
Comment 4 Nat Duca 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..
Comment 5 Stephen White 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.
Comment 6 Nat Duca 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."
Comment 7 Adrienne Walker 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.
Comment 8 James Robinson 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.
Comment 9 Stephen White 2012-03-14 11:47:23 PDT
Created attachment 131894 [details]
Patch
Comment 10 Stephen White 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.
Comment 11 James Robinson 2012-03-14 12:04:23 PDT
Comment on attachment 131894 [details]
Patch

R=me
Comment 12 Stephen White 2012-03-14 14:29:04 PDT
Committed r110753: <http://trac.webkit.org/changeset/110753>