Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903673002:
VR: Update UiSceneManager with screen capturing flag (Closed)
DescriptionVR: Update UiSceneManager with screen capturing flag
UiSceneManager is updated to receive the screen capturing flag
and show the indicator accordingly.
BUG=722836
Review-Url: https://codereview.chromium.org/2903673002
Cr-Commit-Position: refs/heads/master@{#475061}
Committed: https://chromium.googlesource.com/chromium/src/+/5a12ab7ad16ad3e801b3182270b1e5b2956f7dc9
Patch Set 1 #
Total comments: 1
Patch Set 2 : VR: Update UiSceneManager with screen capturing flag #Patch Set 3 : UI webvr unittest is added #
Total comments: 14
Patch Set 4 : VR: Update UiSceneManager with screen capturing flag #
Total comments: 4
Patch Set 5 : VR: Update UiSceneManager with screen capturing flag #
Total comments: 4
Patch Set 6 : VR: Update UiSceneManager with screen capturing flag #Patch Set 7 : Indicators tests are added. Update the flags only when they change #
Total comments: 3
Messages
Total messages: 26 (10 generated)
asimjour@chromium.org changed reviewers: + cjgrant@chromium.org
PTAL
https://codereview.chromium.org/2903673002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/ui_scene_manager.cc (right): https://codereview.chromium.org/2903673002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/ui_scene_manager.cc:362: screen_capture_indicator_->set_visible(enabled); I think you need to cover the WebVR case, where these would be head-locked and positioned somewhere relative to the field of view (rather than world). You'll probably want to do something like the security warnings, where the elements are configured based on both the "enabled" flag being set, and the webvr flag being set. Same question for full-screen mode I guess (Adam is perpetually fixing full-screen mode). :)
PTAL
PTAL
cjgrant@chromium.org changed reviewers: + amp@chromium.org
Adam, please see my comments on the unit test. Feel free to disagree. :) https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:203: TEST_F(UiSceneManagerTest, UiUpdatesForWebVRChanges) { I'm not sure it makes sense to include browser-mode here as well. Ie, I could see two tests, one that starts in browser and changes to WebVR, and one that starts in WebVR, but I don't think they should check *what* is visible in browser mode, only that nothing is visible in WebVR. The fullscreen test above already covers regular mode, and this test is therefore duplicating work (and the list of elements). I still recommend the original suggestion of entering WebVR and making sure nothing is visible, that's it. Adam, thoughts? https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:221: // Everything should be visible by default. As above, can skip. https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:231: manager_->SetAudioCapturingIndicator(true); No need to re-set these flags. https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:237: SCOPED_TRACE(element->debug_id()); All elements should be invisible, so there's no logic needed here other than "EXPECT_FALSE(element->visible()) https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:244: // Exit webvr mode. And, no need for this block.
https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:203: TEST_F(UiSceneManagerTest, UiUpdatesForWebVRChanges) { On 2017/05/25 23:02:56, cjgrant wrote: > I'm not sure it makes sense to include browser-mode here as well. Ie, I could > see two tests, one that starts in browser and changes to WebVR, and one that > starts in WebVR, but I don't think they should check *what* is visible in > browser mode, only that nothing is visible in WebVR. > > The fullscreen test above already covers regular mode, and this test is > therefore duplicating work (and the list of elements). I still recommend the > original suggestion of entering WebVR and making sure nothing is visible, that's > it. Adam, thoughts? I was thinking about this as well. We could split this into one large test that verifies visibility in all the modes, or split it into smaller tests that verify specific things. I think either way could work. The way the tests are run I think it would be clear which element fails (as the tests don't fail at the first incorrect assertion, but check them all). Otherwise I would say we should have smaller tests so we can see what fails easier. For WebVr specifically I would like to see two variations: one that starts in WebVR and makes sure nothing is visible and then transitions to normal and make sure they show up, and a second one that starts in normal mode, verifies things are showing up, and then transitions to WebVR and verifies they are gone (and maybe back again). I suppose it might get a little verbose though with all the different variations. As long as everyone feels good about the test coverage in general we probably don't need to make sure every possible transition is covered. (for instance, we can probably get by without a test transitioning from fullscreen to WebVR, but then again maybe those are exactly the kind of scenario's most likely to go wrong). https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:237: SCOPED_TRACE(element->debug_id()); On 2017/05/25 23:02:56, cjgrant wrote: > All elements should be invisible, so there's no logic needed here other than > "EXPECT_FALSE(element->visible()) +1 https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:244: // Exit webvr mode. On 2017/05/25 23:02:56, cjgrant wrote: > And, no need for this block. This one I would argue it's nice to verify the return transition and make sure things show up again.
https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:203: TEST_F(UiSceneManagerTest, UiUpdatesForWebVRChanges) { On 2017/05/25 23:02:56, cjgrant wrote: > I'm not sure it makes sense to include browser-mode here as well. Ie, I could > see two tests, one that starts in browser and changes to WebVR, and one that > starts in WebVR, but I don't think they should check *what* is visible in > browser mode, only that nothing is visible in WebVR. > > The fullscreen test above already covers regular mode, and this test is > therefore duplicating work (and the list of elements). I still recommend the > original suggestion of entering WebVR and making sure nothing is visible, that's > it. Adam, thoughts? Done. https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:221: // Everything should be visible by default. On 2017/05/25 23:02:56, cjgrant wrote: > As above, can skip. Done. https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:231: manager_->SetAudioCapturingIndicator(true); On 2017/05/25 23:02:56, cjgrant wrote: > No need to re-set these flags. Done. https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:237: SCOPED_TRACE(element->debug_id()); On 2017/05/25 23:02:56, cjgrant wrote: > All elements should be invisible, so there's no logic needed here other than > "EXPECT_FALSE(element->visible()) Done. https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:244: // Exit webvr mode. On 2017/05/25 23:02:56, cjgrant wrote: > And, no need for this block. Done.
lgtm with a couple last nits. https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:244: // Exit webvr mode. On 2017/05/25 23:33:32, asimjour1 wrote: > On 2017/05/25 23:02:56, cjgrant wrote: > > And, no need for this block. > > Done. Adam, could go either way, but with unit tests, it feels cleaner to me to focus on a single mode at a time, with variants for the ways into a particular mode (this could be parameterized if we were ambitious). That was my motivation anyway. The browser mode test itself could be one big test that activates warnings, or many small tests... https://codereview.chromium.org/2903673002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:217: std::set<UiElementDebugId> visible_in_browsing = { If the text is going to check this set of elements, it should be declared at module scope and shared.
Correction, not LGTM. See comments. https://codereview.chromium.org/2903673002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager.cc (right): https://codereview.chromium.org/2903673002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager.cc:398: audio_capture_indicator_->set_visible(enabled && !web_vr_mode_); Actually, wait, this is broken, isn't it? This state has to be updated when we actually enter and exit WebVR. https://codereview.chromium.org/2903673002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:204: MakeManager(kNotInCct, kInWebVr); This line is initializing a scene manager in WebVR mode, then the line below transitions into WebVR mode. Can you please add the two tests as suggested, one that starts out of webvr with the indicators enabled, then transitions in, and one that starts in webvr, enables the indicators, and makes sure they don't show up? That will verify the two bits of logic needed by the scene manager to control visibility. https://codereview.chromium.org/2903673002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:230: manager_->SetAudioCapturingIndicator(true); Miscommunication. The point of adding this test is to verify that these indicators do not show in WebVR. But here, they are no longer being set while in WebVR. So therefore, the test is not catching the fact that the logic is broken. Or, I am confused.
https://codereview.chromium.org/2903673002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager.cc (right): https://codereview.chromium.org/2903673002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager.cc:346: if (web_vr) { More detailed explanation: The way this is coded, it relies on VrShell to make repeated calls (currently done due to polling). We should fix the repeated calls to only update scene manager when a state changes, but even then, we need unit tests to work and can't have elements flickering in and out of site. :) And also, without polling, this code wouldn't restore indicator visibility when exiting webvr. So this code should be replaced with a helper method just like ConfigureSecurityWarnings(). In there, it's just: capture_indicator->set_visible(!webvr_ && indicator_enabled_); And you'd call that whenever either of those two values updates. https://codereview.chromium.org/2903673002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:203: TEST_F(UiSceneManagerTest, UiUpdatesForWebVRChanges) { I put a proposed test structure here. Let me know what you think! https://paste.googleplex.com/5593044975878144
https://codereview.chromium.org/2903673002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager.cc (right): https://codereview.chromium.org/2903673002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager.cc:346: if (web_vr) { On 2017/05/26 14:08:15, cjgrant wrote: > More detailed explanation: > > The way this is coded, it relies on VrShell to make repeated calls (currently > done due to polling). We should fix the repeated calls to only update scene > manager when a state changes, but even then, we need unit tests to work and > can't have elements flickering in and out of site. :) And also, without > polling, this code wouldn't restore indicator visibility when exiting webvr. > > So this code should be replaced with a helper method just like > ConfigureSecurityWarnings(). In there, it's just: > > capture_indicator->set_visible(!webvr_ && indicator_enabled_); > > And you'd call that whenever either of those two values updates. > Updated according to the offline discussion https://codereview.chromium.org/2903673002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:203: TEST_F(UiSceneManagerTest, UiUpdatesForWebVRChanges) { On 2017/05/26 14:08:15, cjgrant wrote: > I put a proposed test structure here. Let me know what you think! > > https://paste.googleplex.com/5593044975878144 Done.
lgtm https://codereview.chromium.org/2903673002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:232: } Should we test that transitioning out of WebVR makes everything visible here? This is kind of already verified by the test below (line 254 and after), but that test starts with WebVR disabled, enables it and then disables it, where this test starts with it enabled and then disables it. There is a possibility that they could be hitting a different path that way. OTOH feel free to ignore me if you feel this is too pedantic. :)
The CQ bit was checked by asimjour@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Thanks for retooling the tests Amir! https://codereview.chromium.org/2903673002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:232: } On 2017/05/26 17:26:01, amp wrote: > Should we test that transitioning out of WebVR makes everything visible here? > > This is kind of already verified by the test below (line 254 and after), but > that test starts with WebVR disabled, enables it and then disables it, where > this test starts with it enabled and then disables it. There is a possibility > that they could be hitting a different path that way. > > OTOH feel free to ignore me if you feel this is too pedantic. :) I still feel the same about this as before. I think testing non-WebVR visibility is a bigger problem, and should be separate tests. These tests are just meant to cover "wherever you were before, when you get into WebVR, nothing should show". That a different intent that Amir's test below, which verifies the indicators across multiple modes. So my opinion is that this is good as is, and when we have time, we should add a "regular browsing" test that does heavy testing of the browsing elements, goes into WebVR and back out, reverifies, etc"..
https://codereview.chromium.org/2903673002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right): https://codereview.chromium.org/2903673002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:232: } On 2017/05/26 17:33:35, cjgrant wrote: > On 2017/05/26 17:26:01, amp wrote: > > Should we test that transitioning out of WebVR makes everything visible here? > > > > This is kind of already verified by the test below (line 254 and after), but > > that test starts with WebVR disabled, enables it and then disables it, where > > this test starts with it enabled and then disables it. There is a possibility > > that they could be hitting a different path that way. > > > > OTOH feel free to ignore me if you feel this is too pedantic. :) > > I still feel the same about this as before. I think testing non-WebVR > visibility is a bigger problem, and should be separate tests. These tests are > just meant to cover "wherever you were before, when you get into WebVR, nothing > should show". That a different intent that Amir's test below, which verifies > the indicators across multiple modes. > > So my opinion is that this is good as is, and when we have time, we should add a > "regular browsing" test that does heavy testing of the browsing elements, goes > into WebVR and back out, reverifies, etc".. That makes sense. I agree a single large test that goes through all the modes makes more sense that adding those parts piecemeal into each of the other parts. So yea this is good as is, we'll revisit those bigger tests in a future change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by asimjour@chromium.org
The CQ bit was checked by asimjour@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495822523849620,
"parent_rev": "aa4fdf26a8bb80fc51e06a3f7f8ce281fee23f35", "commit_rev":
"5a12ab7ad16ad3e801b3182270b1e5b2956f7dc9"}
Message was sent while issue was closed.
Description was changed from ========== VR: Update UiSceneManager with screen capturing flag UiSceneManager is updated to receive the screen capturing flag and show the indicator accordingly. BUG=722836 ========== to ========== VR: Update UiSceneManager with screen capturing flag UiSceneManager is updated to receive the screen capturing flag and show the indicator accordingly. BUG=722836 Review-Url: https://codereview.chromium.org/2903673002 Cr-Commit-Position: refs/heads/master@{#475061} Committed: https://chromium.googlesource.com/chromium/src/+/5a12ab7ad16ad3e801b3182270b1... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/5a12ab7ad16ad3e801b3182270b1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
