Adam, this adds the enum-based identifier system previously discussed, and puts it to immediate use.
11 months, 1 week ago
(2017-05-18 20:25:19 UTC)
#2
Adam, this adds the enum-based identifier system previously discussed, and puts
it to immediate use.
mthiesse
lgtm https://codereview.chromium.org/2888283005/diff/1/chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2888283005/diff/1/chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc#newcode58 chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:58: // TODO(mthiesse): When we have UI to test ...
11 months, 1 week ago
(2017-05-18 23:47:09 UTC)
#3
https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/vr_shell/ui_elements/ui_element.h File chrome/browser/android/vr_shell/ui_elements/ui_element.h (right): https://codereview.chromium.org/2888283005/diff/40001/chrome/browser/android/vr_shell/ui_elements/ui_element.h#newcode310 chrome/browser/android/vr_shell/ui_elements/ui_element.h:310: UiElementIdentifier identifier_ = UiElementIdentifier::kNone; On 2017/05/19 14:42:32, Ian Vollick ...
11 months, 1 week ago
(2017-05-19 15:10:40 UTC)
#8
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
Renamed the debug ID as discussed.
11 months, 1 week ago
(2017-05-19 19:22:19 UTC)
#9
Renamed the debug ID as discussed.
cjgrant
11 months, 1 week ago
(2017-05-19 19:22:39 UTC)
#10
cjgrant
The CQ bit was checked by cjgrant@chromium.org
11 months, 1 week ago
(2017-05-19 19:22:44 UTC)
#11
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495221764233240, "parent_rev": "edd5e5dcf36dabc8e8d9b2169605072d61188171", "commit_rev": "d01f2e3a7396302e91f56391f4f62e7d2cc0ff68"}
11 months, 1 week ago
(2017-05-19 21:13:47 UTC)
#14
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
Description was changed from ========== VR: Fix HTTP warning staying visible after exiting WebVR. - ...
11 months, 1 week ago
(2017-05-19 21:14:00 UTC)
#15
Message was sent while issue was closed.
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
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d01f2e3a7396302e91f56391f4f62e7d2cc0ff68
11 months, 1 week ago
(2017-05-19 21:14:02 UTC)
#16
Issue 2888283005: VR: Fix HTTP warning staying visible after exiting WebVR.
(Closed)
Created 11 months, 1 week ago by cjgrant
Modified 11 months, 1 week ago
Reviewers: amp, mthiesse, Ian Vollick
Base URL:
Comments: 11