VR: Disable Overscroll Glow in VrShell

VR: Disable Overscroll Glow in VrShell

Disable the overscroll glow in VrShell. The overscroll controller is still working,
but the animation is disabled when the user is in VR.

BUG=660051
Committed: https://crrev.com/54ea8b659913ce3528c53c6bd675c332d8189796
Cr-Commit-Position: refs/heads/master@{#437282}

[asimjour, 2016-11-24 16:29:15]

Description was changed from

==========
VR: Disable Overscroll Glow in VrShell

Disable the overscroll glow in VrShell. The overscroll controller is still
working,
but the animation is disabled when the user is in VR.

BUG=660051
==========

to

==========
VR: Disable Overscroll Glow in VrShell

Disable the overscroll glow in VrShell. The overscroll controller is still
working,
but the animation is disabled when the user is in VR.

BUG=660051
==========

[asimjour, 2016-11-24 16:29:15]

asimjour@google.com changed reviewers:
+ bshe@chromium.org

[mthiesse, 2016-11-24 16:45:06]

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

[mthiesse, 2016-11-24 16:45:07]

https://codereview.chromium.org/2524423002/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2524423002/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:67: // Tells the viewer whether
the overscroll glow is showing.
nit: Maybe "Tells the renderer whether to show the overscroll glow"

[asimjour1, 2016-11-24 16:55:40]

asimjour@chromium.org changed reviewers:
+ sadrul@chromium.org

[asimjour1, 2016-11-24 16:55:56]

PTAL

https://codereview.chromium.org/2524423002/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2524423002/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host_view.h:67: // Tells the viewer whether
the overscroll glow is showing.
On 2016/11/24 16:45:07, mthiesse wrote:
> nit: Maybe "Tells the renderer whether to show the overscroll glow"

Done.

[bshe, 2016-11-25 14:55:00]

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

https://codereview.chromium.org/2524423002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:239:
SetShowingOverscrollGlow(false);
I am assuming this is the place that calling SetShowingOverscrollGlow on render
thread? Is it possible to move it to ctor of VrShell. IIUC, VrShell is created
on UI thread. This way, you don't need the thread check in the function
implementation?

https://codereview.chromium.org/2524423002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2524423002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:226: void
SetShowingOverscrollGlow(bool showing_glow) override;
nit: Move this to
// RenderWidgetHostView implementation.
section

[asimjour1, 2016-12-01 20:17:45]

PTAL

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

https://codereview.chromium.org/2524423002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:239:
SetShowingOverscrollGlow(false);
On 2016/11/25 14:55:00, bshe wrote:
> I am assuming this is the place that calling SetShowingOverscrollGlow on
render
> thread? Is it possible to move it to ctor of VrShell. IIUC, VrShell is created
> on UI thread. This way, you don't need the thread check in the function
> implementation?

ctor of VrShell is called on UI thread, but RenderViewHost is not ready at that
time.

https://codereview.chromium.org/2524423002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2524423002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:226: void
SetShowingOverscrollGlow(bool showing_glow) override;
On 2016/11/25 14:55:00, bshe wrote:
> nit: Move this to
> // RenderWidgetHostView implementation.
> section

Done.

[bshe, 2016-12-01 20:43:42]

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

https://codereview.chromium.org/2524423002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:239:
SetShowingOverscrollGlow(false);
On 2016/12/01 20:17:45, asimjour1 wrote:
> On 2016/11/25 14:55:00, bshe wrote:
> > I am assuming this is the place that calling SetShowingOverscrollGlow on
> render
> > thread? Is it possible to move it to ctor of VrShell. IIUC, VrShell is
created
> > on UI thread. This way, you don't need the thread check in the function
> > implementation?
> 
> ctor of VrShell is called on UI thread, but RenderViewHost is not ready at
that
> time.

Is it possible to call it once RVH is ready on UI thread?
Would be nice to do everything on a single thread.

[mthiesse, 2016-12-01 22:15:39]

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

https://codereview.chromium.org/2524423002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:239:
SetShowingOverscrollGlow(false);
On 2016/12/01 20:43:41, bshe wrote:
> On 2016/12/01 20:17:45, asimjour1 wrote:
> > On 2016/11/25 14:55:00, bshe wrote:
> > > I am assuming this is the place that calling SetShowingOverscrollGlow on
> > render
> > > thread? Is it possible to move it to ctor of VrShell. IIUC, VrShell is
> created
> > > on UI thread. This way, you don't need the thread check in the function
> > > implementation?
> > 
> > ctor of VrShell is called on UI thread, but RenderViewHost is not ready at
> that
> > time.
> 
> Is it possible to call it once RVH is ready on UI thread?
> Would be nice to do everything on a single thread.

Yes, please add RenderViewHostChanged to vr_web_contents_observer, and call
SetShowingOverscrollGlow from there.

[asimjour1, 2016-12-05 18:57:08]

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

https://codereview.chromium.org/2524423002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:239:
SetShowingOverscrollGlow(false);
On 2016/12/01 22:15:39, mthiesse wrote:
> On 2016/12/01 20:43:41, bshe wrote:
> > On 2016/12/01 20:17:45, asimjour1 wrote:
> > > On 2016/11/25 14:55:00, bshe wrote:
> > > > I am assuming this is the place that calling SetShowingOverscrollGlow on
> > > render
> > > > thread? Is it possible to move it to ctor of VrShell. IIUC, VrShell is
> > created
> > > > on UI thread. This way, you don't need the thread check in the function
> > > > implementation?
> > > 
> > > ctor of VrShell is called on UI thread, but RenderViewHost is not ready at
> > that
> > > time.
> > 
> > Is it possible to call it once RVH is ready on UI thread?
> > Would be nice to do everything on a single thread.
> 
> Yes, please add RenderViewHostChanged to vr_web_contents_observer, and call
> SetShowingOverscrollGlow from there.

Done.

https://codereview.chromium.org/2524423002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:239:
SetShowingOverscrollGlow(false);
On 2016/12/01 20:43:41, bshe wrote:
> On 2016/12/01 20:17:45, asimjour1 wrote:
> > On 2016/11/25 14:55:00, bshe wrote:
> > > I am assuming this is the place that calling SetShowingOverscrollGlow on
> > render
> > > thread? Is it possible to move it to ctor of VrShell. IIUC, VrShell is
> created
> > > on UI thread. This way, you don't need the thread check in the function
> > > implementation?
> > 
> > ctor of VrShell is called on UI thread, but RenderViewHost is not ready at
> that
> > time.
> 
> Is it possible to call it once RVH is ready on UI thread?
> Would be nice to do everything on a single thread.

Done.

[asimjour1, 2016-12-05 18:57:41]

asimjour@chromium.org changed reviewers:
+ creis@chromium.org

[mthiesse, 2016-12-05 19:02:30]

lgtm

https://codereview.chromium.org/2524423002/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right):

https://codereview.chromium.org/2524423002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_web_contents_observer.cc:68:
new_host->GetWidget()->GetView()->SetShowingOverscrollGlow(false);
Should we check for NULL first? Or is that not possible?

[bshe, 2016-12-05 19:25:56]

lgtm with one nit

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

https://codereview.chromium.org/2524423002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:19: #include
"content/public/browser/browser_thread.h"
nit: this include isn't necessary anymore

[asimjour1, 2016-12-05 19:37:31]

@creis, please take a look at content/*

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

https://codereview.chromium.org/2524423002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:19: #include
"content/public/browser/browser_thread.h"
On 2016/12/05 19:25:56, bshe wrote:
> nit: this include isn't necessary anymore

Done.

https://codereview.chromium.org/2524423002/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right):

https://codereview.chromium.org/2524423002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_web_contents_observer.cc:68:
new_host->GetWidget()->GetView()->SetShowingOverscrollGlow(false);
On 2016/12/05 19:02:30, mthiesse wrote:
> Should we check for NULL first? Or is that not possible?

We don't need to check. old_host could be null if the old RVH was shut down, but
new_host is supposed to be valid.

[sadrul, 2016-12-06 20:29:23]

https://codereview.chromium.org/2524423002/diff/80001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2524423002/diff/80001/content/public/browser/...
content/public/browser/render_widget_host_view.h:68: virtual void
SetShowingOverscrollGlow(bool showing) = 0;
Note that there's a WebContentsDelegate::CanOverscrollContent(). It may be
possible to re-use that for this too?

[Charlie Reis, 2016-12-06 22:51:42]

content/ LGTM.

https://codereview.chromium.org/2524423002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right):

https://codereview.chromium.org/2524423002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_web_contents_observer.cc:65: void
VrWebContentsObserver::RenderViewHostChanged(
Note that RenderViewHost is on its way out, in favor of RenderFrameHost.  I
think the version you have should be fine functionally, assuming overscroll only
happens for main frame navigations.  (It's probably also safe to use
RenderFrameHostChanged and check if it's the main frame, but we can do that
later.)

[asimjour1, 2016-12-08 16:59:15]

https://codereview.chromium.org/2524423002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right):

https://codereview.chromium.org/2524423002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_web_contents_observer.cc:65: void
VrWebContentsObserver::RenderViewHostChanged(
On 2016/12/06 22:51:42, Charlie Reis (OOO) wrote:
> Note that RenderViewHost is on its way out, in favor of RenderFrameHost.  I
> think the version you have should be fine functionally, assuming overscroll
only
> happens for main frame navigations.  (It's probably also safe to use
> RenderFrameHostChanged and check if it's the main frame, but we can do that
> later.)

Overscroll only happens for the main frame navigation. I'll use
RenderFrameHostChanged in another patch. Thank you

https://codereview.chromium.org/2524423002/diff/80001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2524423002/diff/80001/content/public/browser/...
content/public/browser/render_widget_host_view.h:68: virtual void
SetShowingOverscrollGlow(bool showing) = 0;
On 2016/12/06 20:29:23, sadrul wrote:
> Note that there's a WebContentsDelegate::CanOverscrollContent(). It may be
> possible to re-use that for this too?

We can't disable the glow using WebContentsDelegate::CanOverscrollContent().

[asimjour1, 2016-12-08 17:01:21]

The CQ bit was checked by asimjour@chromium.org

[asimjour1, 2016-12-08 17:01:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bshe@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2524423002/#ps100001
(title: "Rebase + Rename VrShell::SetShowingOverscrollGlow")

[commit-bot: I haz the power, 2016-12-08 17:02:00]

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-12-08 17:58:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481216481234660,
"parent_rev": "fc6ba320990c82fe6c3350c742dd049d83a46cb2", "commit_rev":
"8065ed17ea4802b916c4b43615945a303f3d7512"}

[commit-bot: I haz the power, 2016-12-08 17:59:03]

Description was changed from

==========
VR: Disable Overscroll Glow in VrShell

Disable the overscroll glow in VrShell. The overscroll controller is still
working,
but the animation is disabled when the user is in VR.

BUG=660051
==========

to

==========
VR: Disable Overscroll Glow in VrShell

Disable the overscroll glow in VrShell. The overscroll controller is still
working,
but the animation is disabled when the user is in VR.

BUG=660051
==========

[commit-bot: I haz the power, 2016-12-08 17:59:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-08 18:03:25]

Description was changed from

==========
VR: Disable Overscroll Glow in VrShell

Disable the overscroll glow in VrShell. The overscroll controller is still
working,
but the animation is disabled when the user is in VR.

BUG=660051
==========

to

==========
VR: Disable Overscroll Glow in VrShell

Disable the overscroll glow in VrShell. The overscroll controller is still
working,
but the animation is disabled when the user is in VR.

BUG=660051

Committed: https://crrev.com/54ea8b659913ce3528c53c6bd675c332d8189796
Cr-Commit-Position: refs/heads/master@{#437282}
==========

[commit-bot: I haz the power, 2016-12-08 18:03:27]

Patchset 6 (id:??) landed as
https://crrev.com/54ea8b659913ce3528c53c6bd675c332d8189796
Cr-Commit-Position: refs/heads/master@{#437282}

[rlanday, 2016-12-08 19:06:24]

Quick question for a refactor I'm working on
(https://codereview.chromium.org/2528823002/): is it intentional that you reset
is_showing_overscroll_glow_ to true when SetContentViewCore() is called but not
when OnAttachCompositor() is called? Both of these methods can potentially call
CreateOverscrollController().

[boliu, 2016-12-08 19:13:30]

On 2016/12/08 19:06:24, rlanday wrote:
> Quick question for a refactor I'm working on
> (https://codereview.chromium.org/2528823002/): is it intentional that you
reset
> is_showing_overscroll_glow_ to true when SetContentViewCore() is called but
not
> when OnAttachCompositor() is called? Both of these methods can potentially
call
> CreateOverscrollController().

Also.. does RenderViewHostChanged cover interstitials? I think the setting
should be on webcontents instead.

[asimjour1, 2016-12-21 16:15:36]

Patchset #7 (id:120001) has been deleted