VrShell: Allow native control of UI element opacity.

VrShell: Allow native control of UI element opacity.

- Add support to ContentRectangle.
- Inherit opacity from parent elements.
- Update tests.
- Drive omnibox opacity animations natively rather than via CSS.

BUG=679761
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[cjgrant, 2017-01-11 16:36:37]

Description was changed from

==========
VrShell: Allow native control of UI element opacity.

- Add support to ContentRectangle.
- Inherit opacity from parent elements.
- Update tests.
- Drive omnibox opacity animations natively rather than via CSS.

BUG=679761
==========

to

==========
VrShell: Allow native control of UI element opacity.

- Add support to ContentRectangle.
- Inherit opacity from parent elements.
- Update tests.
- Drive omnibox opacity animations natively rather than via CSS.

BUG=679761
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[cjgrant, 2017-01-11 16:45:47]

cjgrant@chromium.org changed reviewers:
+ bshe@chromium.org, mthiesse@chromium.org

[cjgrant, 2017-01-11 16:45:47]

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:490: if (!plane->visible ||
plane->computed_opacity == 0.0f ||
Checking opacity here relieves the HTML UI of having to manage both visibility
and opacity (it can use one or the other).  I stopped short of removing the
visibility toggle though, because in most cases, the toggle is more analogous to
"enabled".

[mthiesse, 2017-01-11 18:33:04]

lgtm % nits

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (left):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:354: void
UiScene::ApplyRecursiveTransforms(const ContentRectangle& element,
Unrelated, but can you add a TODO to be smarter about how we compute these
transforms? We could theoretically be doing a single pass.

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (right):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:361: float *opacity) {
float*

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.h:75: float *opacity);
nit: float*

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:490: if (!plane->visible ||
plane->computed_opacity == 0.0f ||
On 2017/01/11 16:45:47, cjgrant wrote:
> Checking opacity here relieves the HTML UI of having to manage both visibility
> and opacity (it can use one or the other).  I stopped short of removing the
> visibility toggle though, because in most cases, the toggle is more analogous
to
> "enabled".

Maybe add a function IsVisible() to ContentRectangle?

[cjgrant, 2017-01-11 18:53:07]

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (left):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:354: void
UiScene::ApplyRecursiveTransforms(const ContentRectangle& element,
On 2017/01/11 18:33:04, mthiesse wrote:
> Unrelated, but can you add a TODO to be smarter about how we compute these
> transforms? We could theoretically be doing a single pass.

Agreed.  There are multiple bits to this though:
1. Don't recompute parent transforms multiple times in the render loop.
2. Don't recompute transforms (other than pose-related) for elements that
haven't changed (animation or explicit update).

In lieu of shuffling the whole implementation to a tree structure, we could put
a dirty bit on the computed transform, and keep the recursive approach.  Dirty
bits would be cleared once per render cycle.  That addresses (1).  To achieve
(2), I think we need element changes to clear dirty bits of child elements as
well, and currently parents aren't aware of their kids (sounds blissful).

Thoughts?

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.h:75: float *opacity);
On 2017/01/11 18:33:04, mthiesse wrote:
> nit: float*

Life would be boring if I stopped messing this up.

[mthiesse, 2017-01-11 19:02:49]

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (left):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:354: void
UiScene::ApplyRecursiveTransforms(const ContentRectangle& element,
On 2017/01/11 18:53:07, cjgrant wrote:
> On 2017/01/11 18:33:04, mthiesse wrote:
> > Unrelated, but can you add a TODO to be smarter about how we compute these
> > transforms? We could theoretically be doing a single pass.
> 
> Agreed.  There are multiple bits to this though:
> 1. Don't recompute parent transforms multiple times in the render loop.
> 2. Don't recompute transforms (other than pose-related) for elements that
> haven't changed (animation or explicit update).
> 
> In lieu of shuffling the whole implementation to a tree structure, we could
put
> a dirty bit on the computed transform, and keep the recursive approach.  Dirty
> bits would be cleared once per render cycle.  That addresses (1).  To achieve
> (2), I think we need element changes to clear dirty bits of child elements as
> well, and currently parents aren't aware of their kids (sounds blissful).
> 
> Thoughts?

Speaking as someone who's never had children, a dirty bit sounds like a good
idea.

[bshe, 2017-01-11 20:19:25]

On 2017/01/11 19:02:49, mthiesse wrote:
>
https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/ui_scene.cc (left):
> 
>
https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/ui_scene.cc:354: void
> UiScene::ApplyRecursiveTransforms(const ContentRectangle& element,
> On 2017/01/11 18:53:07, cjgrant wrote:
> > On 2017/01/11 18:33:04, mthiesse wrote:
> > > Unrelated, but can you add a TODO to be smarter about how we compute these
> > > transforms? We could theoretically be doing a single pass.
> > 
> > Agreed.  There are multiple bits to this though:
> > 1. Don't recompute parent transforms multiple times in the render loop.
> > 2. Don't recompute transforms (other than pose-related) for elements that
> > haven't changed (animation or explicit update).
> > 
> > In lieu of shuffling the whole implementation to a tree structure, we could
> put
> > a dirty bit on the computed transform, and keep the recursive approach. 
Dirty
> > bits would be cleared once per render cycle.  That addresses (1).  To
achieve
> > (2), I think we need element changes to clear dirty bits of child elements
as
> > well, and currently parents aren't aware of their kids (sounds blissful).
> > 
> > Thoughts?
> 
> Speaking as someone who's never had children, a dirty bit sounds like a good
> idea.

lgtm

[cjgrant, 2017-01-11 20:58:42]

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (left):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:354: void
UiScene::ApplyRecursiveTransforms(const ContentRectangle& element,
On 2017/01/11 19:02:49, mthiesse wrote:
> On 2017/01/11 18:53:07, cjgrant wrote:
> > On 2017/01/11 18:33:04, mthiesse wrote:
> > > Unrelated, but can you add a TODO to be smarter about how we compute these
> > > transforms? We could theoretically be doing a single pass.
> > 
> > Agreed.  There are multiple bits to this though:
> > 1. Don't recompute parent transforms multiple times in the render loop.
> > 2. Don't recompute transforms (other than pose-related) for elements that
> > haven't changed (animation or explicit update).
> > 
> > In lieu of shuffling the whole implementation to a tree structure, we could
> put
> > a dirty bit on the computed transform, and keep the recursive approach. 
Dirty
> > bits would be cleared once per render cycle.  That addresses (1).  To
achieve
> > (2), I think we need element changes to clear dirty bits of child elements
as
> > well, and currently parents aren't aware of their kids (sounds blissful).
> > 
> > Thoughts?
> 
> Speaking as someone who's never had children, a dirty bit sounds like a good
> idea.

Done.  Filed crbug/680253.

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.cc (right):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.cc:361: float *opacity) {
On 2017/01/11 18:33:04, mthiesse wrote:
> float*

Done.

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene.h:75: float *opacity);
On 2017/01/11 18:53:07, cjgrant wrote:
> On 2017/01/11 18:33:04, mthiesse wrote:
> > nit: float*
> 
> Life would be boring if I stopped messing this up.

Done.

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2624243002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:490: if (!plane->visible ||
plane->computed_opacity == 0.0f ||
On 2017/01/11 18:33:04, mthiesse wrote:
> On 2017/01/11 16:45:47, cjgrant wrote:
> > Checking opacity here relieves the HTML UI of having to manage both
visibility
> > and opacity (it can use one or the other).  I stopped short of removing the
> > visibility toggle though, because in most cases, the toggle is more
analogous
> to
> > "enabled".
> 
> Maybe add a function IsVisible() to ContentRectangle?

Done.

[cjgrant, 2017-01-11 21:03:43]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-01-11 21:03:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2624243002/#ps20001
(title: "for (auto nit : nits) nit.fix();")

[commit-bot: I haz the power, 2017-01-11 21:04:23]

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-01-11 22:54:10]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484168623000800,
"parent_rev": "dd0efaefeb00163421f31ed74880d0d9a5c5b241", "commit_rev":
"eebed48cece6143e1392afe5cd1bc25db67779e8"}

[commit-bot: I haz the power, 2017-01-11 22:54:41]

Description was changed from

==========
VrShell: Allow native control of UI element opacity.

- Add support to ContentRectangle.
- Inherit opacity from parent elements.
- Update tests.
- Drive omnibox opacity animations natively rather than via CSS.

BUG=679761
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
VrShell: Allow native control of UI element opacity.

- Add support to ContentRectangle.
- Inherit opacity from parent elements.
- Update tests.
- Drive omnibox opacity animations natively rather than via CSS.

BUG=679761
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-01-11 22:54:43]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/eebed48cece6143e1392afe5cd1b...