Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(676)

Issue 1447893002: compositor-worker: Introduce WebCompositorMutableState (Closed)

Created:
5 years, 1 month ago by Ian Vollick
Modified:
4 years, 11 months ago
Reviewers:
ajuma, esprehn, enne (OOO)
CC:
blink-reviews, blink-reviews-api_chromium.org, cc-bugs_chromium.org, chromium-reviews, dglazkov+blink
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

compositor-worker: Introduce WebCompositorMutableState This change introduces a wrapper around compositer-owned state that may be mutated by a compositor worker. This depends on crrev.com/1405993008 BUG=430155 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/9c7f6d0e46a46605801d143c04b86f5e4445cd37 Cr-Commit-Position: refs/heads/master@{#368273}

Patch Set 1 #

Total comments: 31

Patch Set 2 : add the unit test #

Patch Set 3 : address reviewer feedback #

Patch Set 4 : s/map/hash_map/ #

Patch Set 5 : s/map/hash_map/ (for real) #

Patch Set 6 : added class level comments #

Patch Set 7 : . #

Patch Set 8 : . #

Patch Set 9 : . #

Patch Set 10 : added yet more missing files. #

Patch Set 11 : attempt to make win bot happy. #

Patch Set 12 : try again. #

Total comments: 4

Patch Set 13 : . #

Total comments: 8

Patch Set 14 : . #

Patch Set 15 : review response. #

Total comments: 10

Patch Set 16 : Address review feedback. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+592 lines, -3 lines) Patch
A cc/animation/layer_tree_mutation.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +63 lines, -0 lines 0 comments Download
M cc/blink/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +5 lines, -0 lines 0 comments Download
M cc/blink/cc_blink.gyp View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M cc/blink/cc_blink_tests.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
A cc/blink/web_compositor_mutable_state_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +46 lines, -0 lines 0 comments Download
A cc/blink/web_compositor_mutable_state_impl.cc View 1 2 1 chunk +72 lines, -0 lines 0 comments Download
A cc/blink/web_compositor_mutable_state_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +172 lines, -0 lines 0 comments Download
A cc/blink/web_compositor_mutable_state_provider_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +44 lines, -0 lines 0 comments Download
A cc/blink/web_compositor_mutable_state_provider_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +34 lines, -0 lines 0 comments Download
M cc/layers/layer_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +9 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +12 lines, -3 lines 0 comments Download
M cc/trees/layer_tree_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +58 lines, -0 lines 0 comments Download
A third_party/WebKit/public/platform/WebCompositorMutableState.h View 1 2 3 4 5 11 1 chunk +34 lines, -0 lines 0 comments Download
A third_party/WebKit/public/platform/WebCompositorMutableStateProvider.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +28 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebPassOwnPtr.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +9 lines, -0 lines 0 comments Download
M ui/gfx/transform.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 29 (8 generated)
Ian Vollick
This is the follow-on patch to the initial element id plumbing: https://codereview.chromium.org/1405993008/
5 years, 1 month ago (2015-11-15 03:34:27 UTC) #3
enne (OOO)
This looks mostly like wrapping/plumbing without anything real being used? Do you think any unit ...
5 years, 1 month ago (2015-11-16 19:12:33 UTC) #4
danakj
https://codereview.chromium.org/1447893002/diff/1/third_party/WebKit/public/platform/WebMutableState.h File third_party/WebKit/public/platform/WebMutableState.h (right): https://codereview.chromium.org/1447893002/diff/1/third_party/WebKit/public/platform/WebMutableState.h#newcode21 third_party/WebKit/public/platform/WebMutableState.h:21: virtual WebMutableState* getMutableStateFor(uint64_t elementId) = 0; On 2015/11/16 19:12:33, ...
5 years, 1 month ago (2015-11-16 19:31:18 UTC) #5
esprehn
https://codereview.chromium.org/1447893002/diff/1/cc/animation/layer_tree_mutation.h File cc/animation/layer_tree_mutation.h (right): https://codereview.chromium.org/1447893002/diff/1/cc/animation/layer_tree_mutation.h#newcode60 cc/animation/layer_tree_mutation.h:60: typedef std::map<uint64_t, LayerTreeMutation> LayerTreeMutationMap; why not hash map? Is ...
5 years, 1 month ago (2015-11-16 20:01:04 UTC) #6
Ian Vollick
Thanks for the reviews! I've hopefully addressed your comments. (btw, if you're tempted to do ...
5 years, 1 month ago (2015-11-18 17:20:34 UTC) #7
Ian Vollick
On 2015/11/18 17:20:34, vollick wrote: > Thanks for the reviews! I've hopefully addressed your comments. ...
5 years ago (2015-12-09 18:19:26 UTC) #8
Ian Vollick
On 2015/12/09 18:19:26, vollick wrote: > On 2015/11/18 17:20:34, vollick wrote: > > Thanks for ...
5 years ago (2015-12-12 04:28:10 UTC) #9
ajuma
https://codereview.chromium.org/1447893002/diff/220001/cc/blink/web_compositor_mutable_state_impl.cc File cc/blink/web_compositor_mutable_state_impl.cc (right): https://codereview.chromium.org/1447893002/diff/220001/cc/blink/web_compositor_mutable_state_impl.cc#newcode42 cc/blink/web_compositor_mutable_state_impl.cc:42: main_layer_->OnTransformAnimated(gfx::Transform(matrix)); Are property tree nodes currently created for mutable ...
5 years ago (2015-12-14 19:41:47 UTC) #11
Ian Vollick
https://codereview.chromium.org/1447893002/diff/220001/cc/blink/web_compositor_mutable_state_impl.cc File cc/blink/web_compositor_mutable_state_impl.cc (right): https://codereview.chromium.org/1447893002/diff/220001/cc/blink/web_compositor_mutable_state_impl.cc#newcode42 cc/blink/web_compositor_mutable_state_impl.cc:42: main_layer_->OnTransformAnimated(gfx::Transform(matrix)); On 2015/12/14 19:41:47, ajuma wrote: > Are property ...
5 years ago (2015-12-14 20:22:07 UTC) #12
ajuma
On 2015/12/14 20:22:07, vollick wrote: > https://codereview.chromium.org/1447893002/diff/220001/cc/blink/web_compositor_mutable_state_impl.cc > File cc/blink/web_compositor_mutable_state_impl.cc (right): > > https://codereview.chromium.org/1447893002/diff/220001/cc/blink/web_compositor_mutable_state_impl.cc#newcode42 > ...
5 years ago (2015-12-14 20:24:08 UTC) #13
esprehn
There's no tests inside blink, and no usage of the API, this patch should contain ...
5 years ago (2015-12-16 04:01:59 UTC) #14
Ian Vollick
> There's no tests inside blink, and no usage of the API, this patch should ...
4 years, 12 months ago (2015-12-23 17:34:40 UTC) #15
Ian Vollick
On 2015/12/23 17:34:40, vollick wrote: > > There's no tests inside blink, and no usage ...
4 years, 11 months ago (2016-01-05 15:38:16 UTC) #16
esprehn
that release() method doesn't seem needed just for tests, why not just use a WebOwnPtr? ...
4 years, 11 months ago (2016-01-06 19:48:38 UTC) #17
Ian Vollick
https://codereview.chromium.org/1447893002/diff/280001/cc/animation/layer_tree_mutation.h File cc/animation/layer_tree_mutation.h (right): https://codereview.chromium.org/1447893002/diff/280001/cc/animation/layer_tree_mutation.h#newcode14 cc/animation/layer_tree_mutation.h:14: struct LayerTreeMutation { On 2016/01/06 19:48:37, esprehn wrote: > ...
4 years, 11 months ago (2016-01-08 03:01:54 UTC) #19
esprehn
Lgtm, if people end up creating a lot of proxies we should consider storing scoped_ptr ...
4 years, 11 months ago (2016-01-08 03:36:26 UTC) #20
Ian Vollick
On 2016/01/08 03:36:26, esprehn wrote: > Lgtm, if people end up creating a lot of ...
4 years, 11 months ago (2016-01-08 03:44:59 UTC) #21
Ian Vollick
On 2016/01/08 03:36:26, esprehn wrote: > Lgtm, if people end up creating a lot of ...
4 years, 11 months ago (2016-01-08 03:45:02 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1447893002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1447893002/300001
4 years, 11 months ago (2016-01-08 03:45:38 UTC) #25
commit-bot: I haz the power
Committed patchset #16 (id:300001)
4 years, 11 months ago (2016-01-08 04:36:45 UTC) #27
commit-bot: I haz the power
4 years, 11 months ago (2016-01-08 04:37:52 UTC) #29
Message was sent while issue was closed.
Patchset 16 (id:??) landed as
https://crrev.com/9c7f6d0e46a46605801d143c04b86f5e4445cd37
Cr-Commit-Position: refs/heads/master@{#368273}

Powered by Google App Engine
This is Rietveld 408576698