Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2610963002:
Add documentation on cc/Blink nodes, ids, and indices. (Closed)
|
Created:
3 years, 11 months ago by wkorman Modified:
3 years, 11 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, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd documentation on cc/Blink nodes, ids, and indices.
BUG=674258
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/6f1b23711b3e28fd78646e3adfe0ca7986bd94b5
Cr-Commit-Position: refs/heads/master@{#441514}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Update documentation. #Patch Set 3 : Fix owner_id description. #
Messages
Total messages: 17 (9 generated)
Description was changed from ========== Document cc property tree ids. BUG=674258 ========== to ========== Document cc property tree ids. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Document cc property tree ids. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add documentation on cc/Blink nodes, ids, and indexes. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Add documentation on cc/Blink nodes, ids, and indexes. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add documentation on cc/Blink nodes, ids, and indices. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
wkorman@chromium.org changed reviewers: + ajuma@chromium.org, chrishtr@chromium.org
I thought helpful to capture notes on what required investigation as I work on adding new mappings from ElementId to cc 'node indexes'. Feedback welcome.
lgtm https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:611: // recent cc code now refers to these as 'node indexes', or 'property tree Could you land a patch that changes them to only have one naming scheme, rather than both "node indexes" and "property tree indexes"?
lgtm, thanks for writing this up! I just have a couple comments: https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:625: // of its current |id|. This could be represented by the layer's ElementId, "current |owner_id|" https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:629: // could then be the |stable_id| of its associated layer. The plan is that there won't be an associated layer, and the ElementId will come from something like the layout object responsible for creating the property tree node.
https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:611: // recent cc code now refers to these as 'node indexes', or 'property tree On 2017/01/04 at 20:54:28, chrishtr wrote: > Could you land a patch that changes them to only have one naming scheme, rather > than both "node indexes" and "property tree indexes"? Sure, I'll look at that in a separate change and can update this comment as part of that. https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:625: // of its current |id|. This could be represented by the layer's ElementId, On 2017/01/04 at 20:57:52, ajuma wrote: > "current |owner_id|" Rephrased this and next paragraph, let me know how it is now. https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:629: // could then be the |stable_id| of its associated layer. On 2017/01/04 at 20:57:52, ajuma wrote: > The plan is that there won't be an associated layer, and the ElementId will come from something like the layout object responsible for creating the property tree node. See above comment.
https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2610963002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:625: // of its current |id|. This could be represented by the layer's ElementId, On 2017/01/04 21:22:55, wkorman wrote: > On 2017/01/04 at 20:57:52, ajuma wrote: > > "current |owner_id|" > > Rephrased this and next paragraph, let me know how it is now. Looks great, thanks!
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, ajuma@chromium.org Link to the patchset: https://codereview.chromium.org/2610963002/#ps40001 (title: "Fix owner_id description.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483567390937620,
"parent_rev": "ddc699091484557aa6e4aefd1357cd6359ab2a4a", "commit_rev":
"5e0956347715de30c489f219e8b89033ec98625e"}
Message was sent while issue was closed.
Description was changed from ========== Add documentation on cc/Blink nodes, ids, and indices. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add documentation on cc/Blink nodes, ids, and indices. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2610963002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add documentation on cc/Blink nodes, ids, and indices. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2610963002 ========== to ========== Add documentation on cc/Blink nodes, ids, and indices. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/6f1b23711b3e28fd78646e3adfe0ca7986bd94b5 Cr-Commit-Position: refs/heads/master@{#441514} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6f1b23711b3e28fd78646e3adfe0ca7986bd94b5 Cr-Commit-Position: refs/heads/master@{#441514} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
