[vr] Close exit prompt when clicking on background

[vr] Close exit prompt when clicking on background

This is a follow-up patch to https://codereview.chromium.org/2913633002/

BUG=726744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2921383002
Cr-Commit-Position: refs/heads/master@{#478057}
Committed: https://chromium.googlesource.com/chromium/src/+/ee13d1180e1625c5d5f6b1ef4b4b90192be20cb9

[ymalik, 2017-06-06 03:49:06]

Description was changed from

==========
[vr] Close exit prompt when clicking on background

This is a follow-up patch to https://codereview.chromium.org/2913633002/

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

to

==========
[vr] Close exit prompt when clicking on background

This is a follow-up patch to https://codereview.chromium.org/2913633002/

BUG=726744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ymalik, 2017-06-06 03:52:54]

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

[ymalik, 2017-06-06 03:52:54]

@cjgrant, PTAL :)

[cjgrant, 2017-06-06 19:42:14]

https://codereview.chromium.org/2921383002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/content_backplane.cc (right):

https://codereview.chromium.org/2921383002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/content_backplane.cc:11:
ContentBackplane::ContentBackplane(const base::Callback<void()>& click_callback)
Documenting an offline conversation, I think the prompt should have its own
backplane, and it also doesn't need to be a separate class.

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

https://codereview.chromium.org/2921383002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:312: for (const
auto& element : scene_->GetUiElements()) {
Based on how many of these loops we have throughout tests, and the fact that
SCOPED_TRACE is our new friend, could we break out a module- or class-level
helper method, VerifyElementsVisible(std::set<elements>)?

This could probably be:
VerifyElementsVisible({kExitPrompt, kBackplane, kCeiling, kFloor});

https://codereview.chromium.org/2921383002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:321: for (const
auto& element : scene_->GetUiElements()) {
Originally I thought it's weird so many tests check which elements are visible
in normal browsing mode.  But I changed my mind, and I think it does make sense.
 If we used the helper method approach, doing so would be a one-liner. 
Thoughts?

[ymalik, 2017-06-06 21:11:58]

PTAL :)

https://codereview.chromium.org/2921383002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/content_backplane.cc (right):

https://codereview.chromium.org/2921383002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/content_backplane.cc:11:
ContentBackplane::ContentBackplane(const base::Callback<void()>& click_callback)
On 2017/06/06 19:42:13, cjgrant wrote:
> Documenting an offline conversation, I think the prompt should have its own
> backplane, and it also doesn't need to be a separate class.

Done. We do need a separate class since UiElement alone doesn't have the notion
of a callback.

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

https://codereview.chromium.org/2921383002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:312: for (const
auto& element : scene_->GetUiElements()) {
On 2017/06/06 19:42:13, cjgrant wrote:
> Based on how many of these loops we have throughout tests, and the fact that
> SCOPED_TRACE is our new friend, could we break out a module- or class-level
> helper method, VerifyElementsVisible(std::set<elements>)?
> 
> This could probably be:
> VerifyElementsVisible({kExitPrompt, kBackplane, kCeiling, kFloor});

Done. Much cleaner.

https://codereview.chromium.org/2921383002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:321: for (const
auto& element : scene_->GetUiElements()) {
On 2017/06/06 19:42:13, cjgrant wrote:
> Originally I thought it's weird so many tests check which elements are visible
> in normal browsing mode.  But I changed my mind, and I think it does make
sense.
>  If we used the helper method approach, doing so would be a one-liner. 
> Thoughts?

Agreed, we want to check the visibility of these elements with the case that a
unittest covers.

[cjgrant, 2017-06-06 21:21:56]

lgtm

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exit_prompt_backplane.h
(right):

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exit_prompt_backplane.h:13: // An
invisible but hittable plane behind the content quad, to keep the
"behind the exit prompt"  (and again later in the sentence)

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

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:64:
std::set<UiElementDebugId> kElementsVisibleWithExitPrompt = {
If this is only used in one test, it could live there.  Your call.

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:95: void
VerifyElementsVisible(std::set<UiElementDebugId> elements) {
const&

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:97:
SCOPED_TRACE(element->debug_id());
If an element is wrong, does the trace still identify the right point of failure
(ie. the line on which this method is called)?  Do we need nested SCOPED_TRACE()
calls?  Just curious.

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:304:
VerifyElementsVisible({});
Thanks for adjusting all these call sites!  code_size--;

[amp, 2017-06-06 21:42:42]

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

[amp, 2017-06-06 21:42:43]

lgtm

Nice cleanup!

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

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:97:
SCOPED_TRACE(element->debug_id());
On 2017/06/06 21:21:56, cjgrant wrote:
> If an element is wrong, does the trace still identify the right point of
failure
> (ie. the line on which this method is called)?  Do we need nested
SCOPED_TRACE()
> calls?  Just curious.

Yes it fails on a specific line number, but then this scoped_trace will also
output with that failure.

It is immensely helpful in debugging loops (to me at least).

The line number is given, but it also gives you the id of the last item
processed (which you have to look up in the enum, but that's not too hard).

It will tell you exactly what element isn't doing what you expected.

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:228: // In
fullscreen mode, content elements should be visible, control elements
My latest change (about to go in) changes this again, just an FYI.

[amp, 2017-06-06 21:46:22]

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

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:97:
SCOPED_TRACE(element->debug_id());
On 2017/06/06 21:42:43, amp wrote:
> On 2017/06/06 21:21:56, cjgrant wrote:
> > If an element is wrong, does the trace still identify the right point of
> failure
> > (ie. the line on which this method is called)?  Do we need nested
> SCOPED_TRACE()
> > calls?  Just curious.
> 
> Yes it fails on a specific line number, but then this scoped_trace will also
> output with that failure.
> 
> It is immensely helpful in debugging loops (to me at least).
> 
> The line number is given, but it also gives you the id of the last item
> processed (which you have to look up in the enum, but that's not too hard).
> 
> It will tell you exactly what element isn't doing what you expected.

Oh wait, realized the question was if this would show the call site line number.
 No, it won't.

We need to add a SCOPED_TRACE('[test name]') before each call of '
VerifyElementsVisible' and then when something fails here it will tell us which
test failed.

We should also keep this one in the loop as well (you can have multiple in a
test).

[ymalik, 2017-06-07 14:55:59]

folks, ptal :). Few small changes
- Removed friend class and added fooForTesting as per discussion
- Passing a debug string to VerifyElementsVisible

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/exit_prompt_backplane.h
(right):

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/exit_prompt_backplane.h:13: // An
invisible but hittable plane behind the content quad, to keep the
On 2017/06/06 21:21:55, cjgrant wrote:
> "behind the exit prompt"  (and again later in the sentence)

Done.

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

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:64:
std::set<UiElementDebugId> kElementsVisibleWithExitPrompt = {
On 2017/06/06 21:21:56, cjgrant wrote:
> If this is only used in one test, it could live there.  Your call.

Its used by two tests. The primary button click and and the backplane.

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:95: void
VerifyElementsVisible(std::set<UiElementDebugId> elements) {
On 2017/06/06 21:21:56, cjgrant wrote:
> const&

Done.

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:97:
SCOPED_TRACE(element->debug_id());
On 2017/06/06 21:46:22, amp wrote:
> On 2017/06/06 21:42:43, amp wrote:
> > On 2017/06/06 21:21:56, cjgrant wrote:
> > > If an element is wrong, does the trace still identify the right point of
> > failure
> > > (ie. the line on which this method is called)?  Do we need nested
> > SCOPED_TRACE()
> > > calls?  Just curious.
> > 
> > Yes it fails on a specific line number, but then this scoped_trace will also
> > output with that failure.
> > 
> > It is immensely helpful in debugging loops (to me at least).
> > 
> > The line number is given, but it also gives you the id of the last item
> > processed (which you have to look up in the enum, but that's not too hard).
> > 
> > It will tell you exactly what element isn't doing what you expected.
> 
> Oh wait, realized the question was if this would show the call site line
number.
>  No, it won't.
> 
> We need to add a SCOPED_TRACE('[test name]') before each call of '
> VerifyElementsVisible' and then when something fails here it will tell us
which
> test failed.
> 
> We should also keep this one in the loop as well (you can have multiple in a
> test).

Ah good catch.

I don't think SCOPED_TRACE('[test name]') adds much value since we'll already
know which test failed. I think passing in a debug string is better because it
saves us an extra line before the call.

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:228: // In
fullscreen mode, content elements should be visible, control elements
On 2017/06/06 21:42:43, amp wrote:
> My latest change (about to go in) changes this again, just an FYI.

Acknowledged.

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:304:
VerifyElementsVisible({});
On 2017/06/06 21:21:56, cjgrant wrote:
> Thanks for adjusting all these call sites!  code_size--;

Acknowledged.

[cjgrant, 2017-06-07 16:03:07]

lgtm

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

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:97:
SCOPED_TRACE(element->debug_id());
On 2017/06/07 14:55:58, ymalik wrote:
> On 2017/06/06 21:46:22, amp wrote:
> > On 2017/06/06 21:42:43, amp wrote:
> > > On 2017/06/06 21:21:56, cjgrant wrote:
> > > > If an element is wrong, does the trace still identify the right point of
> > > failure
> > > > (ie. the line on which this method is called)?  Do we need nested
> > > SCOPED_TRACE()
> > > > calls?  Just curious.
> > > 
> > > Yes it fails on a specific line number, but then this scoped_trace will
also
> > > output with that failure.
> > > 
> > > It is immensely helpful in debugging loops (to me at least).
> > > 
> > > The line number is given, but it also gives you the id of the last item
> > > processed (which you have to look up in the enum, but that's not too
hard).
> > > 
> > > It will tell you exactly what element isn't doing what you expected.
> > 
> > Oh wait, realized the question was if this would show the call site line
> number.
> >  No, it won't.
> > 
> > We need to add a SCOPED_TRACE('[test name]') before each call of '
> > VerifyElementsVisible' and then when something fails here it will tell us
> which
> > test failed.
> > 
> > We should also keep this one in the loop as well (you can have multiple in a
> > test).
> 
> Ah good catch.
> 
> I don't think SCOPED_TRACE('[test name]') adds much value since we'll already
> know which test failed. I think passing in a debug string is better because it
> saves us an extra line before the call.

I played with this a bit, and I think what you have is optimal.  Docs seem to
suggest adding SCOPED_TRACE calls before every helper function call, but that's
cumbersome too.  This seems better.

[ymalik, 2017-06-07 16:04:17]

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

[commit-bot: I haz the power, 2017-06-07 16:05:16]

Dry run: CQ is trying da patch.

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

[amp, 2017-06-07 17:18:42]

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

https://codereview.chromium.org/2921383002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:97:
SCOPED_TRACE(element->debug_id());
On 2017/06/07 16:03:06, cjgrant wrote:
> On 2017/06/07 14:55:58, ymalik wrote:
> > On 2017/06/06 21:46:22, amp wrote:
> > > On 2017/06/06 21:42:43, amp wrote:
> > > > On 2017/06/06 21:21:56, cjgrant wrote:
> > > > > If an element is wrong, does the trace still identify the right point
of
> > > > failure
> > > > > (ie. the line on which this method is called)?  Do we need nested
> > > > SCOPED_TRACE()
> > > > > calls?  Just curious.
> > > > 
> > > > Yes it fails on a specific line number, but then this scoped_trace will
> also
> > > > output with that failure.
> > > > 
> > > > It is immensely helpful in debugging loops (to me at least).
> > > > 
> > > > The line number is given, but it also gives you the id of the last item
> > > > processed (which you have to look up in the enum, but that's not too
> hard).
> > > > 
> > > > It will tell you exactly what element isn't doing what you expected.
> > > 
> > > Oh wait, realized the question was if this would show the call site line
> > number.
> > >  No, it won't.
> > > 
> > > We need to add a SCOPED_TRACE('[test name]') before each call of '
> > > VerifyElementsVisible' and then when something fails here it will tell us
> > which
> > > test failed.
> > > 
> > > We should also keep this one in the loop as well (you can have multiple in
a
> > > test).
> > 
> > Ah good catch.
> > 
> > I don't think SCOPED_TRACE('[test name]') adds much value since we'll
already
> > know which test failed. I think passing in a debug string is better because
it
> > saves us an extra line before the call.
> 
> I played with this a bit, and I think what you have is optimal.  Docs seem to
> suggest adding SCOPED_TRACE calls before every helper function call, but
that's
> cumbersome too.  This seems better.

Cumbersome, but it's not really useful if you don't call it before the helper
function call (it won't be able to tell which test it was that failed).  See my
comment on latest patch set.

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

https://codereview.chromium.org/2921383002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:254:
VerifyElementsVisible("Restore initial", kElementsVisibleInBrowsing);
If we pass the same string to the scoped trace it won't be able to differentiate
which call it was from (this string looks like it is used on line 233 as well as
in another test below).

Alternatively just call the scoped_trace with the string directly here (instead
of passing it in) and then the scoped trace will output the line number where it
was called (which would tell you which test it was).

[amp, 2017-06-07 17:36:45]

lgtm

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

https://codereview.chromium.org/2921383002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:254:
VerifyElementsVisible("Restore initial", kElementsVisibleInBrowsing);
On 2017/06/07 17:18:41, amp wrote:
> If we pass the same string to the scoped trace it won't be able to
differentiate
> which call it was from (this string looks like it is used on line 233 as well
as
> in another test below).
> 
> Alternatively just call the scoped_trace with the string directly here
(instead
> of passing it in) and then the scoped trace will output the line number where
it
> was called (which would tell you which test it was).

Turns out the test that failed is already output, so duplication across tests is
fine (but within the same test would be hard to debug).

[commit-bot: I haz the power, 2017-06-07 17:50:02]

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

[commit-bot: I haz the power, 2017-06-07 17:50:03]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ymalik, 2017-06-07 18:43:14]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2017-06-07 18:43:48]

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 21:03:37]

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

[commit-bot: I haz the power, 2017-06-07 21:03:39]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ymalik, 2017-06-08 13:50:52]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2017-06-08 13:51:05]

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-08 15:32:28]

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

[commit-bot: I haz the power, 2017-06-08 15:32:30]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ymalik, 2017-06-08 18:09:01]

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

[commit-bot: I haz the power, 2017-06-08 18:09:17]

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-06-08 19:29:11]

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

[commit-bot: I haz the power, 2017-06-08 19:29:12]

Dry run: This issue passed the CQ dry run.

[ymalik, 2017-06-08 19:33:09]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-06-08 19:33:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjgrant@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2921383002/#ps140001
(title: "fix bot")

[commit-bot: I haz the power, 2017-06-08 19:33:54]

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-08 19:39:10]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496950389716130,
"parent_rev": "b0d4cc9b3bbd0a7a32b9c13260fbef440d2978a3", "commit_rev":
"ee13d1180e1625c5d5f6b1ef4b4b90192be20cb9"}

[commit-bot: I haz the power, 2017-06-08 19:39:26]

Description was changed from

==========
[vr] Close exit prompt when clicking on background

This is a follow-up patch to https://codereview.chromium.org/2913633002/

BUG=726744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[vr] Close exit prompt when clicking on background

This is a follow-up patch to https://codereview.chromium.org/2913633002/

BUG=726744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

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

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/ee13d1180e1625c5d5f6b1ef4b4b...