Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042403005:
Add effect node support in PaintArtifactCompositor for layer list mode [1/4] (Closed)
|
Created:
4 years, 6 months ago by trchen Modified:
4 years, 6 months ago CC:
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), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd effect node support in PaintArtifactCompositor for layer list mode [1/4]
This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually
it only creates the contents of the layer, while other layer properties will
be setup by the caller. This is because the legacy mode and layer list mode
use different API level to position layers.
BUG=609937
Committed: https://crrev.com/42bd157c539c940682856a55860fbaea6e02f9fe
Cr-Commit-Position: refs/heads/master@{#399881}
Patch Set 1 #
Total comments: 7
Patch Set 2 : rebase & add comments #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Add effect node support in PaintArtifactCompositor for layer list mode [1/3] This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually it only creates the contents of the layer, while other layer properties will be setup by the caller. This is because the legacy mode and layer list mode use different API level to position layers. BUG=609937 ========== to ========== Add effect node support in PaintArtifactCompositor for layer list mode [1/3] This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually it only creates the contents of the layer, while other layer properties will be setup by the caller. This is because the legacy mode and layer list mode use different API level to position layers. BUG=609937 ==========
trchen@chromium.org changed reviewers: + chrishtr@chromium.org, jbroman@chromium.org
Split out #1.
Description was changed from ========== Add effect node support in PaintArtifactCompositor for layer list mode [1/3] This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually it only creates the contents of the layer, while other layer properties will be setup by the caller. This is because the legacy mode and layer list mode use different API level to position layers. BUG=609937 ========== to ========== Add effect node support in PaintArtifactCompositor for layer list mode [1/4] This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually it only creates the contents of the layer, while other layer properties will be setup by the caller. This is because the legacy mode and layer list mode use different API level to position layers. BUG=609937 ==========
https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:357: if (useLayerLists) { You don't have to set the layer's transform any more if useLayerLists is true? Why? https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h (right): https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:57: scoped_refptr<cc::Layer> layerForPaintChunk(const PaintArtifact&, const PaintChunk&, gfx::Vector2dF& layerOffset); Please document the last parameter a little.
Thanks for splitting the CL. lgtm after addressing chrishtr's comment https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:357: if (useLayerLists) { On 2016/06/09 at 02:20:05, chrishtr wrote: > You don't have to set the layer's transform any more if useLayerLists is true? Why? Because the transform is ignored with useLayerLists; it's only used to build property trees (but we no longer do that with useLayerLists). https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:404: // If the paint chunk is a foreign layer, just return that layer. super-nit: blank line above block comment?
https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:357: if (useLayerLists) { On 2016/06/09 at 19:59:05, jbroman wrote: > On 2016/06/09 at 02:20:05, chrishtr wrote: > > You don't have to set the layer's transform any more if useLayerLists is true? Why? > > Because the transform is ignored with useLayerLists; it's only used to build property trees (but we no longer do that with useLayerLists). How about adding an assert if they are read w/that flag on then? And a comment that it's going away. Also, if what you said is true, what is the purpose of line 379 in the new patch?
https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:357: if (useLayerLists) { On 2016/06/09 at 22:17:50, chrishtr wrote: > On 2016/06/09 at 19:59:05, jbroman wrote: > > On 2016/06/09 at 02:20:05, chrishtr wrote: > > > You don't have to set the layer's transform any more if useLayerLists is true? Why? > > > > Because the transform is ignored with useLayerLists; it's only used to build property trees (but we no longer do that with useLayerLists). > > How about adding an assert if they are read w/that flag on then? And a comment that it's going away. I wouldn't object to such an assertion, though it might cause a fair amount of clutter in cc (there are a lot of such layer properties, that are mostly/entirely inputs to property tree building). > Also, if what you said is true, what is the purpose of line 379 in the new patch? That line is in the !useLayerLists branch.
Looks good modulo comment request. https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2042403005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:357: if (useLayerLists) { On 2016/06/10 at 19:06:33, jbroman wrote: > On 2016/06/09 at 22:17:50, chrishtr wrote: > > On 2016/06/09 at 19:59:05, jbroman wrote: > > > On 2016/06/09 at 02:20:05, chrishtr wrote: > > > > You don't have to set the layer's transform any more if useLayerLists is true? Why? > > > > > > Because the transform is ignored with useLayerLists; it's only used to build property trees (but we no longer do that with useLayerLists). > > > > How about adding an assert if they are read w/that flag on then? And a comment that it's going away. > > I wouldn't object to such an assertion, though it might cause a fair amount of clutter in cc (there are a lot of such layer properties, that are mostly/entirely inputs to property tree building). > > > Also, if what you said is true, what is the purpose of line 379 in the new patch? > > That line is in the !useLayerLists branch. Oh right, sorry.
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/2042403005/#ps20001 (title: "rebase & add comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042403005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042403005/20001
Message was sent while issue was closed.
Description was changed from ========== Add effect node support in PaintArtifactCompositor for layer list mode [1/4] This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually it only creates the contents of the layer, while other layer properties will be setup by the caller. This is because the legacy mode and layer list mode use different API level to position layers. BUG=609937 ========== to ========== Add effect node support in PaintArtifactCompositor for layer list mode [1/4] This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually it only creates the contents of the layer, while other layer properties will be setup by the caller. This is because the legacy mode and layer list mode use different API level to position layers. BUG=609937 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
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 [1/4] This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually it only creates the contents of the layer, while other layer properties will be setup by the caller. This is because the legacy mode and layer list mode use different API level to position layers. BUG=609937 ========== to ========== Add effect node support in PaintArtifactCompositor for layer list mode [1/4] This CL refactor PaintArtifactCompositor::layerForPaintChunk so conceptually it only creates the contents of the layer, while other layer properties will be setup by the caller. This is because the legacy mode and layer list mode use different API level to position layers. BUG=609937 Committed: https://crrev.com/42bd157c539c940682856a55860fbaea6e02f9fe Cr-Commit-Position: refs/heads/master@{#399881} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/42bd157c539c940682856a55860fbaea6e02f9fe Cr-Commit-Position: refs/heads/master@{#399881} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
