VR: Fix HTTP warning staying visible after exiting WebVR.

VR: Fix HTTP warning staying visible after exiting WebVR.

- Fix the issue.
- Add a previously-discussed way to test element visibility.
- Add tests to make sure this doesn't recur.

BUG=723842
Review-Url: https://codereview.chromium.org/2888283005
Cr-Commit-Position: refs/heads/master@{#473317}
Committed: https://chromium.googlesource.com/chromium/src/+/d01f2e3a7396302e91f56391f4f62e7d2cc0ff68

[cjgrant, 2017-05-18 20:25:18]

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

[cjgrant, 2017-05-18 20:25:19]

Adam, this adds the enum-based identifier system previously discussed, and puts
it to immediate use.

[mthiesse, 2017-05-18 23:47:09]

lgtm

https://codereview.chromium.org/2888283005/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2888283005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:58: //
TODO(mthiesse): When we have UI to test for CCT, we'll need to modify
Seems like you can remove this TODO now?

[cjgrant, 2017-05-19 14:19:12]

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

[cjgrant, 2017-05-19 14:19:13]

Adding Ian to sniff at style, and copying David as we had discussed the bug that
triggered this CL.

https://codereview.chromium.org/2888283005/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2888283005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:58: //
TODO(mthiesse): When we have UI to test for CCT, we'll need to modify
On 2017/05/18 23:47:09, mthiesse wrote:
> Seems like you can remove this TODO now?

Done, and I also added a test for the button. PTAL at that...

[mthiesse, 2017-05-19 14:20:26]

lgtm

[Ian Vollick, 2017-05-19 14:42:32]

lgtm with some nits.

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/ui_element.h (right):

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/ui_element.h:310:
UiElementIdentifier identifier_ = UiElementIdentifier::kNone;
If it's for debugging, could we wrap with #ifndef NDEBUG?

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/ui_element_identifiers.h
(right):

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/ui_element_identifiers.h:15:
kWebVrTransientHttpSecurityWarning,
I love the idea of exposing ID's for testing, but perhaps a UiElement could have
a debug-only virtual property that returns a string? That'd keep the names
closer to the classes and might be nice for output.

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

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene.cc:133: UiElement*
UiScene::GetUiElementByIdentifier(
nit/bikeshed: Id and Identifier are pretty close. Maybe "name"?

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

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:60: enum InCct {
I think you can do enum InCct : bool to specify storage.

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:98:
MakeManager(kNotInCct, kInWebVr);
This probably isn't useful here, but it's kinda neat and I discovered it
recently, so here goes :)

https://g3doc.corp.google.com/third_party/gtest/g3doc/advanced.md#how-to-writ...

[cjgrant, 2017-05-19 15:10:40]

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/ui_element.h (right):

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/ui_element.h:310:
UiElementIdentifier identifier_ = UiElementIdentifier::kNone;
On 2017/05/19 14:42:32, Ian Vollick wrote:
> If it's for debugging, could we wrap with #ifndef NDEBUG?

We originally wanted to use strings for this, but didn't so as to avoid the need
for NDEBUG.  If we use it, we have to wrap the getter/setter, and the scene's
GetElementByIdentifier() method, and maybe other things later (log prints about
elements, maybe).  For the tiny overhead, I'd rather just keep it in production.
 If you feel strongly though, I'll change it.

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/ui_element_identifiers.h
(right):

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/ui_element_identifiers.h:15:
kWebVrTransientHttpSecurityWarning,
On 2017/05/19 14:42:32, Ian Vollick wrote:
> I love the idea of exposing ID's for testing, but perhaps a UiElement could
have
> a debug-only virtual property that returns a string? That'd keep the names
> closer to the classes and might be nice for output.

Continued from previous comment...  As of now, that means a bunch of
conditionals in the scene manager and element classes.  As it is, it seems like
minimal overhead, and something we could use in production if we wanted to.

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

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene.cc:133: UiElement*
UiScene::GetUiElementByIdentifier(
On 2017/05/19 14:42:32, Ian Vollick wrote:
> nit/bikeshed: Id and Identifier are pretty close. Maybe "name"?

Sure.  I'll adapt that when we close on the NDEBUG topic.

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

https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:98:
MakeManager(kNotInCct, kInWebVr);
On 2017/05/19 14:42:32, Ian Vollick wrote:
> This probably isn't useful here, but it's kinda neat and I discovered it
> recently, so here goes :)
> 
>
https://g3doc.corp.google.com/third_party/gtest/g3doc/advanced.md#how-to-writ...

Yep, very handy (used in the scene tests), but not applicable here as the test
bodies differ between CCT and no-CCT cases (and parameterizing the difference is
way harder to read).

[cjgrant, 2017-05-19 19:22:19]

Renamed the debug ID as discussed.

[cjgrant, 2017-05-19 19:22:44]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-05-19 19:22:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2888283005/#ps60001
(title: "Rename to debug_id_ and UiElementDebugId")

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

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-19 21:13:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495221764233240,
"parent_rev": "edd5e5dcf36dabc8e8d9b2169605072d61188171", "commit_rev":
"d01f2e3a7396302e91f56391f4f62e7d2cc0ff68"}

[commit-bot: I haz the power, 2017-05-19 21:14:00]

Description was changed from

==========
VR: Fix HTTP warning staying visible after exiting WebVR.

- Fix the issue.
- Add a previously-discussed way to test element visibility.
- Add tests to make sure this doesn't recur.

BUG=723842
==========

to

==========
VR: Fix HTTP warning staying visible after exiting WebVR.

- Fix the issue.
- Add a previously-discussed way to test element visibility.
- Add tests to make sure this doesn't recur.

BUG=723842

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

[commit-bot: I haz the power, 2017-05-19 21:14:02]

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