Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1109553003:
Test chrome.system.display during TouchView (Closed)
|
Created:
5 years, 8 months ago by jonross Modified:
5 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTest chrome.system.display during TouchView
Verify the behaviour of chrome.system.display.setDisplayProperties, while in
TouchView mode. Changes applied to the screen rotation should enable the
rotation lock, which would prevent accelerometer rotations.
TEST=DisplayInfoProviderChromeosTest.SetRotationDuringMaximizeMode
BUG=358900
Committed: https://crrev.com/e560642ea9247493e4c260b93888d61f9ccb722a
Cr-Commit-Position: refs/heads/master@{#327397}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Messages
Total messages: 16 (5 generated)
Patchset #1 (id:1) has been deleted
jonross@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, Could you provide an initial review for this change? I'm adding an additional test to DisplayInfoProviderChromeOS to verify how its rotation changes interact with TouchView. Thanks, Jon
https://codereview.chromium.org/1109553003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1109553003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:732: Can you add a comment that entering maximized mode unlocks screen rotation? https://codereview.chromium.org/1109553003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:750: } I think that it would also be worth having a test where DisplayInfoProvider::SetInfo() is called prior to entering maximized mode because the behavior there is non-obvious.
https://codereview.chromium.org/1109553003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1109553003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:732: On 2015/04/24 17:27:59, pkotwicz wrote: > Can you add a comment that entering maximized mode unlocks screen rotation? Done. https://codereview.chromium.org/1109553003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:750: } On 2015/04/24 17:27:59, pkotwicz wrote: > I think that it would also be worth having a test where > DisplayInfoProvider::SetInfo() is called prior to entering maximized mode > because the behavior there is non-obvious. Done.
LGTM with nit https://codereview.chromium.org/1109553003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1109553003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:751: // screen rotation. How about: "Rotation lock should not activate because DisplayInfoProvider::SetInfo() was called when not in maximized mode." https://codereview.chromium.org/1109553003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:767: // against accelerometer rotations. Nit: "Maximize Mode" -> "maximize mode"
https://codereview.chromium.org/1109553003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1109553003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:751: // screen rotation. On 2015/04/27 17:44:39, pkotwicz wrote: > How about: "Rotation lock should not activate because > DisplayInfoProvider::SetInfo() was called when not in maximized mode." Done. https://codereview.chromium.org/1109553003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:767: // against accelerometer rotations. On 2015/04/27 17:44:39, pkotwicz wrote: > Nit: "Maximize Mode" -> "maximize mode" Done.
jonross@chromium.org changed reviewers: + kalman@chromium.org
kalman@chromium.org: Please review changes in chrome/browser/extensions/display_info_provider_chromeos_unittest.cc I've added two new tests to confirm existing behaviour of DisplayInfoProvider with Chrome OS maximize mode. Thanks.
lgtm, I don't know anything about this stuff. https://codereview.chromium.org/1109553003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1109553003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:32: void SetUp() override { isn't there a SetUpCommandLine override or something, or am I thinking of a different test framework?
https://codereview.chromium.org/1109553003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1109553003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:32: void SetUp() override { On 2015/04/28 22:04:56, kalman wrote: > isn't there a SetUpCommandLine override or something, or am I thinking of a > different test framework? That is in PPAPI and some browser tests. No one ever wrote one for AshTestBase.
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1109553003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1109553003/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e560642ea9247493e4c260b93888d61f9ccb722a Cr-Commit-Position: refs/heads/master@{#327397} |
