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

Issue 1599673002: compositor-worker: Remove code from cc_blink (Closed)

Created:
4 years, 11 months ago by Ian Vollick
Modified:
4 years, 11 months ago
Reviewers:
danakj, esprehn, jbroman
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-css, blink-reviews-dom_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-platform-graphics_chromium.org, blink-reviews-style_chromium.org, Rik, cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, f(malita), jbroman, jchaffraix+rendering, Justin Novosad, kinuko+watch, leviw+renderwatch, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, sadrul, Stephen Chennney, sof, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, vmpstr+blinkwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

compositor-worker: Remove code from cc_blink In this CL, the compositor worker code in cc_blink has been removed, mostly by moving it into Source/platform/graphics, a more natural home for compositor-related stuff. An additional benefit is that much of the code in public/platform/ is unnecessary now. It was there only so that it could be implemented by classes in cc_blink. There was a wrinkle when moving the test: the cc test machinery expected a base::MessageLoop to have been constructed to work. This violated the current DEPS rules for platform/graphics. I did try to remove this dependency, but the code is quite coupled and the refactor was not only large, it resulted in more complex code in cc. Since platform is in general allowed to talk to base, it seemed more reasonable to update the DEPS in platform/graphics/ to allow the use of message_loop.h in tests. BUG=430155 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/fd6e4f4a7a3f166de9d2fbc89770b4f2eb90272f Cr-Commit-Position: refs/heads/master@{#371006}

Patch Set 1 #

Patch Set 2 : Attempt to fix build issue by updating dependencies of blink_platform_unittests. #

Total comments: 18

Patch Set 3 : Address reviewer feedback. #

Total comments: 7

Patch Set 4 : Address reviewer feedback #

Patch Set 5 : Switch to HashMap. #

Total comments: 4

Patch Set 6 : . #

Patch Set 7 : no more duplicated enums! #

Patch Set 8 : fix blink_platform_unittests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+524 lines, -624 lines) Patch
D cc/animation/layer_tree_mutation.h View 1 chunk +0 lines, -63 lines 0 comments Download
M cc/animation/mutable_properties.h View 1 2 3 4 5 6 1 chunk +10 lines, -6 lines 0 comments Download
M cc/blink/BUILD.gn View 2 chunks +0 lines, -5 lines 0 comments Download
M cc/blink/cc_blink.gyp View 1 chunk +0 lines, -4 lines 0 comments Download
M cc/blink/cc_blink_tests.gyp View 1 chunk +0 lines, -1 line 0 comments Download
D cc/blink/web_compositor_mutable_state_impl.h View 1 chunk +0 lines, -46 lines 0 comments Download
D cc/blink/web_compositor_mutable_state_impl.cc View 1 chunk +0 lines, -72 lines 0 comments Download
D cc/blink/web_compositor_mutable_state_impl_unittest.cc View 1 chunk +0 lines, -172 lines 0 comments Download
D cc/blink/web_compositor_mutable_state_provider_impl.h View 1 chunk +0 lines, -44 lines 0 comments Download
D cc/blink/web_compositor_mutable_state_provider_impl.cc View 1 chunk +0 lines, -34 lines 0 comments Download
M cc/blink/web_layer_impl.cc View 1 2 3 4 5 3 chunks +0 lines, -31 lines 0 comments Download
M cc/layers/layer.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M cc/layers/layer_impl.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M cc/layers/layer_impl_unittest.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M cc/layers/layer_unittest.cc View 1 2 3 4 5 2 chunks +4 lines, -4 lines 0 comments Download
M cc/trees/layer_tree_impl.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/CompositorProxiedPropertySet.h View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/dom/CompositorProxiedPropertySet.cpp View 1 2 3 4 5 3 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/dom/CompositorProxy.cpp View 1 2 3 4 5 8 chunks +22 lines, -22 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.cpp View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp View 1 2 3 4 5 3 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/BUILD.gn View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/blink_platform.gypi View 1 2 3 4 5 3 chunks +6 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/blink_platform_tests.gyp View 1 1 chunk +4 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h View 1 2 3 4 5 6 1 chunk +16 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/CompositorMutableState.h View 1 2 1 chunk +46 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp View 1 2 3 1 chunk +78 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h View 1 chunk +37 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp View 1 2 3 4 1 chunk +43 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp View 1 2 3 4 5 6 7 1 chunk +159 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/CompositorMutation.h View 1 2 3 4 5 1 chunk +61 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/DEPS View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp View 1 2 3 4 5 2 chunks +7 lines, -7 lines 0 comments Download
D third_party/WebKit/public/platform/WebCompositorMutableProperties.h View 1 chunk +0 lines, -24 lines 0 comments Download
D third_party/WebKit/public/platform/WebCompositorMutableState.h View 1 chunk +0 lines, -34 lines 0 comments Download
D third_party/WebKit/public/platform/WebCompositorMutableStateProvider.h View 1 chunk +0 lines, -28 lines 0 comments Download

Messages

Total messages: 30 (14 generated)
Ian Vollick
4 years, 11 months ago (2016-01-18 18:11:49 UTC) #6
jbroman
just some quick drive-by comments https://codereview.chromium.org/1599673002/diff/20001/cc/animation/layer_tree_mutations.h File cc/animation/layer_tree_mutations.h (right): https://codereview.chromium.org/1599673002/diff/20001/cc/animation/layer_tree_mutations.h#newcode10 cc/animation/layer_tree_mutations.h:10: struct LayerTreeMutations { This ...
4 years, 11 months ago (2016-01-18 19:07:22 UTC) #8
Ian Vollick
Thanks for the detailed review. https://codereview.chromium.org/1599673002/diff/20001/cc/animation/layer_tree_mutations.h File cc/animation/layer_tree_mutations.h (right): https://codereview.chromium.org/1599673002/diff/20001/cc/animation/layer_tree_mutations.h#newcode10 cc/animation/layer_tree_mutations.h:10: struct LayerTreeMutations { On ...
4 years, 11 months ago (2016-01-18 21:25:04 UTC) #9
jbroman
lgtm but you should wait for esprehn's review https://codereview.chromium.org/1599673002/diff/40001/third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp File third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp (right): https://codereview.chromium.org/1599673002/diff/40001/third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp#newcode15 third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp:15: static_assert( ...
4 years, 11 months ago (2016-01-18 21:40:26 UTC) #10
Ian Vollick
https://codereview.chromium.org/1599673002/diff/40001/third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp File third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp (right): https://codereview.chromium.org/1599673002/diff/40001/third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp#newcode15 third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp:15: static_assert( On 2016/01/18 21:40:26, jbroman wrote: > nit: I'd ...
4 years, 11 months ago (2016-01-18 21:56:40 UTC) #11
Ian Vollick
https://codereview.chromium.org/1599673002/diff/40001/third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp File third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp (right): https://codereview.chromium.org/1599673002/diff/40001/third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp#newcode31 third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:31: return adoptPtr(new CompositorMutableState(&m_mutations->map.find(element_id)->value, layers.main, layers.scroll)); On 2016/01/18 21:56:40, vollick ...
4 years, 11 months ago (2016-01-19 14:49:30 UTC) #12
esprehn
lgtm https://codereview.chromium.org/1599673002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h File third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h (right): https://codereview.chromium.org/1599673002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h#newcode14 third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h:14: enum CompositorMutableProperty { Talk to Dana about how ...
4 years, 11 months ago (2016-01-21 20:05:25 UTC) #13
Ian Vollick
https://codereview.chromium.org/1599673002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h File third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h (right): https://codereview.chromium.org/1599673002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h#newcode14 third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h:14: enum CompositorMutableProperty { On 2016/01/21 20:05:25, esprehn wrote: > ...
4 years, 11 months ago (2016-01-21 20:08:37 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1599673002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1599673002/80001
4 years, 11 months ago (2016-01-21 20:10:24 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/120138) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, ...
4 years, 11 months ago (2016-01-21 20:15:37 UTC) #19
danakj
https://codereview.chromium.org/1599673002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h File third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h (right): https://codereview.chromium.org/1599673002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h#newcode14 third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h:14: enum CompositorMutableProperty { On 2016/01/21 20:08:37, vollick wrote: > ...
4 years, 11 months ago (2016-01-21 20:54:26 UTC) #21
Ian Vollick
On 2016/01/21 20:54:26, danakj wrote: > https://codereview.chromium.org/1599673002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h > File third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h > (right): > > https://codereview.chromium.org/1599673002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h#newcode14 ...
4 years, 11 months ago (2016-01-22 02:40:57 UTC) #22
danakj
Your approach LGTM :)
4 years, 11 months ago (2016-01-22 18:49:41 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1599673002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1599673002/140001
4 years, 11 months ago (2016-01-22 19:07:25 UTC) #26
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 11 months ago (2016-01-22 19:13:43 UTC) #28
commit-bot: I haz the power
4 years, 11 months ago (2016-01-22 19:15:32 UTC) #30
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/fd6e4f4a7a3f166de9d2fbc89770b4f2eb90272f
Cr-Commit-Position: refs/heads/master@{#371006}

Powered by Google App Engine
This is Rietveld 408576698