cc: Put to_target and to_screen behind an accessor.

cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2c5f8d42b4bc2282953fb4c724831b8d7aac98f1
Cr-Commit-Position: refs/heads/master@{#398989}

[sunxd, 2016-06-02 21:05:49]

Description was changed from

==========
cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
==========

to

==========
cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-06-03 17:27:01]

sunxd@chromium.org changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org, weiliangc@chromium.org

[ajuma, 2016-06-03 19:35:29]

It'd be nice to enforce the use of the accessors (right now the fields are still
public). Here's an idea: what if we make a TransformNodeCachedData struct, move
the fields we plan to eventually remove from TransformNodeData into
TransformNodeCachedData (start with the ones you've already added accessors for,
along with from_target and from_screen), and have a private vector of
TransformNodeCachedData inside TransformTree? Then the end point of the
transform tree cleanup would be the removal of TransformNodeCachedData (with the
caching we actually need moved outside the tree in a way that doesn't depend on
transform nodes knowing about their target_ids).

https://codereview.chromium.org/2032213002/diff/20001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2032213002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:508: gfx::Transform ToTarget(int id) const;
const gfx::Transform&

https://codereview.chromium.org/2032213002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:510: gfx::Transform ToScreen(int id) const;
const gfx::Transform&

https://codereview.chromium.org/2032213002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:512: int TargetId(int id) const;
Nit: here and below, s/id/node_id (so it's clearer we mean the node id and not
the target id).

[ajuma, 2016-06-07 19:51:01]

Just a few nits:

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.cc:1424: cached_data_.pop_back();
Can we just read the 0-th node directly into the existing node (like
PropertyTree::FromProtobuf does)?

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.h:521: void SetFromTarget(int node_id, const
gfx::Transform& transform);
Please add whitespace in between pairs of getters/setters so this is easier to
read.

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_unittest.cc (right):

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_unittest.cc:168: third.id = original.Insert(third, 1);
Is this change needed? We don't seem to use |second| or |third| again after
this. (Also more places below.)

[sunxd, 2016-06-07 20:34:22]

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.cc:1424: cached_data_.pop_back();
On 2016/06/07 19:51:01, ajuma wrote:
> Can we just read the 0-th node directly into the existing node (like
> PropertyTree::FromProtobuf does)?

Done.

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.h:521: void SetFromTarget(int node_id, const
gfx::Transform& transform);
On 2016/06/07 19:51:01, ajuma wrote:
> Please add whitespace in between pairs of getters/setters so this is easier to
> read.

Done.

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_unittest.cc (right):

https://codereview.chromium.org/2032213002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_unittest.cc:168: third.id = original.Insert(third, 1);
On 2016/06/07 19:51:01, ajuma wrote:
> Is this change needed? We don't seem to use |second| or |third| again after
> this. (Also more places below.)

Done.

[ajuma, 2016-06-07 20:39:19]

Thanks, lgtm

https://codereview.chromium.org/2032213002/diff/80001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2032213002/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.cc:1423: // delete the node created when initializing
transform tree
This comment is now obsolete.

[sunxd, 2016-06-07 20:47:26]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[sunxd, 2016-06-07 21:01:35]

The CQ bit was unchecked by sunxd@chromium.org

[sunxd, 2016-06-07 21:01:41]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-06-07 21:01:42]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2032213002/#ps100001
(title: "Delete obsolete comments")

[commit-bot: I haz the power, 2016-06-07 21:02:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032213002/100001

[commit-bot: I haz the power, 2016-06-07 21:29:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-07 21:29:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sunxd, 2016-06-07 23:13:21]

Description was changed from

==========
cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-06-07 23:13:22]

sunxd@chromium.org changed reviewers:
+ jbroman@chromium.org

[sunxd, 2016-06-07 23:15:11]

Hi jbroman@,

Can you review the changes under third_party/WebKit/Source/platform/graphics/?

Thanks!

[jbroman, 2016-06-08 14:39:18]

lgtm

[sunxd, 2016-06-08 14:42:25]

The CQ bit was checked by sunxd@chromium.org

[commit-bot: I haz the power, 2016-06-08 14:43:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032213002/100001

[commit-bot: I haz the power, 2016-06-08 16:33:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-08 16:33:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sunxd, 2016-06-08 17:15:01]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-06-08 17:15:01]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2032213002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-08 17:15:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032213002/120001

[commit-bot: I haz the power, 2016-06-08 19:41:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-08 19:41:11]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sunxd, 2016-06-09 17:51:36]

The CQ bit was checked by sunxd@chromium.org

[commit-bot: I haz the power, 2016-06-09 17:53:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032213002/120001

[commit-bot: I haz the power, 2016-06-09 20:05:52]

Description was changed from

==========
cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-09 20:05:57]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-09 20:06:18]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-09 20:07:08]

Description was changed from

==========
cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Put to_target and to_screen behind an accessor.

Target-related information will eventually be removed from transform
tree and computed when needed.

This CL wraps the target-related information in transform tree in an
accessor, so that we can hide the implementations of the accessor, thus
we can make the code work with either the old way or the new way of
computing the information each time we need it.

BUG=597721
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2c5f8d42b4bc2282953fb4c724831b8d7aac98f1
Cr-Commit-Position: refs/heads/master@{#398989}
==========

[commit-bot: I haz the power, 2016-06-09 20:07:09]

Patchset 7 (id:??) landed as
https://crrev.com/2c5f8d42b4bc2282953fb4c724831b8d7aac98f1
Cr-Commit-Position: refs/heads/master@{#398989}