Disable back button when stack is empty

Disable back button when stack is empty

If the back stack is empty
- The back button icon's color is that of the foreground with an alpha of 0x33
- Hovering over the back button doesn't change its color

Note:
- tested manually due to time constraints, but we should add a tests

BUG=726889
Review-Url: https://codereview.chromium.org/2906403002
Cr-Commit-Position: refs/heads/master@{#475375}
Committed: https://chromium.googlesource.com/chromium/src/+/ed4df89fed450d8f2741bba7cd2651ad28c6a779

[ymalik, 2017-05-29 16:01:03]

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

[cjgrant, 2017-05-29 16:12:25]

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:111: // Note: We
don't have a forward button in the URL bar.
Comment disappears with the arg.

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:188: if
(can_go_back_) {
Can we kill the ternaries here?  This would read way easier with just if, else
if, else if, else, IMO.

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:217: can_go_back_ ?
color_scheme().foreground : color_scheme().deemphasized);
deemphasized is a URL text color, right?  Shouldn't we have a separate color for
inactive buttons?  Seems like we shouldn't tie these together.

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.h (right):

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.h:42: void
SetHistoryButtonsEnabled(bool can_go_back, bool can_go_forward);
Since the texture only supports back, why not drop forward here?

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements/url_bar.cc (right):

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements/url_bar.cc:86: void
UrlBar::SetHistoryButtonsEnabled(bool can_go_back, bool can_go_forward) {
URL bar presumably also doesn't care about forward.

[ymalik, 2017-05-29 17:33:46]

Description was changed from

==========
Disable back button when stack is empty

If the back stack is empty
- The back button icon's color is deemphasized
- Hovering over the back button doesn't change its color

Note:
- the deemphasized color scheme is pretty subtle IMO, we we may change it
if needed.
- tested manually due to time constraints, but we should add a tests

BUG=726889
==========

to

==========
Disable back button when stack is empty

If the back stack is empty
- The back button icon's color is that of the foreground with an alpha of 0x33
- Hovering over the back button doesn't change its color

Note:
- tested manually due to time constraints, but we should add a tests

BUG=726889
==========

[ymalik, 2017-05-29 17:34:10]

@cjgrant, PTAL :)

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:111: // Note: We
don't have a forward button in the URL bar.
On 2017/05/29 16:12:25, cjgrant wrote:
> Comment disappears with the arg.

Done.

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:188: if
(can_go_back_) {
On 2017/05/29 16:12:25, cjgrant wrote:
> Can we kill the ternaries here?  This would read way easier with just if, else
> if, else if, else, IMO.

Done.

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:188: if
(can_go_back_) {
On 2017/05/29 16:12:25, cjgrant wrote:
> Can we kill the ternaries here?  This would read way easier with just if, else
> if, else if, else, IMO.

Done.

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:217: can_go_back_ ?
color_scheme().foreground : color_scheme().deemphasized);
On 2017/05/29 16:12:25, cjgrant wrote:
> deemphasized is a URL text color, right?  Shouldn't we have a separate color
for
> inactive buttons?  Seems like we shouldn't tie these together.

Yeah was just here as placeholder. Posted hoverboard-dev for color and updated
with the correct value.

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.h (right):

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.h:42: void
SetHistoryButtonsEnabled(bool can_go_back, bool can_go_forward);
On 2017/05/29 16:12:25, cjgrant wrote:
> Since the texture only supports back, why not drop forward here?

I figured that it may at some point, so plumbed through, but its probably okay
to remove. DOne.

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements/url_bar.cc (right):

https://codereview.chromium.org/2906403002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements/url_bar.cc:86: void
UrlBar::SetHistoryButtonsEnabled(bool can_go_back, bool can_go_forward) {
On 2017/05/29 16:12:25, cjgrant wrote:
> URL bar presumably also doesn't care about forward.

Done.

[cjgrant, 2017-05-29 17:42:13]

lgtm

https://codereview.chromium.org/2906403002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2906403002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:187: if (hovered_)
nit: Why override one if with the next?  Isn't it just:

if(pressed)
  color = down
else if (hovered)
  color = hover

[ymalik, 2017-05-29 17:55:36]

https://codereview.chromium.org/2906403002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2906403002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:187: if (hovered_)
On 2017/05/29 17:42:13, cjgrant wrote:
> nit: Why override one if with the next?  Isn't it just:
> 
> if(pressed)
>   color = down
> else if (hovered)
>   color = hover

Done.

[ymalik, 2017-05-29 17:55:52]

The CQ bit was checked by ymalik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 17:56:04]

Dry run: 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-05-29 18:34:52]

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

[commit-bot: I haz the power, 2017-05-29 18:34:53]

Dry run: This issue passed the CQ dry run.

[ymalik, 2017-05-29 19:33:04]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-05-29 19:33:04]

The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2906403002/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2017-05-29 19:33:23]

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-05-29 19:37:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496086384267000,
"parent_rev": "8b83adb8cd1eec1b0db28353b9cd106613160915", "commit_rev":
"ed4df89fed450d8f2741bba7cd2651ad28c6a779"}

[commit-bot: I haz the power, 2017-05-29 19:37:24]

Description was changed from

==========
Disable back button when stack is empty

If the back stack is empty
- The back button icon's color is that of the foreground with an alpha of 0x33
- Hovering over the back button doesn't change its color

Note:
- tested manually due to time constraints, but we should add a tests

BUG=726889
==========

to

==========
Disable back button when stack is empty

If the back stack is empty
- The back button icon's color is that of the foreground with an alpha of 0x33
- Hovering over the back button doesn't change its color

Note:
- tested manually due to time constraints, but we should add a tests

BUG=726889

Review-Url: https://codereview.chromium.org/2906403002
Cr-Commit-Position: refs/heads/master@{#475375}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed4df89fed450d8f2741bba7cd26...
==========

[commit-bot: I haz the power, 2017-05-29 19:37:25]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ed4df89fed450d8f2741bba7cd26...