Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2112013002:
Allow arc app to lock screen orientation in TouchView/Tablet mode (Closed)
|
Created:
4 years, 5 months ago by oshima Modified:
4 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, kalyank, lhchavez+watch_chromium.org, Matt Giuca, mlamouri+watch-screen-orientation_chromium.org, qsr+mojo_chromium.org, sadrul, tapted, tfarina, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow arc app to lock screen orientation in TouchView/Tablet mode
mojo change:
* Added orientation lock parameter to AppInfo
* Added OnTaskOrientationLockRequested method on AppHost
c++ change:
* Apply orientation lock to arc windows upon Tablet mode entry
* Unset lock and set the rotation back upon Tablet mode exit.
* Apply the rotation lock when a task requested. This is kept only on memory and does not persist.
BUG=626445
,b/28341028
TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock
Committed: https://crrev.com/342f3485150be7e10404671e811d9c881f8b62ea
Cr-Commit-Position: refs/heads/master@{#404244}
Patch Set 1 : . #
Total comments: 6
Patch Set 2 : addressed comments, fixed unit tests #Patch Set 3 : unit test #
Total comments: 4
Patch Set 4 : rebase #
Total comments: 1
Patch Set 5 : Allow arc app to lock screen orientation in TouchView/Tablet mode #Messages
Total messages: 63 (32 generated)
Patchset #3 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Description was changed from ========== Allow arc app to lock screen orientation in TouchView/Tablet mode BUG=b/28341028 ========== to ========== Allow arc app to lock screen orientation in TouchView/Tablet mode BUG=b/28341028 ==========
oshima@chromium.org changed reviewers: + reveman@chromium.org, xiyuan@chromium.org, yusukes@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Allow arc app to lock screen orientation in TouchView/Tablet mode BUG=b/28341028 ========== to ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation ock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=b/28341028 TEST=TBD ==========
yusukes -> mojo xiyuan -> app_list change reveman -> all still working on tests but the rest is ready.
Description was changed from ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation ock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=b/28341028 TEST=TBD ========== to ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=b/28341028 TEST=TBD ==========
https://codereview.chromium.org/2112013002/diff/60001/ash/display/screen_orie... File ash/display/screen_orientation_controller_chromeos.cc (right): https://codereview.chromium.org/2112013002/diff/60001/ash/display/screen_orie... ash/display/screen_orientation_controller_chromeos.cc:121: SetDisplayRotation(user_rotation_, display::Display::ROTATION_SOURCE_USER); We're unlocking all, not just arc windows, but I believe this is what we want. If this works for arc++ enabled devices, I'll merge this to OnMaximizeModeEnded below.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2112013002/diff/60001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2112013002/diff/60001/components/arc/common/a... components/arc/common/app.mojom:12: enum OrientationLock { Do you envision adding more stuff to this enum? (Maybe upside-down to force no rotation taking place?) If so, add [Extensible]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2112013002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2112013002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:719: FOR_EACH_OBSERVER(Observer, observer_list_, So this changes the orientation_lock for a task. What is the use of the orientation_lock in AppInfo? Is it needed at all? If it is needed, then do we need to update the cached AppInfo in pref here?
https://codereview.chromium.org/2112013002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2112013002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:719: FOR_EACH_OBSERVER(Observer, observer_list_, On 2016/06/30 23:25:26, xiyuan wrote: > So this changes the orientation_lock for a task. What is the use of the > orientation_lock in AppInfo? Is it needed at all? If it is needed, then do we > need to update the cached AppInfo in pref here? You can specify the orientation in manifest (which will be in the AppInfo), or in the code at runtime (which will be stored in AppWindow). The former is persistent, while the latter is dynamic (therefore should not be in AppInfo) and will overwrite the old (manifest, or previous request) value. https://codereview.chromium.org/2112013002/diff/60001/components/arc/common/a... File components/arc/common/app.mojom (right): https://codereview.chromium.org/2112013002/diff/60001/components/arc/common/a... components/arc/common/app.mojom:12: enum OrientationLock { On 2016/06/30 23:01:47, Luis Héctor Chávez wrote: > Do you envision adding more stuff to this enum? (Maybe upside-down to force no > rotation taking place?) > > If so, add [Extensible] I'm not sure, and since I'm not sure, I added it :) Thank you for the suggestion!
The CQ bit was checked by oshima@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...
Actually components/arc/OWNERS do not own components/arc/common/*.mojom files because mojom changes require a security review. Please add rickyz@ instead.
oshima@chromium.org changed reviewers: + rickyz@chromium.org - yusukes@chromium.org
On 2016/07/01 00:48:19, Yusuke Sato wrote: > Actually components/arc/OWNERS do not own components/arc/common/*.mojom files > because mojom changes require a security review. Please add rickyz@ instead. ah, sorry about that. rickyz@ -> app.mojom
app_list lgtm https://codereview.chromium.org/2112013002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2112013002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:719: FOR_EACH_OBSERVER(Observer, observer_list_, On 2016/07/01 00:44:47, oshima wrote: > On 2016/06/30 23:25:26, xiyuan wrote: > > So this changes the orientation_lock for a task. What is the use of the > > orientation_lock in AppInfo? Is it needed at all? If it is needed, then do we > > need to update the cached AppInfo in pref here? > > You can specify the orientation in manifest (which will be in the AppInfo), or > in the code at runtime (which will be stored in AppWindow). > > The former is persistent, while the latter is dynamic (therefore should not be > in AppInfo) and will overwrite the old (manifest, or previous request) value. Thanks for the clarification.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=b/28341028 TEST=TBD ========== to ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock ==========
Added unit tests. PTAL
mojom lgtm https://codereview.chromium.org/2112013002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2112013002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:585: void ArcAppListPrefs::AddAppAndShortcut( Feel free to leave as is, but maybe it's time to make this function take a struct instead. https://codereview.chromium.org/2112013002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2112013002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:217: bool has_requested_orientation_lock_ = false; I'm not familiar with the desired behavior here, but would it be sufficient to use requested_orientation_lock_ == arc::mojom::OrientationLock::NONE instead this variable?
https://codereview.chromium.org/2112013002/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2112013002/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:585: void ArcAppListPrefs::AddAppAndShortcut( On 2016/07/02 00:10:37, rickyz wrote: > Feel free to leave as is, but maybe it's time to make this function take a > struct instead. I'm happy to change it but let me do that in a separate CL. https://codereview.chromium.org/2112013002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2112013002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:217: bool has_requested_orientation_lock_ = false; On 2016/07/02 00:10:37, rickyz wrote: > I'm not familiar with the desired behavior here, but would it be sufficient to > use requested_orientation_lock_ == arc::mojom::OrientationLock::NONE instead > this variable? No, because NONE will unlock the orientation, which was set by manifest.
The CQ bit was checked by oshima@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by oshima@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Patchset #4 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by oshima@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...
components/exo lgtm
still lgtm https://codereview.chromium.org/2112013002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/2112013002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:205: // ArcAppPinPolicy); nit: remove the commented out lines?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by oshima@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock ========== to ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=626445, b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock ==========
Description was changed from ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=626445, b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock ========== to ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=626445,b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock ==========
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org, xiyuan@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2112013002/#ps180001 (title: "Allow arc app to lock screen orientation in TouchView/Tablet mode")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=626445,b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock ========== to ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=626445,b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=626445,b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock ========== to ========== Allow arc app to lock screen orientation in TouchView/Tablet mode mojo change: * Added orientation lock parameter to AppInfo * Added OnTaskOrientationLockRequested method on AppHost c++ change: * Apply orientation lock to arc windows upon Tablet mode entry * Unset lock and set the rotation back upon Tablet mode exit. * Apply the rotation lock when a task requested. This is kept only on memory and does not persist. BUG=626445,b/28341028 TEST=ChromeLauncherControllerImplTest.ArcOrientatinoLock Committed: https://crrev.com/342f3485150be7e10404671e811d9c881f8b62ea Cr-Commit-Position: refs/heads/master@{#404244} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/342f3485150be7e10404671e811d9c881f8b62ea Cr-Commit-Position: refs/heads/master@{#404244} |
