[vr] Clicking on the security icon should prompt the user to bail out of VR

[vr] Clicking on the security icon should prompt the user to bail out of VR

BUG=726744
Review-Url: https://codereview.chromium.org/2913633002
Cr-Commit-Position: refs/heads/master@{#477546}
Committed: https://chromium.googlesource.com/chromium/src/+/db30f9ca562c571e7233710f0e9f69a882a7045e

[ymalik, 2017-06-02 01:14:03]

Description was changed from

==========
[vr] Clicking on the security icon should prompt the user to bail out of VR

BUG=726744
==========

to

==========
[vr] Clicking on the security icon should prompt the user to bail out of VR

BUG=726744
==========

[ymalik, 2017-06-02 01:14:03]

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

[cjgrant, 2017-06-02 04:16:36]

An initial round of feedback.  This is a big CL.  I know you said it's not
polished but I picked nits anyway.  :)

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

https://codereview.chromium.org/2913633002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:140:
// out of VR when they attempt to view the PageInfo from VR.
s/from VR// maybe, as it's already stated that you're in VR.

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

https://codereview.chromium.org/2913633002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:359:
// Called because showing PageInfo isn't supported in VR. This happens when
Wrapping looks off.  Auto-format?

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/color_scheme.h (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.h:34: // The colors used for text
and buttons on prompts.
So these are different than the URL bar's background and hover colors?  IIRC
Josh wanted per-element settings, so this is probably good.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:27: constexpr
float kWidth = 0.672;
Hmm, I'm used to these being static, but the codebase has both.  Seems like
there's a difference but I don't know what's better.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:55: // Prompt
text area
nit: Periods on comments. :)

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:76:
ToPixels(kWidth) / 2 - ToPixels(kButtonsSeperatorWidth) -
Why 3 ToPixels calls instead of one, on the result of math on unconverted
constants?

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:96: text =
l10n_util::GetStringUTF16(IDS_OK);
This block is really similar to the one above. Do you think it's worth pulling
out a helper method, and calling it twice with primary and secondary parameters?

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:157: bool
ExitPromptTexture::HitsPrimaryButton(const gfx::PointF& position) const {
nit: Should probably move this up with the other Hits method (ie. some kind of
sorting for readability).

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/exit_prompt_texture.h (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.h:27: void
SetPrimaryHovered(bool hovered);
Maybe call these SetPrimaryButtonHovered, etc. to match the Hits methods below,
and be more clear?

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/url_bar_texture.h (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.h:66: // Returns the
location of the security icon relative to the url bar in
This comment doesn't add much as it's a private method.  What about just calling
the method security_icon_position_meters()?  That helps at call sites.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exit_prompt.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exit_prompt.cc:48: if
(texture_->HitsPrimaryButton(position))
Don't we want to check that a particular button is down, before running its
callback?  We need to make sure that a click started and stopped on a button,
even if the reticle moved off and back on while clicked, right?

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/url_bar.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/url_bar.cc:66: if
(texture_->HitsSecurityIcon(position)) {
Same comment as earlier on down/up on the button.  I could be missing something.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:351: // backplane that we
add right behind the content rect so that the rectile
s/rectile/reticle/

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:460:
scene_->set_is_prompting_to_exit(true);
I need to look at this tomorrow, but I don't get why the scene should have any
notion of prompting.  Conceptually it's a batch of elements for GL to draw. 
Seems odd...

[amp, 2017-06-02 20:37:01]

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

[amp, 2017-06-02 20:37:02]

My color change is possibly going to need be merged depending on which makes it
in first.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/color_scheme.h (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.h:35: SkColor prompt_text;
Nit: The other values use 'foreground' instead of 'text', see line 30 above.  It
would be nice to be consistent.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.h:40: SkColor
button_background_hover;
do we need a button_background_down as well?  and are the hover states the same
for primary and secondary?

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:259:
background_elements_.push_back(element.get());
This is going to confuse the unit tests for transitions.

[ymalik, 2017-06-02 21:14:29]

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

[ymalik, 2017-06-02 21:14:32]

Guys, PTAL

I've added tests and addressed most of cjgrant@'s comments. Still have to
address some minor and amp@'s comments.

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

https://codereview.chromium.org/2913633002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:140:
// out of VR when they attempt to view the PageInfo from VR.
On 2017/06/02 04:16:35, cjgrant wrote:
> s/from VR// maybe, as it's already stated that you're in VR.

Done.

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

https://codereview.chromium.org/2913633002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:359:
// Called because showing PageInfo isn't supported in VR. This happens when
On 2017/06/02 04:16:35, cjgrant wrote:
> Wrapping looks off.  Auto-format?

The auto-format didn't fix it for some reason. Fixed manually.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/color_scheme.h (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.h:34: // The colors used for text
and buttons on prompts.
On 2017/06/02 04:16:35, cjgrant wrote:
> So these are different than the URL bar's background and hover colors?  IIRC
> Josh wanted per-element settings, so this is probably good.

Yeah, Josh has them here:
https://docs.google.com/document/d/139CuKm9NmArYqPAHIj5gXciGibUulcvyZvNKmVKjl...

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:27: constexpr
float kWidth = 0.672;
On 2017/06/02 04:16:35, cjgrant wrote:
> Hmm, I'm used to these being static, but the codebase has both.  Seems like
> there's a difference but I don't know what's better.

There's no difference because things in anonymous namespace have static linkage.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:55: // Prompt
text area
On 2017/06/02 04:16:35, cjgrant wrote:
> nit: Periods on comments. :)

Done.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:76:
ToPixels(kWidth) / 2 - ToPixels(kButtonsSeperatorWidth) -
On 2017/06/02 04:16:36, cjgrant wrote:
> Why 3 ToPixels calls instead of one, on the result of math on unconverted
> constants?

I was being silly :)

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:96: text =
l10n_util::GetStringUTF16(IDS_OK);
On 2017/06/02 04:16:36, cjgrant wrote:
> This block is really similar to the one above. Do you think it's worth pulling
> out a helper method, and calling it twice with primary and secondary
parameters?

We could, but we would either have to pass a lot of things as parameters, or
have "if(primary) else" blocks in the helper method. For example, the
differences are things like color, text, location.  I don't think it'll end up
looking much cleaner, so not worth it.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:157: bool
ExitPromptTexture::HitsPrimaryButton(const gfx::PointF& position) const {
On 2017/06/02 04:16:36, cjgrant wrote:
> nit: Should probably move this up with the other Hits method (ie. some kind of
> sorting for readability).

Done.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/exit_prompt_texture.h (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.h:27: void
SetPrimaryHovered(bool hovered);
On 2017/06/02 04:16:36, cjgrant wrote:
> Maybe call these SetPrimaryButtonHovered, etc. to match the Hits methods
below,
> and be more clear?

Done.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/url_bar_texture.h (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.h:66: // Returns the
location of the security icon relative to the url bar in
On 2017/06/02 04:16:36, cjgrant wrote:
> This comment doesn't add much as it's a private method.  What about just
calling
> the method security_icon_position_meters()?  That helps at call sites.

Done.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exit_prompt.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exit_prompt.cc:48: if
(texture_->HitsPrimaryButton(position))
On 2017/06/02 04:16:36, cjgrant wrote:
> Don't we want to check that a particular button is down, before running its
> callback?  We need to make sure that a click started and stopped on a button,
> even if the reticle moved off and back on while clicked, right?

Yeah totally. Fixed.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:351: // backplane that we
add right behind the content rect so that the rectile
On 2017/06/02 04:16:36, cjgrant wrote:
> s/rectile/reticle/

Done.

[amp, 2017-06-02 21:54:00]

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

https://codereview.chromium.org/2913633002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.cc:70:
incognito_scheme.button_background_hover = 0xFF8C8C8C;
The normal background_hover and background_down (line 26-27) were supposed to be
for buttons as well (I think, but I'm not sure which buttons), but now we have a
specific button_xxx and I think it could get confusing which is used where.

Should we be even more specific here (ie prompt_button_background_hover)?  Or is
that overkill?

https://codereview.chromium.org/2913633002/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2913633002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.h:54: // TODO(ymalik): We
shouldn't have ForTesting functions.
I had at one point (in a patch set, not commited) just added the macro
FRIEND_TEST_ALL_PREFIXES(UiSceneManagerTest, [testname]);

which gives the test access to more internals without having to declare specific
methods.

Would that be preferable?

[ymalik, 2017-06-04 04:08:56]

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

[ymalik, 2017-06-04 04:08:57]

Folks, PTAL :) 

+ted for PageInfo changes

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/color_scheme.h (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.h:35: SkColor prompt_text;
On 2017/06/02 20:37:01, amp wrote:
> Nit: The other values use 'foreground' instead of 'text', see line 30 above. 
It
> would be nice to be consistent.

Done. Though personally I found foreground weird at first. What if we have more
things in the "foreground"?

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.h:40: SkColor
button_background_hover;
On 2017/06/02 20:37:01, amp wrote:
> do we need a button_background_down as well?  and are the hover states the
same
> for primary and secondary?

Yes totally, latest patch adds that.

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:259:
background_elements_.push_back(element.get());
On 2017/06/02 20:37:01, amp wrote:
> This is going to confuse the unit tests for transitions.

I ran all the vr unit tests and they seem to pass. Which test in particular are
you talking about?

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:460:
scene_->set_is_prompting_to_exit(true);
On 2017/06/02 04:16:36, cjgrant wrote:
> I need to look at this tomorrow, but I don't get why the scene should have any
> notion of prompting.  Conceptually it's a batch of elements for GL to draw. 
> Seems odd...

The prompt is a part of the scene so it should know when something is visible?
Not sure. We also do the same for ExitWarning.

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

https://codereview.chromium.org/2913633002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/color_scheme.cc:70:
incognito_scheme.button_background_hover = 0xFF8C8C8C;
On 2017/06/02 21:53:59, amp wrote:
> The normal background_hover and background_down (line 26-27) were supposed to
be
> for buttons as well (I think, but I'm not sure which buttons), but now we have
a
> specific button_xxx and I think it could get confusing which is used where.
> 
> Should we be even more specific here (ie prompt_button_background_hover)?  Or
is
> that overkill?

I think being specific in this case definitely adds more clarity, so +1. Done.

https://codereview.chromium.org/2913633002/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2913633002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.h:54: // TODO(ymalik): We
shouldn't have ForTesting functions.
On 2017/06/02 21:53:59, amp wrote:
> I had at one point (in a patch set, not commited) just added the macro
> FRIEND_TEST_ALL_PREFIXES(UiSceneManagerTest, [testname]);
> 
> which gives the test access to more internals without having to declare
specific
> methods.
> 
> Would that be preferable?

Yes, that seems better. Done.

[amp, 2017-06-05 16:44:57]

lgtm

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2913633002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:259:
background_elements_.push_back(element.get());
On 2017/06/04 04:08:56, ymalik wrote:
> On 2017/06/02 20:37:01, amp wrote:
> > This is going to confuse the unit tests for transitions.
> 
> I ran all the vr unit tests and they seem to pass. Which test in particular
are
> you talking about?

Sorry, ignore this.  I was misremembering an older version of the tests (the
same one where I made the internals accessible) where the tests used 
content_elements_ to verify what is visible.  I momentarily forgot that we
transitioned to declaring explicitly what elements should be visible.

https://codereview.chromium.org/2913633002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2913633002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager.h:56: friend class
UiSceneManagerTest;
I thought you didn't need to declare the test as a friend class (doesn't the
macro below do that)?

[cjgrant, 2017-06-05 17:33:53]

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:111: SkColor
ExitPromptTexture::GetButtonColor(bool primary) const {
With all the separate methods for primary and secondary below, and all the
checks in this method to differentiate, why not have
GetPrimaryButtonColor()/GetSecondaryButtonColor()?

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:117: color =
color_scheme().prompt_button_background_down;
For me, the default color and overriding based on pressed looks a bit awkward. 
Any reason not to do:
if (pressed)
  return color_scheme()....
if (hovered)
  return color_scheme()...
if (primary)
  return...
return secondary

Just curious what you think.

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

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:138: gfx::PointF
UrlBarTexture::security_icon_position_meters() const {
This isn't a simple getter.  Shouldn't it be SecurityIconPositionMeters()?

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:149: return
rect.Contains(meters);
Maybe drop the 'meters' intermediate?  Would actually be more readable I think.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exit_prompt.h (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exit_prompt.h:36: // TODO(ymalik):
Should this move into a parent class?
Can we drop the todo?  Should either be a bug filed to refactor (with no need to
mention it here), or just assume future refactoring will happen if needed, IMO.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exit_prompt_unittest.cc
(right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exit_prompt_unittest.cc:49: 
Drop extra line?

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/url_bar.cc (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/url_bar.cc:58: if
(texture_->HitsSecurityIcon(position))
else if?

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:352: bool
disabled_for_exit_prompt =
It looks like we now have two content elements, and one of them is special-cased
here.  I think we should lose the loop for clarity.  I put the loops in only
because we had sets of elements that receive identical treatment.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:456: if
(scene_->is_prompting_to_exit())
In this method and the next, why is the initial if() needed?  For example, when
will we be not prompting to exit, and handling an exit prompt button click?

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:253:
std::set<UiElementDebugId> visible_in_browsing = {
visible_in_browsing should be in the empty namespace, so we don't duplicate the
set (it's shared with fullscreen tests).

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:254:
UiElementDebugId::kContentQuad, UiElementDebugId::kBackplane,
Even though it's not done above, can we use a 'using' to lose all the prefixes? 
Based on the explicit set type, they are superfluous.

[ymalik, 2017-06-05 20:02:42]

cjgrant, PTAL :)

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:111: SkColor
ExitPromptTexture::GetButtonColor(bool primary) const {
On 2017/06/05 17:33:53, cjgrant wrote:
> With all the separate methods for primary and secondary below, and all the
> checks in this method to differentiate, why not have
> GetPrimaryButtonColor()/GetSecondaryButtonColor()?

I had it this way because the hover and down colors are the same, but I agree
that this is more consistent. Dome.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/exit_prompt_texture.cc:117: color =
color_scheme().prompt_button_background_down;
On 2017/06/05 17:33:53, cjgrant wrote:
> For me, the default color and overriding based on pressed looks a bit awkward.

> Any reason not to do:
> if (pressed)
>   return color_scheme()....
> if (hovered)
>   return color_scheme()...
> if (primary)
>   return...
> return secondary
> 
> Just curious what you think.

I'm pretty indifferent, they both seem reasonable to me. Done :)

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

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:138: gfx::PointF
UrlBarTexture::security_icon_position_meters() const {
On 2017/06/05 17:33:53, cjgrant wrote:
> This isn't a simple getter.  Shouldn't it be SecurityIconPositionMeters()?

You're right. Done.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:149: return
rect.Contains(meters);
On 2017/06/05 17:33:53, cjgrant wrote:
> Maybe drop the 'meters' intermediate?  Would actually be more readable I
think.

Agreed. Done.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exit_prompt.h (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exit_prompt.h:36: // TODO(ymalik):
Should this move into a parent class?
On 2017/06/05 17:33:53, cjgrant wrote:
> Can we drop the todo?  Should either be a bug filed to refactor (with no need
to
> mention it here), or just assume future refactoring will happen if needed,
IMO.

Done.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exit_prompt_unittest.cc
(right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exit_prompt_unittest.cc:49: 
On 2017/06/05 17:33:53, cjgrant wrote:
> Drop extra line?

Done.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/url_bar.cc (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/url_bar.cc:58: if
(texture_->HitsSecurityIcon(position))
On 2017/06/05 17:33:53, cjgrant wrote:
> else if?

Done.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:352: bool
disabled_for_exit_prompt =
On 2017/06/05 17:33:53, cjgrant wrote:
> It looks like we now have two content elements, and one of them is
special-cased
> here.  I think we should lose the loop for clarity.  I put the loops in only
> because we had sets of elements that receive identical treatment.

Totally, good catch.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:456: if
(scene_->is_prompting_to_exit())
On 2017/06/05 17:33:53, cjgrant wrote:
> In this method and the next, why is the initial if() needed?  For example,
when
> will we be not prompting to exit, and handling an exit prompt button click?

This is there for safety and you're right that this should never happen. Maybe
if we ever decide to show the url bar with the prompt.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:253:
std::set<UiElementDebugId> visible_in_browsing = {
On 2017/06/05 17:33:53, cjgrant wrote:
> visible_in_browsing should be in the empty namespace, so we don't duplicate
the
> set (it's shared with fullscreen tests).

Done.

https://codereview.chromium.org/2913633002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:254:
UiElementDebugId::kContentQuad, UiElementDebugId::kBackplane,
On 2017/06/05 17:33:53, cjgrant wrote:
> Even though it's not done above, can we use a 'using' to lose all the
prefixes? 
> Based on the explicit set type, they are superfluous.

Done.

[ymalik, 2017-06-05 20:10:33]

https://codereview.chromium.org/2913633002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2913633002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager.h:56: friend class
UiSceneManagerTest;
On 2017/06/05 16:44:57, amp wrote:
> I thought you didn't need to declare the test as a friend class (doesn't the
> macro below do that)?

It sure does. Thanks.

[cjgrant, 2017-06-05 20:47:17]

lgtm with one last comment.

https://codereview.chromium.org/2913633002/diff/120001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2913633002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:18: using
UiElementDebugId::kContentQuad;
Oops, I didn't mean this.  But actually, if you ditch this, and also strip off
the UiElementDebugId prefix on all the uses, it compiles fine for me (on Android
and soon-to-be Linux).  So we can just trim them off, unless you think this is
bad.

[ymalik, 2017-06-06 02:14:09]

https://codereview.chromium.org/2913633002/diff/120001/chrome/browser/android...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2913633002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:18: using
UiElementDebugId::kContentQuad;
On 2017/06/05 20:47:17, cjgrant wrote:
> Oops, I didn't mean this.  But actually, if you ditch this, and also strip off
> the UiElementDebugId prefix on all the uses, it compiles fine for me (on
Android
> and soon-to-be Linux).  So we can just trim them off, unless you think this is
> bad.

Ah, the reason this works is because its not a strongly typed enum. I think it
should be and we should fix that in a follow up CL, in which case we'd need
this. Strongly typed enums are better to void namespace collisions. I still like
this better since its we're using this in more than 1 place and it makes the
test look nicer.

[ymalik, 2017-06-06 02:16:45]

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

[ymalik, 2017-06-06 02:16:46]

+isherman for tools/metrics

[Ted C, 2017-06-06 05:00:14]

On 2017/06/06 02:16:46, ymalik wrote:
> +isherman for tools/metrics

PageInfoPopup - lgtm

[Ilya Sherman, 2017-06-06 05:40:49]

actions.xml lgtm % nits:

https://codereview.chromium.org/2913633002/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2913633002/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:10805: Page Info opened via the toolbar page
info (i.e https icon) icon in VR.
nit: "i.e."

https://codereview.chromium.org/2913633002/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:10805: Page Info opened via the toolbar page
info (i.e https icon) icon in VR.
nit: Duplicate "icon"; please rephrase

[ymalik, 2017-06-06 15:19:59]

https://codereview.chromium.org/2913633002/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2913633002/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:10805: Page Info opened via the toolbar page
info (i.e https icon) icon in VR.
On 2017/06/06 05:40:49, Ilya Sherman wrote:
> nit: Duplicate "icon"; please rephrase

Done.

https://codereview.chromium.org/2913633002/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:10805: Page Info opened via the toolbar page
info (i.e https icon) icon in VR.
On 2017/06/06 05:40:49, Ilya Sherman wrote:
> nit: "i.e."

Done.

[ymalik, 2017-06-06 15:20:06]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-06 15:20:06]

The patchset sent to the CQ was uploaded after l-g-t-m from amp@chromium.org,
cjgrant@chromium.org, tedchoc@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2913633002/#ps160001
(title: "actions.xml nit")

[commit-bot: I haz the power, 2017-06-06 15:20:28]

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-06-06 15:34:00]

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

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

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...)

[ymalik, 2017-06-06 15:50:03]

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

[ymalik, 2017-06-06 15:50:05]

+Mike for android vr_shell

[mthiesse, 2017-06-06 15:57:07]

lgtm % comments

https://codereview.chromium.org/2913633002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2913633002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:909:
sInstance.showPageInfoPopup();
nit: drop the sInstance.

https://codereview.chromium.org/2913633002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:909:
sInstance.showPageInfoPopup();
Why not move this down below the shutdownVr call? Then in showPageInfoPopup()
you can clear mShouldShowPageInfo

[ymalik, 2017-06-06 16:15:44]

https://codereview.chromium.org/2913633002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2913633002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:909:
sInstance.showPageInfoPopup();
On 2017/06/06 15:57:07, mthiesse wrote:
> Why not move this down below the shutdownVr call? Then in showPageInfoPopup()
> you can clear mShouldShowPageInfo

We want to clear mShouldShowPageInfo for shutdownVr calls for reasons other than
doff flow successes. Clarified with comment.

https://codereview.chromium.org/2913633002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:909:
sInstance.showPageInfoPopup();
On 2017/06/06 15:57:07, mthiesse wrote:
> nit: drop the sInstance.

Done.

[ymalik, 2017-06-06 19:54:57]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-06 19:54:58]

The patchset sent to the CQ was uploaded after l-g-t-m from amp@chromium.org,
tedchoc@chromium.org, mthiesse@chromium.org, cjgrant@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2913633002/#ps200001
(title: "UX adjustments as per josh's feedback")

[commit-bot: I haz the power, 2017-06-06 19:55:28]

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-06-06 20:10:30]

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

[commit-bot: I haz the power, 2017-06-06 20:10:34]

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...)

[ymalik, 2017-06-06 20:50:36]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-06 20:50:37]

The patchset sent to the CQ was uploaded after l-g-t-m from amp@chromium.org,
tedchoc@chromium.org, mthiesse@chromium.org, cjgrant@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2913633002/#ps220001
(title: "presubmit fix")

[commit-bot: I haz the power, 2017-06-06 20:51:02]

CQ is trying da patch.

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

[ymalik, 2017-06-06 21:00:54]

The CQ bit was unchecked by ymalik@chromium.org

[ymalik, 2017-06-07 01:45:01]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-07 01:45:02]

The patchset sent to the CQ was uploaded after l-g-t-m from amp@chromium.org,
tedchoc@chromium.org, mthiesse@chromium.org, cjgrant@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2913633002/#ps240001
(title: "fix compile issues on arm")

[commit-bot: I haz the power, 2017-06-07 01:45:22]

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-06-07 01:47:33]

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

[commit-bot: I haz the power, 2017-06-07 01:47:35]

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-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ymalik, 2017-06-07 02:54:55]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-07 02:54:56]

The patchset sent to the CQ was uploaded after l-g-t-m from amp@chromium.org,
tedchoc@chromium.org, mthiesse@chromium.org, cjgrant@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2913633002/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-07 02:55: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-06-07 04:25:26]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496804095446540,
"parent_rev": "93679c17dea4c99e6d654e3d038097e79f0c52f9", "commit_rev":
"db30f9ca562c571e7233710f0e9f69a882a7045e"}

[commit-bot: I haz the power, 2017-06-07 04:25:35]

Description was changed from

==========
[vr] Clicking on the security icon should prompt the user to bail out of VR

BUG=726744
==========

to

==========
[vr] Clicking on the security icon should prompt the user to bail out of VR

BUG=726744

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

[commit-bot: I haz the power, 2017-06-07 04:25:41]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/db30f9ca562c571e7233710f0e9f...