[vr] Introduce VrTabHelper (issue 2874623004 by vollick@chromium.org)

[vol...@chromium.org]

Reviewers: Ted C
CL: https://codereview.chromium.org/2874623004/

Message:
Hey Ted. Is this heading in the direction you'd suggested?

Description:
[vr] Introduce VrTabHelper

This is currently a boolean holder, but will eventually
be responsible for both setting per-dialog suppression
flags on the WebContents and informing VrShell when
suppression notifications need to shown to the user.

BUG=688122

Affected files (+68, -1 lines):
M chrome/browser/android/vr_shell/vr_shell.cc
A chrome/browser/android/vr_shell/vr_tab_helper.h
A chrome/browser/android/vr_shell/vr_tab_helper.cc
M chrome/browser/ui/tab_helpers.cc


Index: chrome/browser/android/vr_shell/vr_shell.cc
diff --git a/chrome/browser/android/vr_shell/vr_shell.cc b/chrome/browser/android/vr_shell/vr_shell.cc
index 5d7b6bbb6ea9851693c9ed9fecc6710b1cadaaee..ae9d4b40bf2913e6c306e89801c8e24b78f2e48a 100644
--- a/chrome/browser/android/vr_shell/vr_shell.cc
+++ b/chrome/browser/android/vr_shell/vr_shell.cc
@@ -28,6 +28,7 @@
#include "chrome/browser/android/vr_shell/vr_input_manager.h"
#include "chrome/browser/android/vr_shell/vr_shell_delegate.h"
#include "chrome/browser/android/vr_shell/vr_shell_gl.h"
+#include "chrome/browser/android/vr_shell/vr_tab_helper.h"
#include "chrome/browser/android/vr_shell/vr_usage_monitor.h"
#include "chrome/browser/android/vr_shell/vr_web_contents_observer.h"
#include "content/public/browser/browser_thread.h"
@@ -62,8 +63,16 @@ namespace {
vr_shell::VrShell* g_instance;

void SetIsInVR(content::WebContents* contents, bool is_in_vr) {
- if (contents && contents->GetRenderWidgetHostView())
+ if (contents && contents->GetRenderWidgetHostView()) {
+ // TODO(asimjour) Contents should not be aware of VR mode. Instead, we
+ // should add a flag for disabling specific UI such as the keyboard (see
+ // VrTabHelper for details).
contents->GetRenderWidgetHostView()->SetIsInVR(is_in_vr);
+
+ VrTabHelper* vr_tab_helper = VrTabHelper::FromWebContents(source);
+ DCHECK(vr_blocker_helper);
+ vr_tab_helper->set_is_in_vr(is_in_vr);
+ }
}

void LoadControllerModelTask(
Index: chrome/browser/android/vr_shell/vr_tab_helper.cc
diff --git a/chrome/browser/android/vr_shell/vr_tab_helper.cc b/chrome/browser/android/vr_shell/vr_tab_helper.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3429663c86196d84701192ecb231fd2c97047edc
--- /dev/null
+++ b/chrome/browser/android/vr_shell/vr_tab_helper.cc
@@ -0,0 +1,16 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/android/vr_shell/vr_tab_helper.h"
+
+#include "chrome/browser/android/vr_shell/vr_shell_delegate.h"
+#include "device/vr/android/gvr/gvr_delegate_provider.h"
+
+namespace vr_shell {
+
+VrTabHelper::VrTabHelper(content::WebContents* contents)
+ : content::WebContentsObserver(contents) {}
+VrTabHelper::~VrTabHelper() {}
+
+} // namespace vr_shell
Index: chrome/browser/android/vr_shell/vr_tab_helper.h
diff --git a/chrome/browser/android/vr_shell/vr_tab_helper.h b/chrome/browser/android/vr_shell/vr_tab_helper.h
new file mode 100644
index 0000000000000000000000000000000000000000..84cb9f1d32b8f850e206ff1ec88ac7ade545cccb
--- /dev/null
+++ b/chrome/browser/android/vr_shell/vr_tab_helper.h
@@ -0,0 +1,40 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_ANDROID_VR_SHELL_VR_TAB_HELPER_H_
+#define CHROME_BROWSER_ANDROID_VR_SHELL_VR_TAB_HELPER_H_
+
+#include "base/macros.h"
+
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace vr_shell {
+
+class VrTabHelper : public content::WebContentsObserver,
+ public content::WebContentsUserData<VrTabHelper> {
+ public:
+ ~VrTabHelper() override;
+
+ bool is_in_vr() const { return is_in_vr_; }
+
+ // Called by VrShell when we enter and exit vr mode. It finds us by looking us
+ // up on the WebContents. Eventually, we will also set a number of flags here
+ // as we enter and exit vr mode (see TODO below).
+ void set_is_in_vr(bool is_in_vr) { is_in_vr_ = is_in_vr_; }
+
+ private:
+ explicit VrTabHelper(content::WebContents* contents);
+
+ // TODO(asimjour): once we have per-dialog flags for disabling specific
+ // content-related popups, we should hang onto a pointer to the web contents
+ // and set those flags as we enter and exit vr mode.
+ bool in_vr_mode_;
+
+ DISALLOW_COPY_AND_ASSIGN(VrTabHelper);
+};
+
+} // namespace vr_shell
+
+#endif // CHROME_BROWSER_ANDROID_VR_SHELL_VR_TAB_HELPER_H_
Index: chrome/browser/ui/tab_helpers.cc
diff --git a/chrome/browser/ui/tab_helpers.cc b/chrome/browser/ui/tab_helpers.cc
index a70d9b267a6c77c6f5809e67b6f51e800f5c6fe9..a76f609cdbd134422007a1641474700c2f66fdf2 100644
--- a/chrome/browser/ui/tab_helpers.cc
+++ b/chrome/browser/ui/tab_helpers.cc
@@ -77,6 +77,7 @@
#include "chrome/browser/android/offline_pages/recent_tab_helper.h"
#include "chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.h"
#include "chrome/browser/android/voice_search_tab_helper.h"
+#include "chrome/browser/android/vr_tab_helper.h"
#include "chrome/browser/android/webapps/single_tab_mode_tab_helper.h"
#include "chrome/browser/ui/android/context_menu_helper.h"
#include "chrome/browser/ui/android/view_android_helper.h"
@@ -230,6 +231,7 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
SingleTabModeTabHelper::CreateForWebContents(web_contents);
ViewAndroidHelper::CreateForWebContents(web_contents);
VoiceSearchTabHelper::CreateForWebContents(web_contents);
+ VrTabHelper::CreateForWebContents(web_contents);
#else
BookmarkTabHelper::CreateForWebContents(web_contents);
extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(

[vol...@chromium.org]

On 2017/05/10 16:45:54, Ian Vollick wrote:
> Hey Ted. Is this heading in the direction you'd suggested?

Bleh. Looks like I've uploaded a bad patchset. Please disregard for the moment.

https://codereview.chromium.org/2874623004/

[vol...@chromium.org]

On 2017/05/10 16:51:53, Ian Vollick wrote:
> On 2017/05/10 16:45:54, Ian Vollick wrote:
> > Hey Ted. Is this heading in the direction you'd suggested?
>
> Bleh. Looks like I've uploaded a bad patchset. Please disregard for the
moment.

Ok, I think it's in a reviewable state now. Sorry for the spam.

https://codereview.chromium.org/2874623004/

[Brandon Jones]

Yes

--
You received this message because you are subscribed to the Google Groups "feature-vr-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email to feature-vr-revi...@chromium.org.
To post to this group, send email to feature-v...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4302fffd9a054f2e7aff%40google.com.

[vol...@chromium.org]

On 2017/05/10 17:16:11, chromium-reviews wrote:
> Yes
>
>
> On Wed, May 10, 2017, 11:05 AM <mailto:vol...@chromium.org> wrote:
>
> > On 2017/05/10 16:51:53, Ian Vollick wrote:
> > > On 2017/05/10 16:45:54, Ian Vollick wrote:
> > > > Hey Ted. Is this heading in the direction you'd suggested?
> > >
> > > Bleh. Looks like I've uploaded a bad patchset. Please disregard for the
> > moment.
> >
> > Ok, I think it's in a reviewable state now. Sorry for the spam.
> >
> > https://codereview.chromium.org/2874623004/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "feature-vr-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an

> > email to mailto:feature-vr-revi...@chromium.org.
> > To post to this group, send email to mailto:feature-v...@chromium.org.

> > To view this discussion on the web visit
> >
>
https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4302fffd9a054f2e7aff%40google.com
> >
>

<https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4302fffd9a054f2e7aff%40google.com?utm_medium=email&utm_source=footer&gt;
> > .

> >
>
> --
> You received this message because you are subscribed to the Google Groups

> "Chromium-reviews" group.

> To unsubscribe from this group and stop receiving emails from it, send an
email

> to mailto:chromium-revie...@chromium.org.

Sigh. My local build lied to me :( Still have some fixing up to do. I'll check
the trybots before bugging you again.

https://codereview.chromium.org/2874623004/

[vol...@chromium.org]

After an embarrassingly long struggle with the BUILD files, I think things are
green enough to review.

tedchoc@ can you PTAL?

https://codereview.chromium.org/2874623004/

[ted...@chromium.org]

Are there any initial examples of where we would use this? It would be nice to
land this with at least one so we "feel" this seems right.


https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h
File chrome/browser/android/vr_shell/vr_tab_helper.h (right):

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h#newcode14
chrome/browser/android/vr_shell/vr_tab_helper.h:14: class VrTabHelper :
public content::WebContentsObserver,
why a WebContentsObserver? Why being a user data on web contents,
you're going to be deleted when they do so I don't see why you need
this.

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h#newcode17
chrome/browser/android/vr_shell/vr_tab_helper.h:17: ~VrTabHelper()
override;
I suspect it might also be nice to have a simple static helper method
for most callers where they could do:

VrTabHelper::IsInVr(WebContents* web_contents);

Since the bulk of callers would only really care about that, we should
make it silly simple.

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h#newcode24
chrome/browser/android/vr_shell/vr_tab_helper.h:24: void
set_is_in_vr(bool is_in_vr) { is_in_vr_ = is_in_vr; }
since we expect this to do more in the not distance future, I would not
inline this now and make this: SetInVR(bool in_vr);

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/ui/tab_helpers.cc
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/ui/tab_helpers.cc#newcode81
chrome/browser/ui/tab_helpers.cc:81: #include
"device/vr/features/features.h"
why this include? why not in the ifdef if needed?

https://codereview.chromium.org/2874623004/

[vol...@chromium.org]

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h
File chrome/browser/android/vr_shell/vr_tab_helper.h (right):

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h#newcode14
chrome/browser/android/vr_shell/vr_tab_helper.h:14: class VrTabHelper :
public content::WebContentsObserver,

On 2017/05/11 18:32:23, Ted C wrote:
> why a WebContentsObserver? Why being a user data on web contents,
you're going
> to be deleted when they do so I don't see why you need this.

It's true, I don't need to be a WebContentsObserver at this point.
Removed.

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h#newcode17
chrome/browser/android/vr_shell/vr_tab_helper.h:17: ~VrTabHelper()
override;

On 2017/05/11 18:32:23, Ted C wrote:
> I suspect it might also be nice to have a simple static helper method
for most
> callers where they could do:
>
> VrTabHelper::IsInVr(WebContents* web_contents);
>
> Since the bulk of callers would only really care about that, we should
make it
> silly simple.

Actually, we were hoping to do this slightly differently to allow
per-popup notifications to be shown via the VrShell. Basically, we're
adding a HandleThisParticularPopup() function to VrTabHelper which will
do the "right thing" for that popup. This could include closing open
popups, and showing the notifications I mentioned earlier.


Please take a look at asimjour@'s cl here for an example:
https://codereview.chromium.org/2876023002


Since this change involved creating those HandleFoo functions, I'd left
that to Amir's follow-up, but I could fuse that in if you think it'd
help.

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h#newcode24
chrome/browser/android/vr_shell/vr_tab_helper.h:24: void
set_is_in_vr(bool is_in_vr) { is_in_vr_ = is_in_vr; }

On 2017/05/11 18:32:22, Ted C wrote:
> since we expect this to do more in the not distance future, I would
not inline
> this now and make this: SetInVR(bool in_vr);

Done.

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/ui/tab_helpers.cc
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/ui/tab_helpers.cc#newcode81
chrome/browser/ui/tab_helpers.cc:81: #include
"device/vr/features/features.h"

On 2017/05/11 18:32:23, Ted C wrote:
> why this include? why not in the ifdef if needed?

IIUC, it's needed for the following ifdef to work - this header is what
sets up ENABLE_VR.

https://codereview.chromium.org/2874623004/

[ted...@chromium.org]

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h
File chrome/browser/android/vr_shell/vr_tab_helper.h (right):

https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android/vr_shell/vr_tab_helper.h#newcode17
chrome/browser/android/vr_shell/vr_tab_helper.h:17: ~VrTabHelper()
override;

On 2017/05/11 19:30:10, Ian Vollick wrote:
> On 2017/05/11 18:32:23, Ted C wrote:
> > I suspect it might also be nice to have a simple static helper
method for most
> > callers where they could do:
> >
> > VrTabHelper::IsInVr(WebContents* web_contents);
> >
> > Since the bulk of callers would only really care about that, we
should make it
> > silly simple.
>
> Actually, we were hoping to do this slightly differently to allow
per-popup
> notifications to be shown via the VrShell. Basically, we're adding a
> HandleThisParticularPopup() function to VrTabHelper which will do the
"right
> thing" for that popup. This could include closing open popups, and
showing the
> notifications I mentioned earlier.
>
>
> Please take a look at asimjour@'s cl here for an example:
> https://codereview.chromium.org/2876023002
>
>
> Since this change involved creating those HandleFoo functions, I'd
left that to
> Amir's follow-up, but I could fuse that in if you think it'd help.

Hmm...from our other meeting, I didn't think that was the resolution we
came
to. I thought the VrTabHelper was the way to enable all the content
layer
settings, but we would look at replacing TabWebContentsDelegate with one
that is specific to VR. I think much of that would be looking at
exposing
java calls for all the UI bits to avoid the C++ inheritance.

For the things that go through the delegate, I think the short term plan
was to just use this to check whether you were in VR and abort. But the
long term plan was for it not to delegate into here to actually do any
work.

So I think instead of having those HandleXXX functions, they should just
be returning based on the boolean and we should work on finding a way
for
VR to handle this more broadly without this being the UI dumping ground
for all UI features in Chrome.

https://codereview.chromium.org/2874623004/

[vol...@chromium.org]

Gotcha. Ok, I've done this for a few things in TWCDA. Seem better?

https://codereview.chromium.org/2874623004/

[ted...@chromium.org]

lgtm




https://codereview.chromium.org/2874623004/diff/300001/chrome/browser/android/tab_web_contents_delegate_android.cc
File chrome/browser/android/tab_web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2874623004/diff/300001/chrome/browser/android/tab_web_contents_delegate_android.cc#newcode128
chrome/browser/android/tab_web_contents_delegate_android.cc:128: #if
BUILDFLAG(ENABLE_VR)
probably out of scope, but I wonder if we should make VrTabHelper
internalize this buildflag.

whether that is including a dummy implementation if the flag is off and
having different .cc files or if we should ifdef all the contents of
each of the functions.

I think if we see this moving throughout the codebase that might be
better, but for now this is fine.

https://codereview.chromium.org/2874623004/

[vol...@chromium.org]

This is a good point -- the ifdefs are pretty gross. I'll chat with Amir about a
plan for cleaning this up.

https://codereview.chromium.org/2874623004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2874623004/300001

https://codereview.chromium.org/2874623004/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/435262)

https://codereview.chromium.org/2874623004/

[vol...@chromium.org]

s...@chromium.org: Please review changes in chrome/browser/ui/

cjg...@chromium.org: Please review changes in vr_shell/



https://codereview.chromium.org/2874623004/

[cjg...@chromium.org]

lgtm



https://codereview.chromium.org/2874623004/

[s...@chromium.org]

On 2017/05/12 13:30:00, Ian Vollick wrote:
> mailto:s...@chromium.org: Please review changes in chrome/browser/ui/
>
> mailto:cjg...@chromium.org: Please review changes in vr_shell/

One question. The helper, and code, look to be android specific. Should the dep
in the BUILD.gn file be android specific too? Also, are you sure you don't need
to update a DEPS file too?

https://codereview.chromium.org/2874623004/

[vol...@chromium.org]

On 2017/05/12 17:08:25, sky wrote:
> On 2017/05/12 13:30:00, Ian Vollick wrote:
> > mailto:s...@chromium.org: Please review changes in chrome/browser/ui/
> >
> > mailto:cjg...@chromium.org: Please review changes in vr_shell/
>
> One question. The helper, and code, look to be android specific. Should the
dep
> in the BUILD.gn file be android specific too? Also, are you sure you don't
need
> to update a DEPS file too?

Sg, updated. Provided the bots are happy, does this seem better?

https://codereview.chromium.org/2874623004/

[s...@chromium.org]

Yes, LGTM

https://codereview.chromium.org/2874623004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2874623004/320001

https://codereview.chromium.org/2874623004/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/1bcc560114c81e6c9caea8f1fc27939734c81c1f

https://codereview.chromium.org/2874623004/