cc: Stop creating unused 0 property tree nodes other than transform

cc: Stop creating unused 0 property tree nodes other than transform

Other than transform trees, all other types of property trees don't
need a 0 node, thus remove them. Transform tree uses the 0 node as
device space.

Also use contants with names instead of raw numbers.

R=jbroman, ajuma
BUG=620322
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[weiliangc, 2016-06-21 20:51:32]

Description was changed from

==========
cc: Stop creating unused 0 property tree nodes other than transform

Other than transform trees, all other types of property trees don't
need a 0 node, thus remove them. Transform tree uses the 0 node as
device space.

Also use contants with names instead of raw numbers.

R=jbroman, ajuma
BUG=620322
==========

to

==========
cc: Stop creating unused 0 property tree nodes other than transform

Other than transform trees, all other types of property trees don't
need a 0 node, thus remove them. Transform tree uses the 0 node as
device space.

Also use contants with names instead of raw numbers.

R=jbroman, ajuma
BUG=620322
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[weiliangc, 2016-06-21 20:55:33]

Stop the evil that is raw int in code! (which is at least half my fault)

Also I saw in some TransformTree unittest we can ComputeTransform to -1, so
maybe we can get away without the dummy node in TransformTree as well? I
certainly hits some DCHECKs along the way if I try to just make
TransformTree::kDeviceNodeId -1.

[weiliangc, 2016-06-21 21:18:45]

On 2016/06/21 20:55:33, weiliangc wrote:
> Stop the evil that is raw int in code! (which is at least half my fault)
> 
> Also I saw in some TransformTree unittest we can ComputeTransform to -1, so
> maybe we can get away without the dummy node in TransformTree as well? I
> certainly hits some DCHECKs along the way if I try to just make
> TransformTree::kDeviceNodeId -1.

...well I fixed all the DCHECKs but still 21 unittests failures and some are
diff in clip_rect and visible_rect so not gonna attempt that right now. Going
back to vacation mode now kthxbye.

[ajuma, 2016-06-22 13:29:51]

Removing the dummy node in TransformTree as well would be really nice, if we can
get away without it.

https://codereview.chromium.org/2087963003/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2087963003/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:1245: if (node->data.replica_layer_id !=
EffectTree::kInvalidNodeId)
Layer::INVALID_ID since this is a layer id

https://codereview.chromium.org/2087963003/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:1250: return node->data.replica_layer_id !=
EffectTree::kInvalidNodeId;
Here too, Layer::INVALID_ID

https://codereview.chromium.org/2087963003/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2087963003/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:666: } else if (!can_render_to_separate_surface)
{
This can be combined with the previous condition now that both branches use
kRootNodeId.

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:120: // Add the first node.
I think this comment is obsolete now.

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1340: DCHECK(static_cast<int>(size()) > target_id) <<
target_id << " v.s. "
These can be DCHECK_GE (and then we get a readable error for free).

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1343: DCHECK_LE(target_id, node_id) << target_id << "
v.s. " << node_id;
No need to include the error message here since DCHECK_{GE,LE} will take care of
that.

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1440: //
cached_data_.back().FromProtobuf(data.cached_data(0));
<_<

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

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:383: return t->parent_id >= 0 ? Node(t->parent_id) :
nullptr;
The Node method already checks if its argument is > -1, so do we need the logic
added here? (Also for the const version below.)

[ajuma, 2016-06-22 17:06:57]

Noticed a few more ints:

https://codereview.chromium.org/2087963003/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2087963003/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:787: for (int i = 2; i <
static_cast<int>(effect_tree->size()); ++i) {
Should this be kRootNodeId+1 instead of 2?

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1603: DCHECK_EQ(0, destination_id);
TransformTree::kDeviceNodeId

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1604: source_id = 1;
TransformTree::kRootNodeId

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1662: if (size() < 2)
I think this should be 1 now that the viewport has shifted to node 0.

https://codereview.chromium.org/2087963003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1673: DCHECK_GT(size(), min_size);
And this should be a DCHECK_GE now.