Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2310323002:
Add comments describing the cc ScrollNode members (Closed)
DescriptionAdd comments describing the cc ScrollNode members
This patch adds some comments to describe the cc ScrollNode members.
BUG=644514
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/227d9305559686377e566989ae24b8656acad15e
Cr-Commit-Position: refs/heads/master@{#417071}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address reviewer comments #
Total comments: 3
Patch Set 3 : Rework owner_id comment a little more #Messages
Total messages: 17 (4 generated)
Description was changed from ========== Add comments describing the cc ScrollNode members This patch adds some comments to describe the cc ScrollNode members. BUG=644514 ========== to ========== Add comments describing the cc ScrollNode members This patch adds some comments to describe the cc ScrollNode members. BUG=644514 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
pdr@chromium.org changed reviewers: + ajuma@chromium.org, chrishtr@chromium.org, sunxd@chromium.org
Pulling this off https://codereview.chromium.org/2299533002. Happy to shed bikes on this one.
https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h File cc/trees/scroll_node.h (right): https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:32: int owner_id; Rename to owning_layer_id?
lgtm, thanks for adding these comments! https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h File cc/trees/scroll_node.h (right): https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:31: // The layer id that corresponds to the layer contents that are scrolled. Please also mention that, unlike "id" which can change each time we rebuild property trees, this id is stable. https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:32: int owner_id; On 2016/09/06 23:40:16, chrishtr wrote: > Rename to owning_layer_id? Since changing the scroll offset stored at this node can potentially move many layers, and since there's no hierarchical relationship between these layers in SPv2, or even now on the compositor thread, if we rename this I'd prefer that the name emphasizes that this id is stable rather than that it happens to match a layer id right now (we're working towards removing the assumption that property tree nodes have "owning" layers). Also whatever terminology we end up using here should be consistent across node types. https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:34: // This is used to delineate a subtree that should not be scrolled. For "should not be scrolled independently" maybe? https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:53: // This offset is used when |scrollable| is false and there isn't a transform "transform node"
PTAL https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h File cc/trees/scroll_node.h (right): https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:32: int owner_id; On 2016/09/07 at 14:19:23, ajuma wrote: > On 2016/09/06 23:40:16, chrishtr wrote: > > Rename to owning_layer_id? > > Since changing the scroll offset stored at this node can potentially move many layers, and since there's no hierarchical relationship between these layers in SPv2, or even now on the compositor thread, if we rename this I'd prefer that the name emphasizes that this id is stable rather than that it happens to match a layer id right now (we're working towards removing the assumption that property tree nodes have "owning" layers). Also whatever terminology we end up using here should be consistent across node types. It looks like the owner_id name is used for many other trees too. I think it could make sense to rename them all to stable_id (or similar), but in a separate patch. Are you okay with deferring this to a later patch Chrishtr? I've updated the comment to mention that this id is stable. https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:34: // This is used to delineate a subtree that should not be scrolled. For On 2016/09/07 at 14:19:23, ajuma wrote: > "should not be scrolled independently" maybe? I changed this line to "This is used for subtrees that should not be scrolled independently." which is cleaner and simpler. https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:53: // This offset is used when |scrollable| is false and there isn't a transform On 2016/09/07 at 14:19:23, ajuma wrote: > "transform node" Done
https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h File cc/trees/scroll_node.h (right): https://codereview.chromium.org/2310323002/diff/1/cc/trees/scroll_node.h#newc... cc/trees/scroll_node.h:32: int owner_id; On 2016/09/07 at 17:58:11, pdr. wrote: > On 2016/09/07 at 14:19:23, ajuma wrote: > > On 2016/09/06 23:40:16, chrishtr wrote: > > > Rename to owning_layer_id? > > > > Since changing the scroll offset stored at this node can potentially move many layers, and since there's no hierarchical relationship between these layers in SPv2, or even now on the compositor thread, if we rename this I'd prefer that the name emphasizes that this id is stable rather than that it happens to match a layer id right now (we're working towards removing the assumption that property tree nodes have "owning" layers). Also whatever terminology we end up using here should be consistent across node types. > > It looks like the owner_id name is used for many other trees too. I think it could make sense to rename them all to stable_id (or similar), but in a separate patch. Are you okay with deferring this to a later patch Chrishtr? cool. > > I've updated the comment to mention that this id is stable. https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h File cc/trees/scroll_node.h (right): https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h#... cc/trees/scroll_node.h:33: // is stable. "stable across property tree rebuilds". It's not stable against mutations in Blink which require rebuilding the composited layer tree.
https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h File cc/trees/scroll_node.h (right): https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h#... cc/trees/scroll_node.h:33: // is stable. On 2016/09/07 at 17:59:45, chrishtr wrote: > "stable across property tree rebuilds". It's not stable against mutations in Blink which require > rebuilding the composited layer tree. Hmm... I see what you mean. This is tough because we don't have a concrete definition of stability yet. Do you have a suggestion for how to phrase this better?
https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h File cc/trees/scroll_node.h (right): https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h#... cc/trees/scroll_node.h:33: // is stable. On 2016/09/07 at 18:02:48, pdr. wrote: > On 2016/09/07 at 17:59:45, chrishtr wrote: > > "stable across property tree rebuilds". It's not stable against mutations in Blink which require > > rebuilding the composited layer tree. > > Hmm... I see what you mean. This is tough because we don't have a concrete definition of stability yet. > > Do you have a suggestion for how to phrase this better? Stable across frames that don't include DOM mutations? Or maybe just explictly "stable across frames that don't change the composited layer list"
On 2016/09/07 at 18:05:30, chrishtr wrote: > https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h > File cc/trees/scroll_node.h (right): > > https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h#... > cc/trees/scroll_node.h:33: // is stable. > On 2016/09/07 at 18:02:48, pdr. wrote: > > On 2016/09/07 at 17:59:45, chrishtr wrote: > > > "stable across property tree rebuilds". It's not stable against mutations in Blink which require > > > rebuilding the composited layer tree. > > > > Hmm... I see what you mean. This is tough because we don't have a concrete definition of stability yet. > > > > Do you have a suggestion for how to phrase this better? > > Stable across frames that don't include DOM mutations? Or maybe just explictly "stable across frames that don't change the composited layer list" SGTM, done. Ali, is the updated comment okay with you too?
On 2016/09/07 18:08:44, pdr. wrote: > On 2016/09/07 at 18:05:30, chrishtr wrote: > > https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h > > File cc/trees/scroll_node.h (right): > > > > > https://codereview.chromium.org/2310323002/diff/20001/cc/trees/scroll_node.h#... > > cc/trees/scroll_node.h:33: // is stable. > > On 2016/09/07 at 18:02:48, pdr. wrote: > > > On 2016/09/07 at 17:59:45, chrishtr wrote: > > > > "stable across property tree rebuilds". It's not stable against mutations > in Blink which require > > > > rebuilding the composited layer tree. > > > > > > Hmm... I see what you mean. This is tough because we don't have a concrete > definition of stability yet. > > > > > > Do you have a suggestion for how to phrase this better? > > > > Stable across frames that don't include DOM mutations? Or maybe just explictly > "stable across frames that don't change the composited layer list" > > SGTM, done. > > Ali, is the updated comment okay with you too? Yes, still lgtm.
The CQ bit was checked by pdr@chromium.org
In we go! Thanks all
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.
Description was changed from ========== Add comments describing the cc ScrollNode members This patch adds some comments to describe the cc ScrollNode members. BUG=644514 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Add comments describing the cc ScrollNode members This patch adds some comments to describe the cc ScrollNode members. BUG=644514 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/227d9305559686377e566989ae24b8656acad15e Cr-Commit-Position: refs/heads/master@{#417071} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/227d9305559686377e566989ae24b8656acad15e Cr-Commit-Position: refs/heads/master@{#417071} |
