Switch tabs from VR shell.

Switch tabs from VR shell.
- When clicking on a tab in VR shell this tab is shown in the content quad.

BUG=641487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2684233003
Cr-Commit-Position: refs/heads/master@{#450872}
Committed: https://chromium.googlesource.com/chromium/src/+/4807ace9c09bd194d2b453d9cab1689383539923

[tiborg, 2017-02-09 21:16:07]

Description was changed from

==========
Switch tabs from VR shell.
- When clicking on a tab in VR shell this tab is shown in the content quad.

BUG=641487
==========

to

==========
Switch tabs from VR shell.
- When clicking on a tab in VR shell this tab is shown in the content quad.

BUG=641487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tiborg, 2017-02-09 21:21:59]

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

[tiborg, 2017-02-09 21:22:00]

cjgrant@chromium.org: PTAL.

mthiesse@chromium.org: PTAL.

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

https://codereview.chromium.org/2684233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:636:
int index =
mActivity.getTabModelSelector().getModel(tab.isIncognito()).indexOf(tab);
I don't know why this line and the one below are so long. It's the result of
'git cl format'.

https://codereview.chromium.org/2684233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:643:
}
What would be the best way to deal with an unknown tab ID? Just ignore (like it
is right now), exception, or something else?

[mthiesse, 2017-02-09 21:37:56]

lgtm

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

https://codereview.chromium.org/2684233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:636:
int index =
mActivity.getTabModelSelector().getModel(tab.isIncognito()).indexOf(tab);
On 2017/02/09 21:22:00, tiborg wrote:
> I don't know why this line and the one below are so long. It's the result of
> 'git cl format'.

java lines are 100 characters long.

https://codereview.chromium.org/2684233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:643:
}
On 2017/02/09 21:22:00, tiborg wrote:
> What would be the best way to deal with an unknown tab ID? Just ignore (like
it
> is right now), exception, or something else?

Yeah, ignoring it is probably best.

[cjgrant, 2017-02-10 16:02:15]

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

https://codereview.chromium.org/2684233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:635:
if (tab != null) {
Should this be an early return instead?

[tiborg, 2017-02-10 16:34:17]

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

https://codereview.chromium.org/2684233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:635:
if (tab != null) {
On 2017/02/10 16:02:15, cjgrant wrote:
> Should this be an early return instead?

You mean instead of the nested if statements?

[cjgrant, 2017-02-10 18:14:29]

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

https://codereview.chromium.org/2684233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:635:
if (tab != null) {
On 2017/02/10 16:34:17, tiborg wrote:
> On 2017/02/10 16:02:15, cjgrant wrote:
> > Should this be an early return instead?
> 
> You mean instead of the nested if statements?

Yes.  That's the preference, to avoid excessive indentation, and make sure that
the typical execution path is clear.

[tiborg, 2017-02-13 17:35:37]

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

https://codereview.chromium.org/2684233003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:635:
if (tab != null) {
On 2017/02/10 18:14:29, cjgrant wrote:
> On 2017/02/10 16:34:17, tiborg wrote:
> > On 2017/02/10 16:02:15, cjgrant wrote:
> > > Should this be an early return instead?
> > 
> > You mean instead of the nested if statements?
> 
> Yes.  That's the preference, to avoid excessive indentation, and make sure
that
> the typical execution path is clear.

Done.

[cjgrant, 2017-02-15 17:50:44]

lgtm

[cjgrant, 2017-02-15 17:50:44]

lgtm

[tiborg, 2017-02-15 17:52:39]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-02-15 17:52:40]

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/2684233003/#ps20001
(title: "Clearer execution path, rebased on ToT")

[commit-bot: I haz the power, 2017-02-15 17:53:37]

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, 2017-02-15 17:57:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 17:57:08]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tiborg, 2017-02-15 22:19:47]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-02-15 22:19:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2684233003/#ps40001
(title: "Rebased on ToT, fixed crash")

[commit-bot: I haz the power, 2017-02-15 22:20:46]

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, 2017-02-15 22:37:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 22:37:07]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tiborg, 2017-02-15 22:43:54]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-02-15 22:43:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2684233003/#ps60001
(title: "Rebased on ToT")

[commit-bot: I haz the power, 2017-02-15 22:45:04]

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, 2017-02-15 23:34:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 23:34:34]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mthiesse, 2017-02-16 02:48:57]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-02-16 02:50:24]

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, 2017-02-16 04:48:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487213337697900,
"parent_rev": "c96485dba1223b9a7c6378b4a5b6c5a883e2b3e1", "commit_rev":
"4807ace9c09bd194d2b453d9cab1689383539923"}

[commit-bot: I haz the power, 2017-02-16 04:49:23]

Description was changed from

==========
Switch tabs from VR shell.
- When clicking on a tab in VR shell this tab is shown in the content quad.

BUG=641487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Switch tabs from VR shell.
- When clicking on a tab in VR shell this tab is shown in the content quad.

BUG=641487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2684233003
Cr-Commit-Position: refs/heads/master@{#450872}
Committed:
https://chromium.googlesource.com/chromium/src/+/4807ace9c09bd194d2b453d9cab1...
==========

[commit-bot: I haz the power, 2017-02-16 04:49:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4807ace9c09bd194d2b453d9cab1...