Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874623004:
[vr] Introduce VrTabHelper (Closed)
|
Created:
3 years, 7 months ago by Ian Vollick Modified:
3 years, 7 months ago CC:
chromium-reviews, feature-vr-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
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
Review-Url: https://codereview.chromium.org/2874623004
Cr-Commit-Position: refs/heads/master@{#471456}
Committed: https://chromium.googlesource.com/chromium/src/+/1bcc560114c81e6c9caea8f1fc27939734c81c1f
Patch Set 1 #Patch Set 2 : now with more code. #Patch Set 3 : fix typo #Patch Set 4 : fix a whole slew of errors (facepalm) #Patch Set 5 : BUILD updates #Patch Set 6 : more BUILD fixes #Patch Set 7 : another tilt at the windmill #Patch Set 8 : and another #Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : device #Patch Set 12 : unconditional dependency on device/vr #Patch Set 13 : . #
Total comments: 9
Patch Set 14 : . #Patch Set 15 : remove unused header #Patch Set 16 : . #
Total comments: 1
Patch Set 17 : nogncheck (thanks sadrul@, sky@)! #Messages
Total messages: 59 (38 generated)
Description was changed from ========== WIP - introduce vr tab helper BUG= ========== to ========== [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 ==========
vollick@chromium.org changed reviewers: + tedchoc@chromium.org
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey Ted. Is this heading in the direction you'd suggested?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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.
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
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.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vollick@chromium.org changed reviewers: + asimjour@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Yes On Wed, May 10, 2017, 11:05 AM <vollick@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 feature-vr-reviews+unsubscribe@chromium.org. > To post to this group, send email to feature-vr-reviews@chromium.org. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4... > <https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4...> > . > -- 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 chromium-reviews+unsubscribe@chromium.org.
On 2017/05/10 17:16:11, chromium-reviews wrote: > Yes > > > On Wed, May 10, 2017, 11:05 AM <mailto:vollick@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-reviews+unsubscribe@chromium.org. > > To post to this group, send email to mailto:feature-vr-reviews@chromium.org. > > To view this discussion on the web visit > > > https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4... > > > <https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4...> > > . > > > > -- > 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-reviews+unsubscribe@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.
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/10 17:33:24, Ian Vollick wrote: > On 2017/05/10 17:16:11, chromium-reviews wrote: > > Yes > > > > > > On Wed, May 10, 2017, 11:05 AM <mailto:vollick@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-reviews+unsubscribe@chromium.org. > > > To post to this group, send email to mailto:feature-vr-reviews@chromium.org. > > > To view this discussion on the web visit > > > > > > https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4... > > > > > > <https://groups.google.com/a/chromium.org/d/msgid/feature-vr-reviews/94eb2c1b4...> > > > . > > > > > > > -- > > 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-reviews+unsubscribe@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. After an embarrassingly long struggle with the BUILD files, I think things are green enough to review. tedchoc@ can you PTAL?
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... File chrome/browser/android/vr_shell/vr_tab_helper.h (right): https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android... 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... 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... 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_... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/ui/tab_... chrome/browser/ui/tab_helpers.cc:81: #include "device/vr/features/features.h" why this include? why not in the ifdef if needed?
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_tab_helper.h (right): https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android... 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... 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... 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_... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/ui/tab_... 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.
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_tab_helper.h (right): https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android... 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.
On 2017/05/11 20:25:36, Ted C wrote: > https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_tab_helper.h (right): > > https://codereview.chromium.org/2874623004/diff/240001/chrome/browser/android... > 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. Gotcha. Ok, I've done this for a few things in TWCDA. Seem better?
lgtm https://codereview.chromium.org/2874623004/diff/300001/chrome/browser/android... File chrome/browser/android/tab_web_contents_delegate_android.cc (right): https://codereview.chromium.org/2874623004/diff/300001/chrome/browser/android... 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.
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/11 22:35:05, Ted C wrote: > lgtm > > https://codereview.chromium.org/2874623004/diff/300001/chrome/browser/android... > File chrome/browser/android/tab_web_contents_delegate_android.cc (right): > > https://codereview.chromium.org/2874623004/diff/300001/chrome/browser/android... > 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. This is a good point -- the ifdefs are pretty gross. I'll chat with Amir about a plan for cleaning this up.
The CQ bit was unchecked by vollick@chromium.org
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@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_presub...)
vollick@chromium.org changed reviewers: + cjgrant@chromium.org, sky@chromium.org
sky@chromium.org: Please review changes in chrome/browser/ui/ cjgrant@chromium.org: Please review changes in vr_shell/
lgtm
On 2017/05/12 13:30:00, Ian Vollick wrote: > mailto:sky@chromium.org: Please review changes in chrome/browser/ui/ > > mailto:cjgrant@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?
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/12 17:08:25, sky wrote: > On 2017/05/12 13:30:00, Ian Vollick wrote: > > mailto:sky@chromium.org: Please review changes in chrome/browser/ui/ > > > > mailto:cjgrant@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?
Yes, LGTM
The CQ bit was unchecked by vollick@chromium.org
The CQ bit was checked by vollick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, cjgrant@chromium.org Link to the patchset: https://codereview.chromium.org/2874623004/#ps320001 (title: "nogncheck (thanks sadrul@, sky@)!")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1494625519338380,
"parent_rev": "e263cd1373b7f330054bc1b252c19abf1ac9ba09", "commit_rev":
"1bcc560114c81e6c9caea8f1fc27939734c81c1f"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 Review-Url: https://codereview.chromium.org/2874623004 Cr-Commit-Position: refs/heads/master@{#471456} Committed: https://chromium.googlesource.com/chromium/src/+/1bcc560114c81e6c9caea8f1fc27... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/1bcc560114c81e6c9caea8f1fc27... |
