Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2052763002:
Add effect node support in PaintArtifactCompositor for layer list mode [4/4] (Closed)
|
Created:
4 years, 6 months ago by trchen Modified:
4 years, 5 months ago CC:
jbroman, ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@pac_layer_list_step_3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd effect node support in PaintArtifactCompositor for layer list mode [4/4]
This CL adds a local EffectStack class similar to ClipManager, that keep track
of effect state and assign generated cc effect node to layers. This is only
used for layer list mode.
BUG=609937
Committed: https://crrev.com/889b2a8057be02fefc2fdd9e5deb289844f94040
Cr-Commit-Position: refs/heads/master@{#404575}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : fix test & add test #Patch Set 5 : rebase #Patch Set 6 : rebase & meld into PropertyTreeManager #
Total comments: 11
Patch Set 7 : rebase & fix nits #
Depends on Patchset: Messages
Total messages: 37 (14 generated)
trchen@chromium.org changed reviewers: + chrishtr@chromium.org, jbroman@chromium.org
Split out #4.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Does this need to be updated, or is it ready for review? There are blink_platform_unittests failures on the trybots.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Oops I forgot to ping. Will upload another rebase.
On 2016/07/07 23:12:39, trchen wrote: > Oops I forgot to ping. Will upload another rebase. Looks like we prefer to consolidate state management into PropertyTreeManager class. I will do so when I rebase it. Note that effect tree will have slightly different public method from transform and clip because effects have strong ordering and hierarchy, i.e. an effect node can't be entered twice.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
CL uploaded
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:365: const EffectPaintPropertyNode* currentEffectNode() { return m_effectStack.last().effect; } super-nit: for consistency with compositorIdForCurrentEffectNode(), make this method const? https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:473: const EffectPaintPropertyNode* lowestCommonAncestor(const EffectPaintPropertyNode* nodeA, const EffectPaintPropertyNode* nodeB) super-nit: blank line above this function please https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:531: dummyClip.data.transform_id = kRealRootNodeId; It looks like they don't yet exist on EffectPaintPropertyNode, but I assume in the future these will point at the appropriate transform/clip node for the effect? https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:566: propertyTreeManager.switchToEffectNode(*paintChunk.properties.effect.get()); nit: is there a reason this has to happen above the other logic, or could this be grouped together with computing the ID for the transform and clip nodes (possibly made even more similar if switchToEffectNode returned the ID)? https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp (right): https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:38: static EffectPaintPropertyNode* node; nit: use DEFINE_STATIC_REF, which does exactly this
https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:365: const EffectPaintPropertyNode* currentEffectNode() { return m_effectStack.last().effect; } On 2016/07/08 18:32:34, jbroman wrote: > super-nit: for consistency with compositorIdForCurrentEffectNode(), make this > method const? Done. https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:473: const EffectPaintPropertyNode* lowestCommonAncestor(const EffectPaintPropertyNode* nodeA, const EffectPaintPropertyNode* nodeB) On 2016/07/08 18:32:34, jbroman wrote: > super-nit: blank line above this function please Done. https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:531: dummyClip.data.transform_id = kRealRootNodeId; On 2016/07/08 18:32:34, jbroman wrote: > It looks like they don't yet exist on EffectPaintPropertyNode, but I assume in > the future these will point at the appropriate transform/clip node for the > effect? I don't think the dummy clip should even exist. Most effects don't come with an intrinsic clip. The only exception I'm aware of is CSS filter. Likewise, most effects are spatial independent. The only exception is blur filter for which we need to specify the space of the blur radius. https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:566: propertyTreeManager.switchToEffectNode(*paintChunk.properties.effect.get()); On 2016/07/08 18:32:34, jbroman wrote: > nit: is there a reason this has to happen above the other logic, or could this > be grouped together with computing the ID for the transform and clip nodes > (possibly made even more similar if switchToEffectNode returned the ID)? Done. https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp (right): https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:38: static EffectPaintPropertyNode* node; On 2016/07/08 18:32:34, jbroman wrote: > nit: use DEFINE_STATIC_REF, which does exactly this Done.
The CQ bit was checked by trchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2052763002/#ps120001 (title: "rebase & fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2052763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:531: dummyClip.data.transform_id = kRealRootNodeId; On 2016/07/09 at 01:09:25, trchen wrote: > On 2016/07/08 18:32:34, jbroman wrote: > > It looks like they don't yet exist on EffectPaintPropertyNode, but I assume in > > the future these will point at the appropriate transform/clip node for the > > effect? > > I don't think the dummy clip should even exist. Most effects don't come with an intrinsic clip. The only exception I'm aware of is CSS filter. That's fair, but the effect will have an associated clip and transform. > Likewise, most effects are spatial independent. The only exception is blur filter for which we need to specify the space of the blur radius. Or a drop-shadow filter. Or a reference filter (<feOffset>, for instance). Or a reflection.
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add effect node support in PaintArtifactCompositor for layer list mode [4/4] This CL adds a local EffectStack class similar to ClipManager, that keep track of effect state and assign generated cc effect node to layers. This is only used for layer list mode. BUG=609937 ========== to ========== Add effect node support in PaintArtifactCompositor for layer list mode [4/4] This CL adds a local EffectStack class similar to ClipManager, that keep track of effect state and assign generated cc effect node to layers. This is only used for layer list mode. BUG=609937 Committed: https://crrev.com/889b2a8057be02fefc2fdd9e5deb289844f94040 Cr-Commit-Position: refs/heads/master@{#404575} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/889b2a8057be02fefc2fdd9e5deb289844f94040 Cr-Commit-Position: refs/heads/master@{#404575}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2138663002/ by yukishiino@chromium.org. The reason for reverting is: This CL seems making webkit_unit_tests' failures. https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.c... https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20(dbg)/... . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
