Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2052743002:
Add effect node support in PaintArtifactCompositor for layer list mode [3/4] (Closed)
|
Created:
4 years, 6 months ago by trchen Modified:
4 years, 5 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), Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@pac_layer_list_step_2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd effect node support in PaintArtifactCompositor for layer list mode [3/4]
This CL trims down the number of properties we assign to generated cc property
nodes to the absolute minimum that cc doesn't crash.
BUG=609937
Committed: https://crrev.com/5728c6c98599daf1bde6ea1c28737e8cec4471bf
Cr-Commit-Position: refs/heads/master@{#402957}
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase #
Total comments: 3
Patch Set 3 : add back dummy layers #
Dependent Patchsets: Messages
Total messages: 22 (6 generated)
trchen@chromium.org changed reviewers: + chrishtr@chromium.org, jbroman@chromium.org
Split out #3.
https://codereview.chromium.org/2052743002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2052743002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:328: constexpr int kRealRootNodeId = 0; static constexpr int, since this is not in an anonymous namespace (and below) https://codereview.chromium.org/2052743002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:334: // cc is hardcoded to use transform node #1 for device scale and transform. #1? It's not clear if you're using zero- or one-based indexing here. Suggest saying "transform node index 1" (or 0 if it's that), or something similar, to be clearer. https://codereview.chromium.org/2052743002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:340: // cc is hardcoded to use clip node #1 for viewport clip. And it's okay for this to use the implicit root, rather than the secondary root, for its transform node?
trchen, are patches 3 and 4 in this sequence ready for review?
Rebased onto jbroman's latest CL and added the following changes: * CC doesn't actually need transform nodes to have owner, so I removed the owner layer workaround from jbroman's transform nodes. * The target space of transform nodes should be the device space. (We haven't plugged device scale factor to transform node #1 yet though.) https://codereview.chromium.org/2052743002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2052743002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:328: constexpr int kRealRootNodeId = 0; On 2016/06/15 12:55:08, jbroman wrote: > static constexpr int, since this is not in an anonymous namespace (and below) Done. https://codereview.chromium.org/2052743002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:334: // cc is hardcoded to use transform node #1 for device scale and transform. On 2016/06/15 12:55:08, jbroman wrote: > #1? It's not clear if you're using zero- or one-based indexing here. Suggest > saying "transform node index 1" (or 0 if it's that), or something similar, to be > clearer. Done. https://codereview.chromium.org/2052743002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:340: // cc is hardcoded to use clip node #1 for viewport clip. On 2016/06/15 12:55:08, jbroman wrote: > And it's okay for this to use the implicit root, rather than the secondary root, > for its transform node? Yes, this matches what's done in cc::PropertyTreeBuilder. What happens is that the LayerTreeHost gets viewport size (or damage rect) in physical space, and the value will be assigned to the clip rect of clip node #1.
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jbroman@chromium.org changed reviewers: + ajuma@chromium.org
+ajuma as I'm not sure all of the implications of leaving these unset. https://codereview.chromium.org/2052743002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (left): https://codereview.chromium.org/2052743002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:440: m_rootLayer->AddChild(dummyLayer); Why delete these? There is code that expects to be able to look up the LayerImpl corresponding to the transform node's owner, even though SPv2 isn't yet far enough along to hit it. If there is no such layer, or it's not committed, that would fail. https://cs.chromium.org/search/?q=LayerById.*transform.*owner_id&sq=package:c... finds a few such call sites.
https://codereview.chromium.org/2052743002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (left): https://codereview.chromium.org/2052743002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:440: m_rootLayer->AddChild(dummyLayer); On 2016/06/29 15:25:34, jbroman wrote: > Why delete these? There is code that expects to be able to look up the LayerImpl > corresponding to the transform node's owner, even though SPv2 isn't yet far > enough along to hit it. If there is no such layer, or it's not committed, that > would fail. > > https://cs.chromium.org/search/?q=LayerById.*transform.*owner_id&sq=package:c... > finds a few such call sites. Yeah, it's safer to leave these in for now (until we remove the code that assumes the existence of owning layers).
Oh, and by the way, watch out for https://codereview.chromium.org/2087963003 which weiliangc@ has out for review right now, as it will change the initial set of property tree nodes slightly.
https://codereview.chromium.org/2052743002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (left): https://codereview.chromium.org/2052743002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:440: m_rootLayer->AddChild(dummyLayer); On 2016/06/29 15:33:22, ajuma wrote: > On 2016/06/29 15:25:34, jbroman wrote: > > Why delete these? There is code that expects to be able to look up the > LayerImpl > > corresponding to the transform node's owner, even though SPv2 isn't yet far > > enough along to hit it. If there is no such layer, or it's not committed, that > > would fail. > > > > > https://cs.chromium.org/search/?q=LayerById.*transform.*owner_id&sq=package:c... > > finds a few such call sites. > > Yeah, it's safer to leave these in for now (until we remove the code that > assumes the existence of owning layers). I feel that's going backward because we are supposed to go toward the direction of no owning layers. Anyway I'm convinced that we will need this workaround again when we work on clip, so alright let's keep this (reluctantly).
OK, LGTM. But we should keep an eye out, because there are a couple places that do check owner_id, target_id, etc. yet, and we might need to add them back as well.
On 2016/06/29 20:44:54, jbroman wrote:
> OK, LGTM.
>
> But we should keep an eye out, because there are a couple places that do check
> owner_id, target_id, etc. yet, and we might need to add them back as well.
Or fix those in cc... It is totally possible we can have two paint chunks belong
to the same transform space, while draw into different target space. e.g.
<div>A</div>
<div style="overflow:scroll">
<div style="opacity:0.5">
<div style="position:absolute;">B</div>
</div>
</div>
The CQ bit was checked by trchen@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 #3 (id:40001)
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 [3/4] This CL trims down the number of properties we assign to generated cc property nodes to the absolute minimum that cc doesn't crash. BUG=609937 ========== to ========== Add effect node support in PaintArtifactCompositor for layer list mode [3/4] This CL trims down the number of properties we assign to generated cc property nodes to the absolute minimum that cc doesn't crash. BUG=609937 Committed: https://crrev.com/5728c6c98599daf1bde6ea1c28737e8cec4471bf Cr-Commit-Position: refs/heads/master@{#402957} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5728c6c98599daf1bde6ea1c28737e8cec4471bf Cr-Commit-Position: refs/heads/master@{#402957} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
