Use the HTML UI to render WebVR warnings

Use the HTML UI to render WebVR warnings

This pulls custom warning rendering code out of the native side, and
uses the HTML UI logic to drive it instead.

BUG=641508
Committed: https://crrev.com/cda687ffaf139a991a8a74728b34a36a28e529f6
Cr-Commit-Position: refs/heads/master@{#427484}

[cjgrant, 2016-10-19 20:12:51]

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

[cjgrant, 2016-10-19 20:12:52]

Michael, feel free to ignore this change for now.  I'm not 100% confident of the
GL steps.  Posting so you can see what's coming next...

[mthiesse, 2016-10-21 14:14:50]

lgtm

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

https://codereview.chromium.org/2436863002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:490: if (webvr_mode_ && rect->id ==
kBrowserUiElementId) {
Technically we could still control this through javascript and not special case
it here.

[cjgrant, 2016-10-24 19:59:18]

Description was changed from

==========
Use the HTML UI to render WebVR warnings

This pulls custom warning rendering code out of the native side, and
uses the HTML UI logic to drive it instead.

BUG=641508
==========

to

==========
Use the HTML UI to render WebVR warnings

This pulls custom warning rendering code out of the native side, and
uses the HTML UI logic to drive it instead.

BUG=641508
==========

[cjgrant, 2016-10-24 19:59:18]

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

[cjgrant, 2016-10-24 20:00:19]

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

https://codereview.chromium.org/2436863002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:490: if (webvr_mode_ && rect->id ==
kBrowserUiElementId) {
On 2016/10/21 14:14:50, mthiesse wrote:
> Technically we could still control this through javascript and not special
case
> it here.

This decoupling is a bit involved, and hence is up as a follow-on change:
https://codereview.chromium.org/2442873002/

[cjgrant, 2016-10-24 20:05:09]

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:471: glEnable(GL_CULL_FACE);
Brandon, in this CL, I try to leave the sequence of GL-related calls untouched
in the webVR and 2D cases.  However, in a follow-on change, I'd like to refactor
this such that the GL calls are as similar as possible.

I don't know enough GL yet, but I'd be starting with:
- Removing some of the "disable everything" calls in the WebVR case
- Figuring out how to clear the buffer before doing any rendering, instead of
clearing one eye at a time as we do in the VrShell rendering.

If this stuff is trivially simple to you, can we chat offline about it?

[bajones, 2016-10-24 21:12:27]

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:446:
frame.BindBuffer(kFrameHeadlockedBuffer);
I don't know how much of a perf hit this introduces, but it seems like always
compositing in the headlocked layer is wasted cycles? Is there a reasonable way
to determine if the HTML UI has anything to be drawn?

Also, I don't think that we'll always want all parts of the HTML UI to be
headlocked when in WebVR (the omnibar is a good example) so I'd rather see this
viewport setting moved down into DrawVRShell and only used for elements
explicitly marked as headlocked.

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:471: glEnable(GL_CULL_FACE);
On 2016/10/24 20:05:09, cjgrant wrote:
> Brandon, in this CL, I try to leave the sequence of GL-related calls untouched
> in the webVR and 2D cases.  However, in a follow-on change, I'd like to
refactor
> this such that the GL calls are as similar as possible.
> 
> I don't know enough GL yet, but I'd be starting with:
> - Removing some of the "disable everything" calls in the WebVR case
> - Figuring out how to clear the buffer before doing any rendering, instead of
> clearing one eye at a time as we do in the VrShell rendering.
> 
> If this stuff is trivially simple to you, can we chat offline about it?

Sure, let's grab a moment to chat. Stick something on my calendar.

[cjgrant, 2016-10-24 21:34:57]

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:446:
frame.BindBuffer(kFrameHeadlockedBuffer);
On 2016/10/24 21:12:27, bajones wrote:
> I don't know how much of a perf hit this introduces, but it seems like always
> compositing in the headlocked layer is wasted cycles? Is there a reasonable
way
> to determine if the HTML UI has anything to be drawn?
> 
> Also, I don't think that we'll always want all parts of the HTML UI to be
> headlocked when in WebVR (the omnibar is a good example) so I'd rather see
this
> viewport setting moved down into DrawVRShell and only used for elements
> explicitly marked as headlocked.

On determining if there's anything to draw:  Yes, that's easy.  I skipped it to
keep the "worst case" visible, rather than degrading when a UI element appears,
but I'll switch this up.

On non-headlocked elements, sure, lets do this now.  I assume you're thinking
this:
- Enumerate UI elements, draw any that aren't headlocked.
- If any headlocked elements:
  - Bind the headlocked buffer
  - Enumerate again, draw any that are headlocked.

[bajones, 2016-10-24 21:43:21]

On 2016/10/24 21:34:57, cjgrant wrote:
> On determining if there's anything to draw:  Yes, that's easy.  I skipped it
to
> keep the "worst case" visible, rather than degrading when a UI element
appears,
> but I'll switch this up.

For this exact use case it shouldn't be too bad. Sites are never going to
magically become HTTP in the middle of an HTTPS session, so if there is a perf
difference it will only manifest between sites. Makes sense to get the extra
perf when we can, in that case.

> On non-headlocked elements, sure, lets do this now.  I assume you're thinking
> this:
> - Enumerate UI elements, draw any that aren't headlocked.
> - If any headlocked elements:
>   - Bind the headlocked buffer
>   - Enumerate again, draw any that are headlocked.

Precisely.

[cjgrant, 2016-10-25 20:09:07]

Brandon, PTAL when you can.

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2436863002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:446:
frame.BindBuffer(kFrameHeadlockedBuffer);
On 2016/10/24 21:34:57, cjgrant wrote:
> On 2016/10/24 21:12:27, bajones wrote:
> > I don't know how much of a perf hit this introduces, but it seems like
always
> > compositing in the headlocked layer is wasted cycles? Is there a reasonable
> way
> > to determine if the HTML UI has anything to be drawn?
> > 
> > Also, I don't think that we'll always want all parts of the HTML UI to be
> > headlocked when in WebVR (the omnibar is a good example) so I'd rather see
> this
> > viewport setting moved down into DrawVRShell and only used for elements
> > explicitly marked as headlocked.
> 
> On determining if there's anything to draw:  Yes, that's easy.  I skipped it
to
> keep the "worst case" visible, rather than degrading when a UI element
appears,
> but I'll switch this up.
> 
> On non-headlocked elements, sure, lets do this now.  I assume you're thinking
> this:
> - Enumerate UI elements, draw any that aren't headlocked.
> - If any headlocked elements:
>   - Bind the headlocked buffer
>   - Enumerate again, draw any that are headlocked.

So this refactor was a little bigger than I expected, but I think we're better
off now.

[bajones, 2016-10-25 20:22:17]

On 2016/10/25 20:09:07, cjgrant wrote:
> So this refactor was a little bigger than I expected, but I think we're better
> off now.

Totally agree. This feels robust and flexible, and is a bit easier to follow to
boot (IMHO).

LGTM!

[cjgrant, 2016-10-25 21:07:58]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2016-10-25 21:07:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2436863002/#ps80001
(title: "Fix whitespace.")

[commit-bot: I haz the power, 2016-10-25 21:08:26]

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, 2016-10-25 21:35:18]

Description was changed from

==========
Use the HTML UI to render WebVR warnings

This pulls custom warning rendering code out of the native side, and
uses the HTML UI logic to drive it instead.

BUG=641508
==========

to

==========
Use the HTML UI to render WebVR warnings

This pulls custom warning rendering code out of the native side, and
uses the HTML UI logic to drive it instead.

BUG=641508
==========

[commit-bot: I haz the power, 2016-10-25 21:35:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-25 21:38:28]

Description was changed from

==========
Use the HTML UI to render WebVR warnings

This pulls custom warning rendering code out of the native side, and
uses the HTML UI logic to drive it instead.

BUG=641508
==========

to

==========
Use the HTML UI to render WebVR warnings

This pulls custom warning rendering code out of the native side, and
uses the HTML UI logic to drive it instead.

BUG=641508

Committed: https://crrev.com/cda687ffaf139a991a8a74728b34a36a28e529f6
Cr-Commit-Position: refs/heads/master@{#427484}
==========

[commit-bot: I haz the power, 2016-10-25 21:38:28]

Patchset 5 (id:??) landed as
https://crrev.com/cda687ffaf139a991a8a74728b34a36a28e529f6
Cr-Commit-Position: refs/heads/master@{#427484}