Fix cc scrollbar layer issues with initialization, and use element ids throughout.

Fix cc scrollbar layer issues with initialization, and use element ids throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.
To do this, added a new namespace for CompsitorElementId, and also performed
some related cleanup along the way.

BUG=718564, 702832
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/2877033002
Cr-Commit-Position: refs/heads/master@{#471564}
Committed: https://chromium.googlesource.com/chromium/src/+/f556cef44f474c02e486e7c3e4eec685dc50794c

[chrishtr, 2017-05-12 04:35:08]

Description was changed from

==========
none


none


none

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

to

==========
none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chrishtr, 2017-05-12 17:44:17]

Description was changed from

==========
none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chrishtr, 2017-05-12 17:44:29]

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

[chrishtr, 2017-05-12 17:44:36]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2017-05-12 17:45:45]

Description was changed from

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.
To do this, added a new namespace for CompsitorElementId, and also performed
some related cleanup along the way.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chrishtr, 2017-05-12 17:58:02]

Description was changed from

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.
To do this, added a new namespace for CompsitorElementId, and also performed
some related cleanup along the way.

BUG=718564
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.
To do this, added a new namespace for CompsitorElementId, and also performed
some related cleanup along the way.

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-12 18:04:02]

Description was changed from

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.
To do this, added a new namespace for CompsitorElementId, and also performed
some related cleanup along the way.

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

to

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.
To do this, added a new namespace for CompsitorElementId, and also performed
some related cleanup along the way.

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

[chrishtr, 2017-05-12 18:44:20]

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

[chrishtr, 2017-05-12 18:44:36]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2017-05-12 18:49:10]

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

[chrishtr, 2017-05-12 18:49:26]

chrishtr@chromium.org changed reviewers:
+ jaydasika@chromium.org, weiliangc@chromium.org, wkorman@chromium.org

[commit-bot: I haz the power, 2017-05-12 18:50: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-12 22:17:14]

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

[commit-bot: I haz the power, 2017-05-12 22:17:16]

Dry run: This issue passed the CQ dry run.

[wkorman, 2017-05-12 22:29:39]

lgtm

https://codereview.chromium.org/2877033002/diff/120001/cc/blink/web_scrollbar...
File cc/blink/web_scrollbar_layer_impl.h (right):

https://codereview.chromium.org/2877033002/diff/120001/cc/blink/web_scrollbar...
cc/blink/web_scrollbar_layer_impl.h:42: void SetElementId(const cc::ElementId&)
override;
Is it worth using ref param given struct is basically an int? Am fine either
way, I think others have commented similarly to me in past so figured to ask and
learn again from this reviewer set.

https://codereview.chromium.org/2877033002/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2877033002/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:329: void
RegisterScrollbarAnimationController(ElementId scroll_element_id,
For comparison we don't bother with const ref here in cc-landia. Maybe we
should?

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

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:9: static
const int numBitsToShift = 3;
See prev comment re: kNamespaceBits.

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

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:23:
kMaxRepresentableNamespaceId = 8
Could define kNamespaceBits = 3 above/somewhere, and then use = 1 <<
kNamespaceBits here, and also use it in the .cpp file.

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:29:
CreateCompositorElementId(uint64_t id, CompositorElementIdNamespace);
Is conflict between scrollbar id and dom node id not possible due to additional
use of namespace? Are we ok with that as the only safeguard.

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebScrollbarLayer.h (right):

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebScrollbarLayer.h:51: // All scrollbar
layers reqiure element ids.
require

[chrishtr, 2017-05-12 23:44:07]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-12 23:44:07]

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

[chrishtr, 2017-05-12 23:44:16]

https://codereview.chromium.org/2877033002/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2877033002/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:329: void
RegisterScrollbarAnimationController(ElementId scroll_element_id,
On 2017/05/12 at 22:29:38, wkorman wrote:
> For comparison we don't bother with const ref here in cc-landia. Maybe we
should?

cc is currently consistently not doing this it seems.

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

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:9: static
const int numBitsToShift = 3;
On 2017/05/12 at 22:29:39, wkorman wrote:
> See prev comment re: kNamespaceBits.

Done.

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

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:23:
kMaxRepresentableNamespaceId = 8
On 2017/05/12 at 22:29:39, wkorman wrote:
> Could define kNamespaceBits = 3 above/somewhere, and then use = 1 <<
kNamespaceBits here, and also use it in the .cpp file.

Great idea! Done. Xianzhu++, it was his idea from elsewhere.

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:29:
CreateCompositorElementId(uint64_t id, CompositorElementIdNamespace);
On 2017/05/12 at 22:29:39, wkorman wrote:
> Is conflict between scrollbar id and dom node id not possible due to
additional use of namespace? Are we ok with that as the only safeguard.

Done.

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebScrollbarLayer.h (right):

https://codereview.chromium.org/2877033002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebScrollbarLayer.h:51: // All scrollbar
layers reqiure element ids.
On 2017/05/12 at 22:29:39, wkorman wrote:
> require

Done.

[commit-bot: I haz the power, 2017-05-12 23:44:41]

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-13 01:29:37]

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

[commit-bot: I haz the power, 2017-05-13 01:29:38]

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-13 03:30:09]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-13 03:30:27]

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-13 05:31:05]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494646209563820,
"parent_rev": "13876f5441f3660a99f4a92b118512e87359deb3", "commit_rev":
"f556cef44f474c02e486e7c3e4eec685dc50794c"}

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

Description was changed from

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.
To do this, added a new namespace for CompsitorElementId, and also performed
some related cleanup along the way.

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

to

==========
Fix cc scrollbar layer issues with initialization, and use element ids
throughout.

1. Previously, there was an issue with initialization paths for cc scrollbar
layers, which may be created when property trees are in an inconsistent state.
Now we never try to update property tree opacity during construction, and
also pass correct initial opacities from the pending tree rather than assuming
0.0f.

2. Deleted workaround code to bail out during the condition #1 described above.

3. Removed always_use_active_tree_opacity_effect_ids and corresponding methods
on layers, in favor of re-using the existing list of scrollbar animation
controllers on LayerTreeHostImpl. It turned out that there was an exact match
between the layers put in always_use_active_tree_opacity_effect_ids and
scrollbars.
This also removes another instance of looking up effect id by node.

4. Start always setting ElementId on scrollbar layers, to support #3 above.
To do this, added a new namespace for CompsitorElementId, and also performed
some related cleanup along the way.

BUG=718564,702832
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/2877033002
Cr-Commit-Position: refs/heads/master@{#471564}
Committed:
https://chromium.googlesource.com/chromium/src/+/f556cef44f474c02e486e7c3e4ee...
==========

[commit-bot: I haz the power, 2017-05-13 05:31:21]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f556cef44f474c02e486e7c3e4ee...