WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Stephen White
Comment 1
2012-03-13 09:17:00 PDT
Created
attachment 131633
[details]
Patch
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
Created
attachment 131644
[details]
Patch
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
Created
attachment 131894
[details]
Patch
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
Committed
r110753
: <
http://trac.webkit.org/changeset/110753
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug