Change cc::ElementId to be a uint64_t

Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a uint64_t
has 62 bits for node id, which is a large enough number to never
be hit in any real web page.

TBR=danakj
BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2860293002
Cr-Commit-Position: refs/heads/master@{#470915}
Committed: https://chromium.googlesource.com/chromium/src/+/c3daf2708acac918554319be74a152821bb7feec

[chrishtr, 2017-05-04 23:13:05]

Description was changed from

==========
none


none


none


none


none

BUG=
==========

to

==========
none


none


none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-05-04 23:13:06]

Description was changed from

==========
none


none


none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
none


none


none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-05-04 23:14:29]

Description was changed from

==========
none


none


none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be an unsigned long long

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-05-04 23:19:51]

Description was changed from

==========
Change cc::ElementId to be an unsigned long long

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be an unsigned long long

This will:
* Makes cc::ElementId a fully opaque id to cc (per original design)
* Makes it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a long long must have at least
64 bits, which means 62 bits for node id, which is a large enough number
to never be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-05-05 03:39:38]

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

[commit-bot: I haz the power, 2017-05-05 03:39:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-05-05 03:40:10]

chrishtr@chromium.org changed reviewers:
+ ajuma@chromium.org, suzyh@chromium.org, wkorman@chromium.org

[chrishtr, 2017-05-05 03:40:11]

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:170:
kNoPlaceholderCanvas = 0,  // DOMNodeIds starts from 1, using 0 to
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/WeakI...

always returns a value > 0.

[chrishtr, 2017-05-05 03:40:40]

Description was changed from

==========
Change cc::ElementId to be an unsigned long long

This will:
* Makes cc::ElementId a fully opaque id to cc (per original design)
* Makes it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a long long must have at least
64 bits, which means 62 bits for node id, which is a large enough number
to never be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be an unsigned long long

This will:
* Makes cc::ElementId a fully opaque id to cc (per original design)
* Makes it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as an unsigned long long must
have at least 64 bits, which means 62 bits for node id, which is a
large enough number to never be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-05-05 03:46:09]

pdr@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2017-05-05 03:46:10]

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:18: enum class
CompositorSubElementId {
Do you know what this is really needed for? I think we can remove it entirely
and save some complexity.

[chrishtr, 2017-05-05 03:47:27]

On 2017/05/05 at 03:46:10, pdr wrote:
>
https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/CompositorElementId.h
(right):
> 
>
https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/CompositorElementId.h:18: enum
class CompositorSubElementId {
> Do you know what this is really needed for? I think we can remove it entirely
and save some complexity.

I don't know what it is for. But 99% of this CL is orthogonal/in the same
direction.

[commit-bot: I haz the power, 2017-05-05 04:16:07]

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

[commit-bot: I haz the power, 2017-05-05 04:16:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chrishtr, 2017-05-05 04:17:37]

chrishtr@chromium.org changed reviewers:
+ junov@chromium.org

[chrishtr, 2017-05-05 04:22:50]

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

[commit-bot: I haz the power, 2017-05-05 04:23:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[suzyh_UTC10 (ex-contributor), 2017-05-05 04:27:31]

A whole host of mostly minor comments, coming from an external perspective not
very familiar with the code.

Most of my questions are around making the types and their relationships
explicit. If there's one type throughout, that's great. If there's actually 2 or
3 different Id types, which may have different ranges, I think that should be
clearer, and explicit conversions should be in place to make it easier to catch
problems if we ever modify the size of one of the types.

Your CL description says "... 62 bits for node id, which is a large enough
number to never be hit during any real web page." Devil's advocate: what would
happen if it did? Is this assumption at least captured with a CHECK somewhere in
the code, or a test?

Otherwise, yay for simplifying code!

https://codereview.chromium.org/2860293002/diff/140001/cc/input/scrollbar_ani...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/input/scrollbar_ani...
cc/input/scrollbar_animation_controller_unittest.cc:83:
scroll_layer->SetElementId(ElementId(scroll_layer->id()));
You construct an ElementId here but not elsewhere in the CL. Is this incorrect
here? It looks like scroll_layer->id() returns an int at the moment rather than
an ElementId, so that might need to change too.

https://codereview.chromium.org/2860293002/diff/140001/cc/layers/layer_impl_u...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/layers/layer_impl_u...
cc/layers/layer_impl_unittest.cc:228: root->SetElementId(20));
Typo '20' instead of '2'?

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/element_id.h
File cc/trees/element_id.h (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/element_id.h#...
cc/trees/element_id.h:12: using ElementId = int;
Why is it an int here, when you say you're making it unsigned long long?

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:1: 
Accidental extra line?

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_damage.cc:344:
content_layer_->SetElementId(ElementId(content_layer_->id()));
Another possibly unnecessary ElementId call?

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/tree_synchron...
File cc/trees/tree_synchronizer_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/tree_synchron...
cc/trees/tree_synchronizer_unittest.cc:508: ElementId scroll_element_id =
ElementId(5);
Does the behaviour of this test change at all, given that the second ID value
that you're removing has a non-zero value this time?

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMNodeIds.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMNodeIds.h:21: static unsigned long long
IdForNode(Node*);
Is this "unsigned long long" different from the ElementId "unsigned long long"?
If not, why not use ElementId here?

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:62: void
SetPlaceholderCanvasId(unsigned canvas_id) {
Would it be worth typedef-ing canvas ID as well, to aid in having
self-documenting code?

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:170:
kNoPlaceholderCanvas = 0,  // DOMNodeIds starts from 1, using 0 to
What is the practical result of changing this from -1 to 0? Are there any tests
that need to be changed (or added) to ensure that this change has not got out of
sync with the assumptions baked into any other part of the code?

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp:96: uint64_t
element_id = DomNodeIdFromCompositorElementId(
Is uint64_t the right type here?

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp
(right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:12:
DCHECK(dom_node_id > 0 && dom_node_id < std::numeric_limits<int>::max() / 4);
Please make this 4 a named constant or similar rather than a magic number, to
make it clear why this holds.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:13: int
element_id = dom_node_id;
Use one of the unsigned types rather than int?

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:14:
element_id = element_id << 2;  // Shift to make room for enum bits.
Nit: specify "sub_element_id enum bits"

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:16: // Do not
add more of these without adjusting the bitfield arithmetic in
(If we don't remove this enum class) Can we enforce this with a test, assertion
or presubmit check?

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:26: static
const int kInvalidElementId = 0;
Why is this redeclared instead of using the ElementId constant already defined?
Indeed, why is CompositorElementId a separate type from ElementId?

Not being comfortable with this code, my best guess is that CompositorElementId
has some additional constraint that makes it a different conceptual type from
ElementId (something to do with the magic 4s around the place?). If that's the
case, then I'd suggest defining CompositorElementId as a separate typedef of the
relevant integer type (that may or may not be the same size), rather than a
typedef of ElementId directly, since that implies more of an equivalence
relationship than may be present.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:793:
visualViewport.ScrollLayer()->GetElementId() % 4);
Please explain this piece of magic with variable/method names, or at least a
comment.

[dcheng, 2017-05-05 05:02:28]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-05-05 05:02:29]

Out of curiosity, any particular reason for unsigned long long rather than
uint64_t?

[chrishtr, 2017-05-05 05:25:51]

On 2017/05/05 at 05:02:29, dcheng wrote:
> Out of curiosity, any particular reason for unsigned long long rather than
uint64_t?

Not a good reason. Fixed. :)

[chrishtr, 2017-05-05 05:34:52]

https://codereview.chromium.org/2860293002/diff/140001/cc/input/scrollbar_ani...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/input/scrollbar_ani...
cc/input/scrollbar_animation_controller_unittest.cc:83:
scroll_layer->SetElementId(ElementId(scroll_layer->id()));
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> You construct an ElementId here but not elsewhere in the CL. Is this incorrect
here? It looks like scroll_layer->id() returns an int at the moment rather than
an ElementId, so that might need to change too.

This is incorrect, fixed.

https://codereview.chromium.org/2860293002/diff/140001/cc/layers/layer_impl_u...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/layers/layer_impl_u...
cc/layers/layer_impl_unittest.cc:228: root->SetElementId(20));
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> Typo '20' instead of '2'?

It wasn't a typo, because 2 is apparently what it was before this
line, so the "change of property" didn't get exercised. Changed
it to 3 for a bit better clarity.

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/element_id.h
File cc/trees/element_id.h (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/element_id.h#...
cc/trees/element_id.h:12: using ElementId = int;
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> Why is it an int here, when you say you're making it unsigned long long?

Fixed that in the latest patchset.

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:1: 
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> Accidental extra line?

Fixed.

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_damage.cc:344:
content_layer_->SetElementId(ElementId(content_layer_->id()));
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> Another possibly unnecessary ElementId call?

Fixed.

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/tree_synchron...
File cc/trees/tree_synchronizer_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/140001/cc/trees/tree_synchron...
cc/trees/tree_synchronizer_unittest.cc:508: ElementId scroll_element_id =
ElementId(5);
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> Does the behaviour of this test change at all, given that the second ID value
that you're removing has a non-zero value this time?

It shouldn't, because cc doesn't understand secondary ids.
Don't know why the 4 was chosen, but from cc'd point of view,
if either primary or secondary key differed, the ElementId was
different.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMNodeIds.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMNodeIds.h:21: static unsigned long long
IdForNode(Node*);
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> Is this "unsigned long long" different from the ElementId "unsigned long
long"? If not, why not use ElementId here?

It's different from ElementId, because:
* ElementId is a conceptual encoding of primary and secondary keys.
* DOMNodeIds has more than one consumer (OffscreenCanvas is another).

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:62: void
SetPlaceholderCanvasId(unsigned canvas_id) {
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> Would it be worth typedef-ing canvas ID as well, to aid in having
self-documenting code?

Sure - done.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:170:
kNoPlaceholderCanvas = 0,  // DOMNodeIds starts from 1, using 0 to
On 2017/05/05 at 04:27:30, suzyh_UTC10 wrote:
> What is the practical result of changing this from -1 to 0?

I made this change because bit-shifting signed integers is weird.
I think the practical change should be nothing, because
kNoPlaceholderCanvas was just the initialized value of
placeholder_canvas_id_ before being set. So either way the
code is able to detect whether the placeholder_canvas_id was set.

> Are there any tests that need to be changed (or added) to ensure that this
change has not got out of sync with the assumptions baked into any other part of
the code?

Re tests changed: I guess not, since they seem to pass.
Re added: I think existing tests should exercise the code,
as well as the DCHECK in DomNodeIds. I'll wait for feedback from
junov@ on this also.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp
(right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:12:
DCHECK(dom_node_id > 0 && dom_node_id < std::numeric_limits<int>::max() / 4);
On 2017/05/05 at 04:27:31, suzyh_UTC10 wrote:
> Please make this 4 a named constant or similar rather than a magic number, to
make it clear why this holds.

Done.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:13: int
element_id = dom_node_id;
On 2017/05/05 at 04:27:31, suzyh_UTC10 wrote:
> Use one of the unsigned types rather than int?

Done. Sorry for forgetting to update this file.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:14:
element_id = element_id << 2;  // Shift to make room for enum bits.
On 2017/05/05 at 04:27:31, suzyh_UTC10 wrote:
> Nit: specify "sub_element_id enum bits"

Done.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:18: enum class
CompositorSubElementId {
On 2017/05/05 at 03:46:10, pdr. wrote:
> Do you know what this is really needed for? I think we can remove it entirely
and save some complexity.

Thought about this some. Perhaps the reason for these enum bits is that
it was a way to express different compositor gestures/animations for
the same DOM Element, under the design constraint that all cc::ElementId
objects are unique. The uniqueness is probably required by the way
there is a cc::ElementId->cc::Layer map
(LayerTreeHost::element_layers_map_).

We can refactor this away I think for SPv2.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:26: static
const int kInvalidElementId = 0;
On 2017/05/05 at 04:27:31, suzyh_UTC10 wrote:
> Why is this redeclared instead of using the ElementId constant already
defined? Indeed, why is CompositorElementId a separate type from ElementId?

Fixed.

> 
> Not being comfortable with this code, my best guess is that
CompositorElementId has some additional constraint that makes it a different
conceptual type from ElementId (something to do with the magic 4s around the
place?). If that's the case, then I'd suggest defining CompositorElementId as a
separate typedef of the relevant integer type (that may or may not be the same
size), rather than a typedef of ElementId directly, since that implies more of
an equivalence relationship than may be present.

I think it's good that cc::Element is the same as CompositorElementId,
as the latter is just a way of plumbing an external type into Blink.
I don't think the change under my patch is any worse really than the
old code, is it?

Also, see above for a guessed response to pdr's comment.

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:793:
visualViewport.ScrollLayer()->GetElementId() % 4);
On 2017/05/05 at 04:27:31, suzyh_UTC10 wrote:
> Please explain this piece of magic with variable/method names, or at least a
comment.

Done.

[chrishtr, 2017-05-05 05:37:28]

Description was changed from

==========
Change cc::ElementId to be an unsigned long long

This will:
* Makes cc::ElementId a fully opaque id to cc (per original design)
* Makes it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as an unsigned long long must
have at least 64 bits, which means 62 bits for node id, which is a
large enough number to never be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be a uint64_t

This will:
* Makes cc::ElementId a fully opaque id to cc (per original design)
* Makes it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as an unsigned long long must
have at least 64 bits, which means 62 bits for node id, which is a
large enough number to never be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-05-05 05:41:09]

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

[chrishtr, 2017-05-05 05:41:20]

Description was changed from

==========
Change cc::ElementId to be a uint64_t

This will:
* Makes cc::ElementId a fully opaque id to cc (per original design)
* Makes it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as an unsigned long long must
have at least 64 bits, which means 62 bits for node id, which is a
large enough number to never be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as an unsigned long long must
have at least 64 bits, which means 62 bits for node id, which is a
large enough number to never be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-05-05 05:41:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-05-05 05:41:50]

Description was changed from

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as an unsigned long long must
have at least 64 bits, which means 62 bits for node id, which is a
large enough number to never be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a uint64_t
has 62 bits for node id, which is a large enough number to never
be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-05-05 05:44:50]

Description was changed from

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a uint64_t
has 62 bits for node id, which is a large enough number to never
be hit during any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a uint64_t
has 62 bits for node id, which is a large enough number to never
be hit in any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-05-05 06:01:48]

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

[commit-bot: I haz the power, 2017-05-05 06:01:50]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ajuma, 2017-05-05 13:46:42]

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/2860293002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:18: enum class
CompositorSubElementId {
On 2017/05/05 05:34:52, chrishtr wrote:
> On 2017/05/05 at 03:46:10, pdr. wrote:
> > Do you know what this is really needed for? I think we can remove it
entirely
> and save some complexity.
> 
> Thought about this some. Perhaps the reason for these enum bits is that
> it was a way to express different compositor gestures/animations for
> the same DOM Element, under the design constraint that all cc::ElementId
> objects are unique. The uniqueness is probably required by the way
> there is a cc::ElementId->cc::Layer map
> (LayerTreeHost::element_layers_map_).
> 
> We can refactor this away I think for SPv2.

Yeah, my recollection is that these were added because a single
CompositedLayerMapping (corresponding to a single DOM Element via the owning
PaintLayer) can own multiple GraphicsLayers that each need an element id and
these ids need to be distinct.

https://codereview.chromium.org/2860293002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/2860293002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:62: void
SetPlaceholderCanvasId(unsigned canvas_id) {
uint64_t? (here and below)

https://codereview.chromium.org/2860293002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right):

https://codereview.chromium.org/2860293002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:792:
EXPECT_EQ(static_cast<unsigned long long>(CompositorSubElementId::kViewport),
uint64_t?

[chrishtr, 2017-05-05 15:37:47]

https://codereview.chromium.org/2860293002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/2860293002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:62: void
SetPlaceholderCanvasId(unsigned canvas_id) {
On 2017/05/05 at 13:46:42, ajuma wrote:
> uint64_t? (here and below)

Done.

https://codereview.chromium.org/2860293002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right):

https://codereview.chromium.org/2860293002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:792:
EXPECT_EQ(static_cast<unsigned long long>(CompositorSubElementId::kViewport),
On 2017/05/05 at 13:46:42, ajuma wrote:
> uint64_t?

Done.

[chrishtr, 2017-05-05 15:41:43]

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

[commit-bot: I haz the power, 2017-05-05 15:42:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 16:56:42]

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

[commit-bot: I haz the power, 2017-05-05 16:56:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-05-05 18:43:38]

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

[commit-bot: I haz the power, 2017-05-05 18:44:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-05-05 19:09:58]

Looks good now? I fixed two more problems caused by an uninitialized variable.

[wkorman, 2017-05-05 20:06:10]

lgtm

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/element_id.h
File cc/trees/element_id.h (left):

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/element_id.h#...
cc/trees/element_id.h:27: // Element ids are chosen by cc's clients and can be
used as a stable identifier
Shouldn't we keep some of this documentation? Or, where has it moved to?

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/element_id.h#...
cc/trees/element_id.h:75: CC_EXPORT std::ostream& operator<<(std::ostream& out,
const ElementId& id);
It occurs to me, just to mention another non obvious effect of this change, that
viewing element id in log output is made more complex.

With separate primary/secondary fields and the stream operator the values could
be easily discerned.

This is not a big deal since perhaps we'll do away with the secondary value but
wanted to mention. We could add a ToString-type method if we needed it in
interim.

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:383: ElementId scrolling_element_id = 0;
kInvalidElementId here?

https://codereview.chromium.org/2860293002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMNodeIds.h (right):

https://codereview.chromium.org/2860293002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMNodeIds.h:21: static uint64_t
IdForNode(Node*);
Could be worth defining something like 'using NodeId = uint64_t' and using
NodeId here and below for more helpful typing where referenced.

https://codereview.chromium.org/2860293002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp
(right):

https://codereview.chromium.org/2860293002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:21: uint64_t
DomNodeIdFromCompositorElementId(CompositorElementId id) {
Naming style question -- feel like usually we see method names for conversions
like this as XToY rather than YFromX. Which is preferred / more common?

[commit-bot: I haz the power, 2017-05-05 20:14:39]

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

[commit-bot: I haz the power, 2017-05-05 20:14:41]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[chrishtr, 2017-05-05 20:46:09]

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/element_id.h
File cc/trees/element_id.h (left):

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/element_id.h#...
cc/trees/element_id.h:27: // Element ids are chosen by cc's clients and can be
used as a stable identifier
On 2017/05/05 at 20:06:10, wkorman wrote:
> Shouldn't we keep some of this documentation? Or, where has it moved to?

Oh right. Re-added.

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2860293002/diff/360001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:383: ElementId scrolling_element_id = 0;
On 2017/05/05 at 20:06:10, wkorman wrote:
> kInvalidElementId here?

Done.

https://codereview.chromium.org/2860293002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMNodeIds.h (right):

https://codereview.chromium.org/2860293002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMNodeIds.h:21: static uint64_t
IdForNode(Node*);
On 2017/05/05 at 20:06:10, wkorman wrote:
> Could be worth defining something like 'using NodeId = uint64_t' and using
NodeId here and below for more helpful typing where referenced.

Done.

https://codereview.chromium.org/2860293002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp
(right):

https://codereview.chromium.org/2860293002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:21: uint64_t
DomNodeIdFromCompositorElementId(CompositorElementId id) {
On 2017/05/05 at 20:06:10, wkorman wrote:
> Naming style question -- feel like usually we see method names for conversions
like this as XToY rather than YFromX. Which is preferred / more common?

Not sure. Happy to change if you want.

[chrishtr, 2017-05-05 20:46:13]

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

[commit-bot: I haz the power, 2017-05-05 20:47:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ajuma, 2017-05-05 20:55:22]

lgtm

[chrishtr, 2017-05-05 20:56:00]

chrishtr@chromium.org changed reviewers:
+ danakj@chromium.org

[chrishtr, 2017-05-05 20:56:01]

+danakj@ to review ui/compositor/

[commit-bot: I haz the power, 2017-05-05 21:17:03]

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

[commit-bot: I haz the power, 2017-05-05 21:17:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2017-05-05 22:32:21]

Chris and I talked about this offline a bit. I'm scared about the loss of type
safety because it's so easy to accidentally pass in a layer id as an element id.
Ali Juma saved me twice from landing code with that bug in layer_ids, and I've
fixed other cases existing in the code. I think Chris is going to try wrapping
the int64 in a simple class so we'll still get the perf wins. The class's
comparison and hash functions will hopefully just forward to the int64 versions
but I am sure the compiler will make that a multi-hour exercise.

[chrishtr, 2017-05-06 06:02:08]

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

[commit-bot: I haz the power, 2017-05-06 06:02:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 06:07:13]

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

[commit-bot: I haz the power, 2017-05-06 06:07:14]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chrishtr, 2017-05-06 17:03:12]

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

[commit-bot: I haz the power, 2017-05-06 17:03:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 17:17:54]

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

[commit-bot: I haz the power, 2017-05-06 17:17:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chrishtr, 2017-05-06 21:01:42]

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

[commit-bot: I haz the power, 2017-05-06 21:01:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 21:15:43]

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

[commit-bot: I haz the power, 2017-05-06 21:15:44]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chrishtr, 2017-05-07 00:17:16]

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

[commit-bot: I haz the power, 2017-05-07 00:17:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-07 00:34:10]

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

[commit-bot: I haz the power, 2017-05-07 00:34:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrishtr, 2017-05-07 14:14:41]

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

[commit-bot: I haz the power, 2017-05-07 14:14:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-07 14:40:29]

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

[commit-bot: I haz the power, 2017-05-07 14:40:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chrishtr, 2017-05-07 16:25:42]

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

[commit-bot: I haz the power, 2017-05-07 16:25:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-05-07 16:26:15]

On 2017/05/05 at 22:32:21, pdr wrote:
> Chris and I talked about this offline a bit. I'm scared about the loss of type
safety because it's so easy to accidentally pass in a layer id as an element id.
Ali Juma saved me twice from landing code with that bug in layer_ids, and I've
fixed other cases existing in the code. I think Chris is going to try wrapping
the int64 in a simple class so we'll still get the perf wins. The class's
comparison and hash functions will hopefully just forward to the int64 versions
but I am sure the compiler will make that a multi-hour exercise.

Now with types. I did it a way that avoids custom hash functions, which is a
performance and complexity win.

[commit-bot: I haz the power, 2017-05-07 17:47:09]

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

[commit-bot: I haz the power, 2017-05-07 17:47:10]

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_...)

[pdr., 2017-05-07 18:34:44]

On 2017/05/07 at 16:26:15, chrishtr wrote:
> On 2017/05/05 at 22:32:21, pdr wrote:
> > Chris and I talked about this offline a bit. I'm scared about the loss of
type safety because it's so easy to accidentally pass in a layer id as an
element id. Ali Juma saved me twice from landing code with that bug in
layer_ids, and I've fixed other cases existing in the code. I think Chris is
going to try wrapping the int64 in a simple class so we'll still get the perf
wins. The class's comparison and hash functions will hopefully just forward to
the int64 versions but I am sure the compiler will make that a multi-hour
exercise.
> 
> Now with types. I did it a way that avoids custom hash functions, which is a
performance and complexity win.

I think this is a great improvement but would it be that much complexity to go
all the way with a custom hash function that forwards to int64_t's? I am
thinking something like the following which should have equivalent performance:
typedef uint64_t Id;
class ElementId {
 public:
  explicit ElementId(Id id) : id_(id) {}

  Id id() const { return id_; }
  bool operator==(const ElementId& other) const { return id_ == other.id_; }
 private:
  Id id_;
};

namespace std {
  template<> struct hash<ElementId> {
    size_t operator()(const ElementId& x) const {
      return hash<Id>()(x.id());
    }
  };
}

[suzyh_UTC10 (ex-contributor), 2017-05-08 01:04:16]

It's no longer clear to me that this substantially simplifies the code, but I
defer to pdr on this. Remember to update the CL description.

Speaking of which, I'm not sure if my other comment was seen. Repeating:
Your CL description says "... 62 bits for node id, which is a large enough
number to never be hit during any real web page." Devil's advocate: what would
happen if it did? Is this assumption at least captured with a CHECK somewhere in
the code, or a test?

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMNodeIds.cpp (right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMNodeIds.cpp:14: uint64_t
DOMNodeIds::IdForNode(Node* node) {
Use DOMNodeId instead of uint64_t throughout this file?

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp (right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp:96: uint64_t
element_id = DomNodeIdFromCompositorElementId(
DOMNodeId instead of uint64_t? Also consider renaming the variable, since it's
not an ElementId.

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp
(right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:17:
element_id.id = element_id.id
Could you just write
element_id.id = dom_node_id << 2;  // Shift to ....
here instead?

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:27:
CompositorSubElementId SubElementIdFromCompositorElementId(
Do these functions belong inside the definition of ElementId instead of as
separate helper functions?

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:23:
kNumSubElementTypes = 4
Add a blank line and comment before this entry to make it obvious that it is a
different kind of thing, and that any new values for the enum should be added
before it. To avoid mistakes in the value for kNumSubElementTypes, you might
want to use the auto-one-more-than-last-entry value instead of specifying the
value.

Random thought: Is there any chance that by including this value in the enum,
the compiler decides it needs more bits to store a CompositorSubElementId, in
such a way that results in a performance/size regression?

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.cpp
(right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.cpp:44:
static_cast<unsigned long>(compositor_element_id_.id),
Is this cast necessary? Is it not possible to specify a uint64_t value in the
format string?

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.cpp
(right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.cpp:47:
static_cast<unsigned long>(compositor_element_id_.id));
Ditto here, is the cast required?

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:794:
EXPECT_EQ(static_cast<uint64_t>(CompositorSubElementId::kViewport),
Is this cast necessary?

[chrishtr, 2017-05-08 16:32:13]

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMNodeIds.cpp (right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMNodeIds.cpp:14: uint64_t
DOMNodeIds::IdForNode(Node* node) {
On 2017/05/08 at 01:04:15, suzyh_UTC10 wrote:
> Use DOMNodeId instead of uint64_t throughout this file?

Done.

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp (right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp:96: uint64_t
element_id = DomNodeIdFromCompositorElementId(
On 2017/05/08 at 01:04:15, suzyh_UTC10 wrote:
> DOMNodeId instead of uint64_t? Also consider renaming the variable, since it's
not an ElementId.

Done.

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp
(right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:17:
element_id.id = element_id.id
On 2017/05/08 at 01:04:15, suzyh_UTC10 wrote:
> Could you just write
> element_id.id = dom_node_id << 2;  // Shift to ....
> here instead?

Done.

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:27:
CompositorSubElementId SubElementIdFromCompositorElementId(
On 2017/05/08 at 01:04:15, suzyh_UTC10 wrote:
> Do these functions belong inside the definition of ElementId instead of as
separate helper functions?

No, because that type is defined in cc and by design cc doesn't know about
the needs of "clients" like Blink or ui/

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:23:
kNumSubElementTypes = 4
On 2017/05/08 at 01:04:15, suzyh_UTC10 wrote:
> Add a blank line and comment before this entry to make it obvious that it is a
different kind of thing, and that any new values for the enum should be added
before it. To avoid mistakes in the value for kNumSubElementTypes, you might
want to use the auto-one-more-than-last-entry value instead of specifying the
value.

Done.

> 
> Random thought: Is there any chance that by including this value in the enum,
the compiler decides it needs more bits to store a CompositorSubElementId, in
such a way that results in a performance/size regression?

I don't think so, because it isn't actually stored anywhere other than
in method parameters after this CL.

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.cpp
(right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.cpp:44:
static_cast<unsigned long>(compositor_element_id_.id),
On 2017/05/08 at 01:04:15, suzyh_UTC10 wrote:
> Is this cast necessary? Is it not possible to specify a uint64_t value in the
format string?

Yes it is, sadly. I ran in to some annoying compile errors in earlier
iterations of the patch because apparently on some platforms uint64 is an
unsigned long long and some an unsigned long. Forcing the static cast
to match the printf % directive makes it work everywhere.

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.cpp
(right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.cpp:47:
static_cast<unsigned long>(compositor_element_id_.id));
On 2017/05/08 at 01:04:15, suzyh_UTC10 wrote:
> Ditto here, is the cast required?

Ditto.

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right):

https://codereview.chromium.org/2860293002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:794:
EXPECT_EQ(static_cast<uint64_t>(CompositorSubElementId::kViewport),
On 2017/05/08 at 01:04:15, suzyh_UTC10 wrote:
> Is this cast necessary?

Not sure. Compiles locally.. Removed it for now, will see what the bots say.

[chrishtr, 2017-05-08 16:32:22]

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

[commit-bot: I haz the power, 2017-05-08 16:33:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-05-08 16:33:39]

On 2017/05/07 at 18:34:44, pdr wrote:
> On 2017/05/07 at 16:26:15, chrishtr wrote:
> > On 2017/05/05 at 22:32:21, pdr wrote:
> > > Chris and I talked about this offline a bit. I'm scared about the loss of
type safety because it's so easy to accidentally pass in a layer id as an
element id. Ali Juma saved me twice from landing code with that bug in
layer_ids, and I've fixed other cases existing in the code. I think Chris is
going to try wrapping the int64 in a simple class so we'll still get the perf
wins. The class's comparison and hash functions will hopefully just forward to
the int64 versions but I am sure the compiler will make that a multi-hour
exercise.
> > 
> > Now with types. I did it a way that avoids custom hash functions, which is a
performance and complexity win.
> 
> I think this is a great improvement but would it be that much complexity to go
all the way with a custom hash function that forwards to int64_t's? I am
thinking something like the following which should have equivalent performance:

It requires adding hash functions in both Blink and cc. I don't think it's worth
it.

> typedef uint64_t Id;
> class ElementId {
>  public:
>   explicit ElementId(Id id) : id_(id) {}
> 
>   Id id() const { return id_; }
>   bool operator==(const ElementId& other) const { return id_ == other.id_; }
>  private:
>   Id id_;
> };
> 
> namespace std {
>   template<> struct hash<ElementId> {
>     size_t operator()(const ElementId& x) const {
>       return hash<Id>()(x.id());
>     }
>   };
> }

[chrishtr, 2017-05-08 16:36:19]

On 2017/05/08 at 16:33:39, chrishtr wrote:
> On 2017/05/07 at 18:34:44, pdr wrote:
> > On 2017/05/07 at 16:26:15, chrishtr wrote:
> > > On 2017/05/05 at 22:32:21, pdr wrote:
> > > > Chris and I talked about this offline a bit. I'm scared about the loss
of type safety because it's so easy to accidentally pass in a layer id as an
element id. Ali Juma saved me twice from landing code with that bug in
layer_ids, and I've fixed other cases existing in the code. I think Chris is
going to try wrapping the int64 in a simple class so we'll still get the perf
wins. The class's comparison and hash functions will hopefully just forward to
the int64 versions but I am sure the compiler will make that a multi-hour
exercise.
> > > 
> > > Now with types. I did it a way that avoids custom hash functions, which is
a performance and complexity win.
> > 
> > I think this is a great improvement but would it be that much complexity to
go all the way with a custom hash function that forwards to int64_t's? I am
thinking something like the following which should have equivalent performance:
> 
> It requires adding hash functions in both Blink and cc. I don't think it's
worth it.

But whatever, you decide.

> 
> > typedef uint64_t Id;
> > class ElementId {
> >  public:
> >   explicit ElementId(Id id) : id_(id) {}
> > 
> >   Id id() const { return id_; }
> >   bool operator==(const ElementId& other) const { return id_ == other.id_; }
> >  private:
> >   Id id_;
> > };
> > 
> > namespace std {
> >   template<> struct hash<ElementId> {
> >     size_t operator()(const ElementId& x) const {
> >       return hash<Id>()(x.id());
> >     }
> >   };
> > }

[commit-bot: I haz the power, 2017-05-08 16:53:11]

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

[commit-bot: I haz the power, 2017-05-08 16:53:13]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[wkorman, 2017-05-08 18:23:25]

It does feel like we've lost a chunk of the "simpler code" bullet point in
original with need to write things like GetCompositorElementId().id in Blink and
my_element_id_map_[element_id.id] throughout code.

What was the specific catalyst that kicked this change off again -- did you need
to add a new wtf impl for something that the current code didn't have? It would
be worth noting that in the CL desc IMO.

I would support putting back the custom hash impl in cc and blink.

I think a primary value of this change is to cease using two ints to hold what
we know only needs one, so could land this and make other tweaks subsequently
IMO. But CL desc should be revised.

https://codereview.chromium.org/2860293002/diff/520001/cc/animation/animation...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/2860293002/diff/520001/cc/animation/animation...
cc/animation/animation_host.h:50: std::unordered_map<ElementId::Id,
scoped_refptr<ElementAnimations>>;
My personal preference given our shift to support pdr@ pitch re: type safety,
and balancing readability/maintenance, would be to not leak ElementId::Id
outside of the class itself and just use ElementId and add operators / hash
funcs if needed. But I am ok trying it this way if we'd like.

https://codereview.chromium.org/2860293002/diff/520001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animations.cc (right):

https://codereview.chromium.org/2860293002/diff/520001/cc/animation/scroll_of...
cc/animation/scroll_offset_animations.cc:35:
element_to_update_map_[element_id.id] = update;
The need to explicitly ref id member data in hash refs does bum me out.

https://codereview.chromium.org/2860293002/diff/520001/cc/input/scrollbar_ani...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2860293002/diff/520001/cc/input/scrollbar_ani...
cc/input/scrollbar_animation_controller_unittest.cc:84: element_id.id =
scroll_layer->id();
In subsequent change we could consider adding a TestElementId that allows
setting via ctor to reduce LOC in tests.

https://codereview.chromium.org/2860293002/diff/520001/cc/layers/painted_over...
File cc/layers/painted_overlay_scrollbar_layer.cc (right):

https://codereview.chromium.org/2860293002/diff/520001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer.cc:57: if (element_id.id ==
scroll_element_id_.id)
Can't we rely on operator== here?

https://codereview.chromium.org/2860293002/diff/520001/cc/layers/solid_color_...
File cc/layers/solid_color_scrollbar_layer.cc (right):

https://codereview.chromium.org/2860293002/diff/520001/cc/layers/solid_color_...
cc/layers/solid_color_scrollbar_layer.cc:103: if (element_id.id ==
solid_color_scrollbar_layer_inputs_.scroll_element_id.id)
Can't we rely on operator== here?

https://codereview.chromium.org/2860293002/diff/520001/cc/trees/element_id.h
File cc/trees/element_id.h (right):

https://codereview.chromium.org/2860293002/diff/520001/cc/trees/element_id.h#...
cc/trees/element_id.h:45: typedef uint64_t Id;
Can we use 'using' here instead?

https://codereview.chromium.org/2860293002/diff/520001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2860293002/diff/520001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:522: auto iter =
element_layers_map_.find(element_id.id);
Similar to my prev comment re: hash, there must be STL operator, interface, etc.
to reduce this verbosity here and below in this file in particular among those
in change.

https://codereview.chromium.org/2860293002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMNodeIds.h (right):

https://codereview.chromium.org/2860293002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMNodeIds.h:17: using DOMNodeId = uint64_t;
Maybe suzyh already asked, can we not put using above DECLARE, and then use
DOMNodeId here and same in .cpp?

https://codereview.chromium.org/2860293002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
(right):

https://codereview.chromium.org/2860293002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:56:
!element_ids.Contains(Effect()->GetCompositorElementId().id))
These bum me out and there's no STL help here due to wtf unless we go back to
the wtf hash def this change was aiming to avoid.

[chrishtr, 2017-05-08 21:56:51]

Now with moar typing. PTAL

[chrishtr, 2017-05-08 21:57:50]

And as an extra bonus, added a test.

[pdr., 2017-05-08 22:01:49]

https://codereview.chromium.org/2860293002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementIdTest.cpp
(right):

https://codereview.chromium.org/2860293002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementIdTest.cpp:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
2017

[pdr., 2017-05-08 22:01:56]

LGTM

[wkorman, 2017-05-08 22:25:26]

lgtm

https://codereview.chromium.org/2860293002/diff/540001/cc/trees/element_id.h
File cc/trees/element_id.h (right):

https://codereview.chromium.org/2860293002/diff/540001/cc/trees/element_id.h#...
cc/trees/element_id.h:67: ElementIdType id_;
For the record: we discussed in person re: making this a class rather than a
struct and having this be private. Could pursue subsequently. Only a couple of
areas in Blink read this directly currently (to output as string, and to
retrieve id we know we stuck in in the first place to pull off sub id).

Alternatives discussed trade off data hiding improvements at cost of more code
complexity or performance overhead or both. So leaving as it is seems ok for
now.

To some extent the '_' suffix should make it clear that one shouldn't monkey
with this unless one knows what one is doing.

[chrishtr, 2017-05-08 22:27:09]

Description was changed from

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a uint64_t
has 62 bits for node id, which is a large enough number to never
be hit in any real web page.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a uint64_t
has 62 bits for node id, which is a large enough number to never
be hit in any real web page.

TBR=danakj
BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-05-08 22:27:13]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-08 22:27:13]

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/2860293002/#ps540001
(title: "none")

[commit-bot: I haz the power, 2017-05-08 22:30:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-05-08 23:06:04]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-08 23:06:05]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
wkorman@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2860293002/#ps580001
(title: "Change cc::ElementId to be a uint64_t")

[chrishtr, 2017-05-08 23:07:01]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2017-05-08 23:07:47]

Patchset #30 (id:580001) has been deleted

[chrishtr, 2017-05-08 23:08:05]

Patchset #29 (id:560001) has been deleted

[chrishtr, 2017-05-08 23:17:27]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-08 23:17:27]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
wkorman@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2860293002/#ps600001
(title: "none")

[commit-bot: I haz the power, 2017-05-08 23:20:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 00:36:07]

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

[commit-bot: I haz the power, 2017-05-09 00:36:10]

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

[chrishtr, 2017-05-09 01:12:56]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-09 01:19:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-05-09 01:42:36]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-09 01:42:37]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
wkorman@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2860293002/#ps620001
(title: "none")

[commit-bot: I haz the power, 2017-05-09 01:44:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 03:05:26]

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

[commit-bot: I haz the power, 2017-05-09 03:05:29]

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

[chrishtr, 2017-05-09 04:22:29]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-09 04:22:31]

Fixed Blink hash function to differentiate between a deleted and empty state.
Used 1 for the
latter and 0 for the former. Real values start at 2.

https://codereview.chromium.org/2860293002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementIdTest.cpp
(right):

https://codereview.chromium.org/2860293002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementIdTest.cpp:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/05/08 at 22:01:49, pdr. wrote:
> 2017

Done.

[chrishtr, 2017-05-09 04:22:31]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
wkorman@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2860293002/#ps640001
(title: "none")

[commit-bot: I haz the power, 2017-05-09 04:23:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 05:46:10]

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

[commit-bot: I haz the power, 2017-05-09 05:46:12]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrishtr, 2017-05-09 14:04:55]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-09 14:05:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-05-09 15:09:09]

ui/compositor rs LGTM

[commit-bot: I haz the power, 2017-05-09 16:13:48]

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

[commit-bot: I haz the power, 2017-05-09 16:13:50]

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

[chrishtr, 2017-05-09 19:58:11]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-09 19:58:13]

Changed the trace macro key from "id" to "id_" to avoid a collision with a
tracing-internal
data structure.

[chrishtr, 2017-05-09 19:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
wkorman@chromium.org, danakj@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2860293002/#ps660001
(title: "none")

[commit-bot: I haz the power, 2017-05-09 19:59:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 22:42:37]

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

[commit-bot: I haz the power, 2017-05-09 22:42:39]

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_...)

[chrishtr, 2017-05-09 22:54:20]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-09 22:55:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 00:58:37]

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

[commit-bot: I haz the power, 2017-05-10 00:58:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))

[chrishtr, 2017-05-10 05:25:00]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-10 05:25:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 08:25:30]

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

[commit-bot: I haz the power, 2017-05-10 08:25:33]

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

[chrishtr, 2017-05-10 16:30:22]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-10 16:31:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 19:20:29]

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

[commit-bot: I haz the power, 2017-05-10 19:20:31]

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

[chrishtr, 2017-05-10 22:03:41]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-10 22:04:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 00:05:23]

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

[commit-bot: I haz the power, 2017-05-11 00:05:26]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrishtr, 2017-05-11 00:07:03]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-11 00:08:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 03:39:53]

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

[commit-bot: I haz the power, 2017-05-11 03:39:55]

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

[chrishtr, 2017-05-11 04:23:27]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-11 04:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
wkorman@chromium.org, danakj@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2860293002/#ps680001
(title: "Merge branch 'master' into secondaryid")

[commit-bot: I haz the power, 2017-05-11 04:24:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 05:49:48]

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

[commit-bot: I haz the power, 2017-05-11 05:49:51]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pdr., 2017-05-11 06:32:03]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-05-11 06:33:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 11:08:08]

CQ is committing da patch.

Bot data: {"patchset_id": 680001, "attempt_start_ts": 1494484323407170,
"parent_rev": "a4980144d56f316156e822eebd14f58ed19ef571", "commit_rev":
"c3daf2708acac918554319be74a152821bb7feec"}

[commit-bot: I haz the power, 2017-05-11 11:08:20]

Description was changed from

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a uint64_t
has 62 bits for node id, which is a large enough number to never
be hit in any real web page.

TBR=danakj
BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Change cc::ElementId to be a uint64_t

This will:
* Make cc::ElementId a fully opaque id to cc (per original design)
* Make it easy to use it as a cache key wherever desired
* Make all hash lookups involving an element id faster in cc
* Simplifies the code

I don't think there is any downside, as a uint64_t
has 62 bits for node id, which is a large enough number to never
be hit in any real web page.

TBR=danakj
BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2860293002
Cr-Commit-Position: refs/heads/master@{#470915}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3daf2708acac918554319be74a1...
==========

[commit-bot: I haz the power, 2017-05-11 11:08:23]

Committed patchset #33 (id:680001) as
https://chromium.googlesource.com/chromium/src/+/c3daf2708acac918554319be74a1...