Visually disable VR Shell back/forward buttons when no history is available

Visually disable VR Shell back/forward buttons when no history is available

The CSS class was defined for this purpose is 'disabled-button'

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

Review-Url: https://codereview.chromium.org/2735983004
Cr-Commit-Position: refs/heads/master@{#455591}
Committed: https://chromium.googlesource.com/chromium/src/+/87cc44e2eb708c276f8fb2b6644588753ed75c6a

[acondor_, 2017-03-08 16:08:29]

Description was changed from

==========
Visually disable VR Shell back/forward buttons when no history is available

The CSS class was defined for this purpose is 'disabled-button'

BUG=684634
==========

to

==========
Visually disable VR Shell back/forward buttons when no history is available

The CSS class was defined for this purpose is 'disabled-button'

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

[acondor_, 2017-03-08 16:10:01]

acondor@google.com changed reviewers:
+ mthiesse@chromium.org

[acondor_, 2017-03-08 16:10:02]

Please take a look

[mthiesse, 2017-03-08 16:15:18]

Looks good overall, a few minor comments.

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

https://codereview.chromium.org/2735983004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:573:
canGoForward = mTab != null && mTab.canGoForward();
Chrome style prefers 'boolean canGoForward = mTab != null &&
mTab.canGoForward();'
We don't usually create multiple variables on one line.

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.css (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.css:120: .disabled-button {
nit: newline before this

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:984:
this.controls.setBackButtonEnabled(this.canGoBack);
This breaks fullscreen exit doesn't it? I think you want something like:
this.controls.setBackButtonEnabled(this.canGoBack || fullscreen);

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui_api.js:630: * Set back/forward
history buttons to enabled/disabled
Nit: period at the end of sentences in comments

[mthiesse, 2017-03-08 16:25:07]

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:186:
this.button.addEventListener('click', callback);
We should also store this callback and add/remove it when enabling/disabling the
button.

[cjgrant, 2017-03-08 16:32:14]

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

[cjgrant, 2017-03-08 16:32:15]

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:292:
this.buttons[0].setEnabled(enabled);
This indexing looks a bit risky.  Anyone could reorder the buttons above, and
break this.  Maybe get rid of 'descriptors' above, manually build each element
and append them to a list (keeping explicit references to the back and forward
buttons), then have the for loop walk through the list of existing elements
instead.  Just a thought...

[amp, 2017-03-08 16:34:13]

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

[amp, 2017-03-08 16:34:14]

Are there any tests we can update to cover these scenarios?

I thought we had some js ui tests, but looking now I wasn't able to find any. 
(If there isn't a base to build on don't worry about adding them for this
change, but if you can add to an existing suite of tests please do so).

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:292:
this.buttons[0].setEnabled(enabled);
These hard coded indexes seem fragile if we ever happen to change the button
order or add new ones etc.

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:984:
this.controls.setBackButtonEnabled(this.canGoBack);
On 2017/03/08 16:15:18, mthiesse wrote:
> This breaks fullscreen exit doesn't it? I think you want something like:
> this.controls.setBackButtonEnabled(this.canGoBack || fullscreen);

+1.  I was going to ask about this.

Note that regular 2D chrome supports exiting full screen on back navitgation as
well, so it might be worthwhile to see if the mTab.canGoBack() check in
VrShellImpl.java already covers it (but I'm not confident that it will as I
suspect they coded in an exception for full screen as well).

[acondor_, 2017-03-08 16:42:26]

Updates on the comments

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

https://codereview.chromium.org/2735983004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:573:
canGoForward = mTab != null && mTab.canGoForward();
On 2017/03/08 16:15:17, mthiesse wrote:
> Chrome style prefers 'boolean canGoForward = mTab != null &&
> mTab.canGoForward();'
> We don't usually create multiple variables on one line.

Done.

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.css (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.css:120: .disabled-button {
On 2017/03/08 16:15:17, mthiesse wrote:
> nit: newline before this

Done.

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:292:
this.buttons[0].setEnabled(enabled);
I agree with the fact that it's risky and the suggested solution seems
reasonable to me. Or, we could make `buttons` an object instead of an array.
What do you think?

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:984:
this.controls.setBackButtonEnabled(this.canGoBack);
On 2017/03/08 16:15:18, mthiesse wrote:
> This breaks fullscreen exit doesn't it? I think you want something like:
> this.controls.setBackButtonEnabled(this.canGoBack || fullscreen);

Done.

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui_api.js:630: * Set back/forward
history buttons to enabled/disabled
On 2017/03/08 16:15:18, mthiesse wrote:
> Nit: period at the end of sentences in comments

Done.

[cjgrant, 2017-03-08 18:19:32]

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:292:
this.buttons[0].setEnabled(enabled);
On 2017/03/08 16:42:26, acondor wrote:
> I agree with the fact that it's risky and the suggested solution seems
> reasonable to me. Or, we could make `buttons` an object instead of an array.
> What do you think?

The array was used only as a convenience for positioning a variable number of
buttons in a row, and for showing/hiding them all easily.  I'm not attached to
that idea, so feel free to suggest any modification through this review. :)

[mthiesse, 2017-03-08 19:38:16]

To adam's question, there is no existing test framework for the UI. We've got a
bug filed to create one, but it's not trivial to set up.

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:984:
this.controls.setBackButtonEnabled(this.canGoBack);
On 2017/03/08 16:34:14, amp wrote:
> On 2017/03/08 16:15:18, mthiesse wrote:
> > This breaks fullscreen exit doesn't it? I think you want something like:
> > this.controls.setBackButtonEnabled(this.canGoBack || fullscreen);
> 
> +1.  I was going to ask about this.
> 
> Note that regular 2D chrome supports exiting full screen on back navitgation
as
> well, so it might be worthwhile to see if the mTab.canGoBack() check in
> VrShellImpl.java already covers it (but I'm not confident that it will as I
> suspect they coded in an exception for full screen as well).

mTab.canGoBack() does not cover the fullscreen case (everything is a special
case in clank :P). They simply ask the navigationController whether the page can
navigate backwards/forwards.

[mthiesse, 2017-03-08 19:40:37]

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:186:
this.button.addEventListener('click', callback);
On 2017/03/08 16:25:07, mthiesse wrote:
> We should also store this callback and add/remove it when enabling/disabling
the
> button.

I think you missed this comment?

[acondor_, 2017-03-08 20:21:39]

The missing comment was addressed.

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:186:
this.button.addEventListener('click', callback);
On 2017/03/08 19:40:37, mthiesse wrote:
> On 2017/03/08 16:25:07, mthiesse wrote:
> > We should also store this callback and add/remove it when enabling/disabling
> the
> > button.
> 
> I think you missed this comment?

Done.

[mthiesse, 2017-03-08 20:40:29]

lgtm

[cjgrant, 2017-03-08 20:52:01]

lgtm after one more comment

https://codereview.chromium.org/2735983004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:974: this.updateState();
Can you call straight through to this.controls, like onRemoveTab() does?  It
would just save reconfiguring the whole UI when we only have to adjust button
properties here.

[acondor_, 2017-03-08 21:21:27]

Suggestion for optimization addressed.

https://codereview.chromium.org/2735983004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2735983004/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:974: this.updateState();
On 2017/03/08 20:52:01, cjgrant wrote:
> Can you call straight through to this.controls, like onRemoveTab() does?  It
> would just save reconfiguring the whole UI when we only have to adjust button
> properties here.

Done.

[acondor_, 2017-03-08 21:23:34]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-03-08 21:23:34]

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/2735983004/#ps60001
(title: "Optimizing code by avoiding reload of whole UI")

[commit-bot: I haz the power, 2017-03-08 21:24:15]

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-03-08 21:35:44]

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

[commit-bot: I haz the power, 2017-03-08 21:35:45]

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

[acondor_, 2017-03-08 22:12:44]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-03-08 22:12:44]

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/2735983004/#ps80001
(title: "Addressing closure compilation error")

[commit-bot: I haz the power, 2017-03-08 22:13:11]

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-03-08 23:24:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489011164297380,
"parent_rev": "cec2c2fc45b7788d8140736d3d0827d91c2d5496", "commit_rev":
"87cc44e2eb708c276f8fb2b6644588753ed75c6a"}

[commit-bot: I haz the power, 2017-03-08 23:25:06]

Description was changed from

==========
Visually disable VR Shell back/forward buttons when no history is available

The CSS class was defined for this purpose is 'disabled-button'

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

to

==========
Visually disable VR Shell back/forward buttons when no history is available

The CSS class was defined for this purpose is 'disabled-button'

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

Review-Url: https://codereview.chromium.org/2735983004
Cr-Commit-Position: refs/heads/master@{#455591}
Committed:
https://chromium.googlesource.com/chromium/src/+/87cc44e2eb708c276f8fb2b66445...
==========

[commit-bot: I haz the power, 2017-03-08 23:25:08]

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