WebVR: Ensure loading indicator starts hidden.

WebVR: Ensure loading indicator starts hidden.

The element started out with enabled=false and default visible=true,
the visibility was intended to be set to false on initializing,
but that setting was skipped since the new enabled state was equal
to the starting enabled state.

To fix this, explicitly set the starting visibility to false.

BUG=723959
Review-Url: https://codereview.chromium.org/2893853002
Cr-Commit-Position: refs/heads/master@{#472878}
Committed: https://chromium.googlesource.com/chromium/src/+/66af134f170e3f6f217c826ede85791151011147

[klausw, 2017-05-18 05:25:58]

klausw@chromium.org changed reviewers:
+ cjgrant@chromium.org

[klausw, 2017-05-18 05:26:10]

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

[commit-bot: I haz the power, 2017-05-18 05:27:20]

Dry run: CQ is trying da patch.

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

[ddorwin, 2017-05-18 06:05:10]

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

[ddorwin, 2017-05-18 06:05:11]

While this fixes this specific instance, the system seems fragile. Should we
default visible_ to false in UiElement?

Looking closer, however, the problem is with the logic in SetEnabled(), which
this class overrides. Overriding such a method in a base class is a bit
concerning and may be the real issue. Note also that enabled_ is defined in this
class. (The base class sets visible_ when this method is called.)

[ddorwin, 2017-05-18 06:11:12]

On 2017/05/18 06:05:11, ddorwin wrote:
> While this fixes this specific instance, the system seems fragile. Should we
> default visible_ to false in UiElement?
> 
> Looking closer, however, the problem is with the logic in SetEnabled(), which
> this class overrides. Overriding such a method in a base class is a bit
> concerning and may be the real issue. Note also that enabled_ is defined in
this
> class. (The base class sets visible_ when this method is called.)

This fixes the immediate issue and the behavior is no worse, so it's fine to
land this. However, I think we should consider refactoring this code in a
separate CL. The comments and questions above are for cjgrant.

[commit-bot: I haz the power, 2017-05-18 06:12:11]

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

[commit-bot: I haz the power, 2017-05-18 06:12:12]

Dry run: This issue passed the CQ dry run.

[amp, 2017-05-18 16:24:30]

On 2017/05/18 06:11:12, ddorwin wrote:
> On 2017/05/18 06:05:11, ddorwin wrote:
> > While this fixes this specific instance, the system seems fragile. Should we
> > default visible_ to false in UiElement?
> > 
> > Looking closer, however, the problem is with the logic in SetEnabled(),
which
> > this class overrides. Overriding such a method in a base class is a bit
> > concerning and may be the real issue. Note also that enabled_ is defined in
> this
> > class. (The base class sets visible_ when this method is called.)
> 
> This fixes the immediate issue and the behavior is no worse, so it's fine to
> land this. However, I think we should consider refactoring this code in a
> separate CL. The comments and questions above are for cjgrant.

+1 to making the base case not visible.

After element creation we always configure the scene for the current mode so
everything should be set to the correct visibility before entering VrShell.

[cjgrant, 2017-05-18 17:00:03]

On 2017/05/18 16:24:30, amp wrote:
> On 2017/05/18 06:11:12, ddorwin wrote:
> > On 2017/05/18 06:05:11, ddorwin wrote:
> > > While this fixes this specific instance, the system seems fragile. Should
we
> > > default visible_ to false in UiElement?
> > > 
> > > Looking closer, however, the problem is with the logic in SetEnabled(),
> which
> > > this class overrides. Overriding such a method in a base class is a bit
> > > concerning and may be the real issue. Note also that enabled_ is defined
in
> > this
> > > class. (The base class sets visible_ when this method is called.)
> > 
> > This fixes the immediate issue and the behavior is no worse, so it's fine to
> > land this. However, I think we should consider refactoring this code in a
> > separate CL. The comments and questions above are for cjgrant.
> 
> +1 to making the base case not visible.
> 
> After element creation we always configure the scene for the current mode so
> everything should be set to the correct visibility before entering VrShell.

+1 on defaulting to invisible.  Ancient history is the only reason it's not this
way already.

Why not do that in this CL as well, as it's true that we explicitly set all
initial states now?

David, I'm not clear on the refactoring you're suggesting.  Naming aside,
"Enabled" means "should be active in the current scene".  For most elements,
that means visible.  The URL bar is transient, and hence needs to set its own
visibility based on whether it's "enabled" and loading is currently happening. 
The fact that it's overriding SetEnabled, but not initializing its visible
status, is certainly a problem. :)

[klausw, 2017-05-18 17:29:45]

On 2017/05/18 17:00:03, cjgrant wrote:
> On 2017/05/18 16:24:30, amp wrote:
> > On 2017/05/18 06:11:12, ddorwin wrote:
> > > On 2017/05/18 06:05:11, ddorwin wrote:
> > > > While this fixes this specific instance, the system seems fragile.
Should
> we
> > > > default visible_ to false in UiElement?
> > > > 
> > > > Looking closer, however, the problem is with the logic in SetEnabled(),
> > which
> > > > this class overrides. Overriding such a method in a base class is a bit
> > > > concerning and may be the real issue. Note also that enabled_ is defined
> in
> > > this
> > > > class. (The base class sets visible_ when this method is called.)
> > > 
> > > This fixes the immediate issue and the behavior is no worse, so it's fine
to
> > > land this. However, I think we should consider refactoring this code in a
> > > separate CL. The comments and questions above are for cjgrant.
> > 
> > +1 to making the base case not visible.
> > 
> > After element creation we always configure the scene for the current mode so
> > everything should be set to the correct visibility before entering VrShell.
> 
> +1 on defaulting to invisible.  Ancient history is the only reason it's not
this
> way already.
> 
> Why not do that in this CL as well, as it's true that we explicitly set all
> initial states now?
> 
> David, I'm not clear on the refactoring you're suggesting.  Naming aside,
> "Enabled" means "should be active in the current scene".  For most elements,
> that means visible.  The URL bar is transient, and hence needs to set its own
> visibility based on whether it's "enabled" and loading is currently happening.

> The fact that it's overriding SetEnabled, but not initializing its visible
> status, is certainly a problem. :)

Is everyone ok with submitting this patch as-is to solve the immediate issue? I
don't have an LGTM yet. 

FYI, based on unscientific event timing, it seemed as if having the URL bar
active caused UI drawing to take up to 2ms per frame wallclock time within
WebVR, this dropped to <0.05ms after setting it not visible. That seems
excessive for a single small quad, can we keep an eye on this to see if there's
a performance bug related to UI drawing? It may have been a temporary glitch.

[cjgrant, 2017-05-18 17:53:12]

On 2017/05/18 17:29:45, klausw wrote:
> On 2017/05/18 17:00:03, cjgrant wrote:
> > On 2017/05/18 16:24:30, amp wrote:
> > > On 2017/05/18 06:11:12, ddorwin wrote:
> > > > On 2017/05/18 06:05:11, ddorwin wrote:
> > > > > While this fixes this specific instance, the system seems fragile.
> Should
> > we
> > > > > default visible_ to false in UiElement?
> > > > > 
> > > > > Looking closer, however, the problem is with the logic in
SetEnabled(),
> > > which
> > > > > this class overrides. Overriding such a method in a base class is a
bit
> > > > > concerning and may be the real issue. Note also that enabled_ is
defined
> > in
> > > > this
> > > > > class. (The base class sets visible_ when this method is called.)
> > > > 
> > > > This fixes the immediate issue and the behavior is no worse, so it's
fine
> to
> > > > land this. However, I think we should consider refactoring this code in
a
> > > > separate CL. The comments and questions above are for cjgrant.
> > > 
> > > +1 to making the base case not visible.
> > > 
> > > After element creation we always configure the scene for the current mode
so
> > > everything should be set to the correct visibility before entering
VrShell.
> > 
> > +1 on defaulting to invisible.  Ancient history is the only reason it's not
> this
> > way already.
> > 
> > Why not do that in this CL as well, as it's true that we explicitly set all
> > initial states now?
> > 
> > David, I'm not clear on the refactoring you're suggesting.  Naming aside,
> > "Enabled" means "should be active in the current scene".  For most elements,
> > that means visible.  The URL bar is transient, and hence needs to set its
own
> > visibility based on whether it's "enabled" and loading is currently
happening.
> 
> > The fact that it's overriding SetEnabled, but not initializing its visible
> > status, is certainly a problem. :)
> 
> Is everyone ok with submitting this patch as-is to solve the immediate issue?
I
> don't have an LGTM yet. 
> 
> FYI, based on unscientific event timing, it seemed as if having the URL bar
> active caused UI drawing to take up to 2ms per frame wallclock time within
> WebVR, this dropped to <0.05ms after setting it not visible. That seems
> excessive for a single small quad, can we keep an eye on this to see if
there's
> a performance bug related to UI drawing? It may have been a temporary glitch.

I'll do the "visible_ = false" default in a separate change.

On the performance, yeah, that's not good.  We've had a lot of churn on texture
generation lately - I'll check whether that texture is possibly being
regenerated when it shouldn't be.  But you're saying "within WebVR", which means
it should not be visible.  Did you mean outside WebVR?  Feel free to message
offline about this.

[cjgrant, 2017-05-18 17:53:20]

lgtm

[klausw, 2017-05-18 18:10:55]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2017-05-18 18:11: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-18 18:19:17]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1495131055306210, "parent_rev":
"6eff4f61a15bfee8451bf106d7b74b99f6b4264c", "commit_rev":
"66af134f170e3f6f217c826ede85791151011147"}

[commit-bot: I haz the power, 2017-05-18 18:19:25]

Description was changed from

==========
WebVR: Ensure loading indicator starts hidden.

The element started out with enabled=false and default visible=true,
the visibility was intended to be set to false on initializing,
but that setting was skipped since the new enabled state was equal
to the starting enabled state.

To fix this, explicitly set the starting visibility to false.

BUG=723959
==========

to

==========
WebVR: Ensure loading indicator starts hidden.

The element started out with enabled=false and default visible=true,
the visibility was intended to be set to false on initializing,
but that setting was skipped since the new enabled state was equal
to the starting enabled state.

To fix this, explicitly set the starting visibility to false.

BUG=723959

Review-Url: https://codereview.chromium.org/2893853002
Cr-Commit-Position: refs/heads/master@{#472878}
Committed:
https://chromium.googlesource.com/chromium/src/+/66af134f170e3f6f217c826ede85...
==========

[commit-bot: I haz the power, 2017-05-18 18:19:26]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/66af134f170e3f6f217c826ede85...