Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 2888283005: VR: Fix HTTP warning staying visible after exiting WebVR. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by cjgrant
Modified:
3 months ago
Reviewers:
Ian Vollick, mthiesse, amp
CC:
chromium-reviews, feature-vr-reviews_chromium.org, ddorwin
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fix comment on the ID enum. #

Patch Set 3 : Remove CCT TODO and add a test for it; fix EXPECTs on booleans; cleanup. #

Total comments: 9

Patch Set 4 : Rename to debug_id_ and UiElementDebugId #

Unified diffs Side-by-side diffs Delta from patch set Stats (+119 lines, -10 lines) Patch
M chrome/browser/android/vr_shell/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/android/vr_shell/ui_elements/ui_element.h View 1 2 3 4 chunks +9 lines, -1 line 0 comments Download
A chrome/browser/android/vr_shell/ui_elements/ui_element_debug_id.h View 1 2 3 1 chunk +21 lines, -0 lines 0 comments Download
M chrome/browser/android/vr_shell/ui_scene.h View 1 2 3 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/android/vr_shell/ui_scene.cc View 1 2 3 2 chunks +11 lines, -1 line 0 comments Download
M chrome/browser/android/vr_shell/ui_scene_manager.cc View 1 2 3 6 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc View 1 2 3 3 chunks +68 lines, -7 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 16 (6 generated)
cjgrant
Adam, this adds the enum-based identifier system previously discussed, and puts it to immediate use.
3 months ago (2017-05-18 20:25:19 UTC) #2
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 ...
3 months ago (2017-05-18 23:47:09 UTC) #3
cjgrant
Adding Ian to sniff at style, and copying David as we had discussed the bug ...
3 months ago (2017-05-19 14:19:13 UTC) #5
mthiesse
lgtm
3 months ago (2017-05-19 14:20:26 UTC) #6
Ian Vollick
lgtm with some nits. 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; If ...
3 months ago (2017-05-19 14:42:32 UTC) #7
cjgrant
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 ...
3 months ago (2017-05-19 15:10:40 UTC) #8
cjgrant
Renamed the debug ID as discussed.
3 months ago (2017-05-19 19:22:19 UTC) #9
cjgrant
3 months ago (2017-05-19 19:22:39 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888283005/60001
3 months ago (2017-05-19 19:23:58 UTC) #13
commit-bot: I haz the power
3 months ago (2017-05-19 21:14:02 UTC) #16
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d01f2e3a7396302e91f56391f4f6...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b