Swap to foreground tab when following links that open in a new tab/window in VR Shell.

Swap to foreground tab when following links that open in a new tab/window in VR Shell.

This implements basic tab switching functionality for VR Shell to switch to the foreground tab.
Currently no UI indicates that this has happened, and no UI has been created to allow for tab switching in any case other than opening links in a new window.

BUG=670452
Review-Url: https://codereview.chromium.org/2657703002
Cr-Commit-Position: refs/heads/master@{#446356}
Committed: https://chromium.googlesource.com/chromium/src/+/027ca1e1259f228e14ad253169ef0263963122fe

[mthiesse, 2017-01-24 21:18:44]

mthiesse@chromium.org changed reviewers:
+ billorr@chromium.org, bshe@chromium.org

[mthiesse, 2017-01-24 21:18:44]

PTAL bshe.

Adding billorr to review the TODO in VrShell::SwapContents, and whether it's
okay to just reset the MetricsHelper there.

[bsheedy, 2017-01-24 21:21:45]

On 2017/01/24 21:18:44, mthiesse wrote:
> PTAL bshe.
> 
> Adding billorr to review the TODO in VrShell::SwapContents, and whether it's
> okay to just reset the MetricsHelper there.

FYI, billorr is OOO until Friday.

[mthiesse, 2017-01-24 21:22:58]

mthiesse@chromium.org changed reviewers:
+ amp@chromium.org
- billorr@chromium.org

[mthiesse, 2017-01-24 21:22:59]

+amp to review in bill's place.

[amp, 2017-01-24 21:44:43]

On 2017/01/24 21:22:59, mthiesse wrote:
> +amp to review in bill's place.

I'm not as familiar with the Metrics helper.  By resetting it, does the previous
instance have a chance to write out the metrics it had gathered up to the point
of the swap?

If so I think resetting is fine for each swap.  We should do better going
forward, but it doesn't fill critical to make the metrics perfect now.


If it's not then we should do what we can to write those out before resetting.

[amp, 2017-01-24 21:51:53]

On 2017/01/24 21:44:43, amp wrote:
> On 2017/01/24 21:22:59, mthiesse wrote:
> > +amp to review in bill's place.
> 
> I'm not as familiar with the Metrics helper.  By resetting it, does the
previous
> instance have a chance to write out the metrics it had gathered up to the
point
> of the swap?
> 
> If so I think resetting is fine for each swap.  We should do better going
> forward, but it doesn't fill critical to make the metrics perfect now.
> 
> 
> If it's not then we should do what we can to write those out before resetting.

Looking through the code at
https://cs.chromium.org/chromium/src/chrome/browser/android/vr_shell/vr_usage...
it looks like there is a cleanup in the deconstructor of the sesion timer (which
the metric helper creates) so it looks like it would just count a swap as the
end of the session (which I think is a fine definition for now).

If we want to update the concept of a metrics session to include tracking across
tab swaps we will need to update the helper (which the TODO covers).

So looks good for the metrics helper question specifically (I haven't looked at
the rest of the code yet).

[mthiesse, 2017-01-24 21:54:55]

On 2017/01/24 21:44:43, amp wrote:
> On 2017/01/24 21:22:59, mthiesse wrote:
> > +amp to review in bill's place.
> 
> I'm not as familiar with the Metrics helper.  By resetting it, does the
previous
> instance have a chance to write out the metrics it had gathered up to the
point
> of the swap?
> 
> If so I think resetting is fine for each swap.  We should do better going
> forward, but it doesn't fill critical to make the metrics perfect now.
> 
> 
> If it's not then we should do what we can to write those out before resetting.

I think it does what it needs to do in the destructor. The sessions get stopped,
which updates the histograms accordingly.

I've added calls to set VR mode and webVR mode appropriately.

[bshe, 2017-01-25 22:15:45]

https://codereview.chromium.org/2657703002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2657703002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:279:
if (!canEnterVR(tab)) forceExitVr();
return after forceExitVr?

https://codereview.chromium.org/2657703002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2657703002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:217:
// TODO(mthiesse): Use last set values, rather than default values here.
nit: create a crbug so that we wont forget.

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

https://codereview.chromium.org/2657703002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:462: content_input_manager_.reset();
Is there case that content was hidden but SwapContents was not called
afterwards? If not, perhaps you can just remove this function, since
content_input_manager_ is set to a new instance in SwapContents

[mthiesse, 2017-01-25 22:34:26]

https://codereview.chromium.org/2657703002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2657703002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:279:
if (!canEnterVR(tab)) forceExitVr();
On 2017/01/25 22:15:45, bshe wrote:
> return after forceExitVr?

Done.

https://codereview.chromium.org/2657703002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2657703002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:217:
// TODO(mthiesse): Use last set values, rather than default values here.
On 2017/01/25 22:15:45, bshe wrote:
> nit: create a crbug so that we wont forget.

Oops this TODO was to remind myself to do this before uploading :P

Fixed.

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

https://codereview.chromium.org/2657703002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:462: content_input_manager_.reset();
On 2017/01/25 22:15:45, bshe wrote:
> Is there case that content was hidden but SwapContents was not called
> afterwards? If not, perhaps you can just remove this function, since
> content_input_manager_ is set to a new instance in SwapContents

I would rather not remove it. SwapContents will be called at some indeterminate
time in the future, and we don't want to keep sending input events while the
content is hidden.

Added the WasShown override as well in case the contents somehow come back.

[bshe, 2017-01-26 16:11:20]

lgtm

[mthiesse, 2017-01-26 16:17:17]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-01-26 16:17:39]

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

[amp, 2017-01-26 16:34:54]

lgtm

[commit-bot: I haz the power, 2017-01-26 16:54:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485447437741510,
"parent_rev": "eae6137965dceb05f01056f60f9ae6c95cd3cd12", "commit_rev":
"027ca1e1259f228e14ad253169ef0263963122fe"}

[commit-bot: I haz the power, 2017-01-26 16:54:40]

Description was changed from

==========
Swap to foreground tab when following links that open in a new tab/window in VR
Shell.

This implements basic tab switching functionality for VR Shell to switch to the
foreground tab.
Currently no UI indicates that this has happened, and no UI has been created to
allow for tab switching in any case other than opening links in a new window.

BUG=670452
==========

to

==========
Swap to foreground tab when following links that open in a new tab/window in VR
Shell.

This implements basic tab switching functionality for VR Shell to switch to the
foreground tab.
Currently no UI indicates that this has happened, and no UI has been created to
allow for tab switching in any case other than opening links in a new window.

BUG=670452

Review-Url: https://codereview.chromium.org/2657703002
Cr-Commit-Position: refs/heads/master@{#446356}
Committed:
https://chromium.googlesource.com/chromium/src/+/027ca1e1259f228e14ad253169ef...
==========

[commit-bot: I haz the power, 2017-01-26 16:54:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/027ca1e1259f228e14ad253169ef...