Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2720913003:
Calls display_configuration_controller to rotate screen in accelerator_controller_delegate_aura (Closed)
DescriptionCalls display_configuration_controller to rotate screen in accelerator_controller_delegate_aura
This unifies the code path calling ScreenRotationAnimator::Rotate through display_configuration_controller SetDisplayRotation. Further screen rotation animation improvement will base on this change.
BUG=678763
TEST=manual, unit_tests
R=bruthig@chromium.org, oshima@chromium.org
Review-Url: https://codereview.chromium.org/2720913003
Cr-Commit-Position: refs/heads/master@{#454288}
Committed: https://chromium.googlesource.com/chromium/src/+/4b326a561bf0feaae081fe6f2037cde4d25a8fd9
Patch Set 1 #
Total comments: 14
Patch Set 2 : Calls display_configuration_controller to rotate screen in accelerator_controller_delegate_aura #
Total comments: 11
Patch Set 3 : Calls display_configuration_controller to rotate screen in accelerator_controller_delegate_aura #
Total comments: 4
Patch Set 4 : Change the descriptions in actions.xml #
Total comments: 1
Patch Set 5 : Last patch for commit #
Messages
Total messages: 44 (25 generated)
The CQ bit was checked by afakhry@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: 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/2720913003/diff/1/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:530: TEST_F(AcceleratorControllerTest, RotateScreen) { You probably need to disable this test on mash_unittests.
Hi Oshima and Ben, Could you please look at the CL. The test case does not pass the "linux_chromium_chromeos_ozone_rel_ng". Do you know any reason why? Maybe no implementation of the ScreenRotation in mash? Ahmend commented to "disable this test on mash_unittests". I will fix it. Thanks, Tao
Overall looking good. Left a few requests and nits. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller_delegate_aura.cc (right): https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_delegate_aura.cc:31: #include "ash/rotator/screen_rotation_animator.h" nit: Remove this include https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_delegate_aura.cc:169: Shell::GetInstance()->display_configuration_controller()->SetDisplayRotation( Do any SetDisplayRotation() call sites even pass false for the |user_action|? If not feel free to remove that param ;) https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:532: // Will it alway return a valid primary display? I believe you can assume GetPrimaryDisplay() will always return a valid value within AshTestBase tests. If you really aren't sure, I would use an ASSERT instead of an early return. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:545: // new_rotation depends on the implementation of GetNextRotation in nit: replace 'new_rotation' with '|new_rotation|' https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:546: // accelerator_controller_delegate_aura.cc and the initial rotation nit: - You don't need to specifically call out the Aura flavor of the AcceleratorControllerDelegate. Just say in general that the next animation is determined by the AcceleratorControllerDelegate. - End the sentence with a period. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:547: // The best test here is to check they are not equal. nit: This comment isn't necessary.
Responded to Ahmed and Ben's comments. Will upload another patch soon. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller_delegate_aura.cc (right): https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_delegate_aura.cc:31: #include "ash/rotator/screen_rotation_animator.h" On 2017/02/28 19:11:46, bruthig wrote: > nit: Remove this include Done. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_delegate_aura.cc:169: Shell::GetInstance()->display_configuration_controller()->SetDisplayRotation( On 2017/02/28 19:11:46, bruthig wrote: > Do any SetDisplayRotation() call sites even pass false for the |user_action|? > If not feel free to remove that param ;) All 3 call sites pass true, therefore remove the param. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:530: TEST_F(AcceleratorControllerTest, RotateScreen) { On 2017/02/28 02:51:53, afakhry wrote: > You probably need to disable this test on mash_unittests. Done. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:532: // Will it alway return a valid primary display? On 2017/02/28 19:11:46, bruthig wrote: > I believe you can assume GetPrimaryDisplay() will always return a valid value > within AshTestBase tests. If you really aren't sure, I would use an ASSERT > instead of an early return. Removed the check. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:545: // new_rotation depends on the implementation of GetNextRotation in On 2017/02/28 19:11:47, bruthig wrote: > nit: replace 'new_rotation' with '|new_rotation|' Done. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:546: // accelerator_controller_delegate_aura.cc and the initial rotation On 2017/02/28 19:11:47, bruthig wrote: > nit: > - You don't need to specifically call out the Aura flavor of the > AcceleratorControllerDelegate. Just say in general that the next animation is > determined by the AcceleratorControllerDelegate. > - End the sentence with a period. Done. https://codereview.chromium.org/2720913003/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:547: // The best test here is to check they are not equal. On 2017/02/28 19:11:47, bruthig wrote: > nit: This comment isn't necessary. Done.
The CQ bit was checked by wutao@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.
wutao@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven, I modified two files you owned. Could you please look at: chrome/browser/extensions/display_info_provider_chromeos.cc chrome/browser/ui/webui/options/chromeos/display_options_handler.cc Hi Ahmed, Ben, Oshima, Could you please look the cl again. Thanks, Tao
lgtm https://codereview.chromium.org/2720913003/diff/20001/ash/accelerators/accele... File ash/accelerators/accelerator_controller_delegate_aura.cc (right): https://codereview.chromium.org/2720913003/diff/20001/ash/accelerators/accele... ash/accelerators/accelerator_controller_delegate_aura.cc:162: base::RecordAction(UserMetricsAction("Accel_Rotate_Screen")); Good eye! https://codereview.chromium.org/2720913003/diff/20001/ash/accelerators/accele... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2720913003/diff/20001/ash/accelerators/accele... ash/accelerators/accelerator_controller_unittest.cc:530: TEST_F(AcceleratorControllerTest, RotateScreen) { Very nice test! https://codereview.chromium.org/2720913003/diff/20001/ash/display/display_con... File ash/display/display_configuration_controller.h (left): https://codereview.chromium.org/2720913003/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.h:56: bool user_action); Thx for removing this. https://codereview.chromium.org/2720913003/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2720913003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:762: <description>Please enter the description of the metric.</description> nit: Please document according to: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/actio... Admittedly it might be hard to add a good <owner> tho. https://codereview.chromium.org/2720913003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:767: <description>Please enter the description of this user action.</description> Can you please add a note here that 'Accel_Rotate_Window' also included screen rotation accelerator events prior to M58 (assuming it makes it into the M58 release).
https://codereview.chromium.org/2720913003/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2720913003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:762: <description>Please enter the description of the metric.</description> On 2017/02/28 22:49:22, bruthig wrote: > nit: Please document according to: > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/actio... > > Admittedly it might be hard to add a good <owner> tho. I will add myself to be the owner. Please feel free to add/change later. https://codereview.chromium.org/2720913003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:767: <description>Please enter the description of this user action.</description> On 2017/02/28 22:49:22, bruthig wrote: > Can you please add a note here that 'Accel_Rotate_Window' also included screen > rotation accelerator events prior to M58 (assuming it makes it into the M58 > release). Will add the note. It could be M59 depends when I get all the lgtm.
lgtm https://codereview.chromium.org/2720913003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (left): https://codereview.chromium.org/2720913003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:499: display::Display::ROTATION_SOURCE_ACTIVE, true /* user_action */); Thanks, we changed this (used to be false) when we decided to use the API to implement setting but didn't notice that we could clean this up. https://codereview.chromium.org/2720913003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2720913003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:478: display_id, true /* user_action */); I believe all these user_actions aren't necessary either. Can you clean them up in a separate CL?
https://codereview.chromium.org/2720913003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (left): https://codereview.chromium.org/2720913003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:499: display::Display::ROTATION_SOURCE_ACTIVE, true /* user_action */); On 2017/03/01 00:47:31, oshima wrote: > Thanks, we changed this (used to be false) when we decided to use the API to > implement setting but didn't notice that we could clean this up. Acknowledged. https://codereview.chromium.org/2720913003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2720913003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:478: display_id, true /* user_action */); On 2017/03/01 00:47:31, oshima wrote: > I believe all these user_actions aren't necessary either. Can you clean them up > in a separate CL? Will clean this up in a separate CL.
lgtm
wutao@chromium.org changed reviewers: + mpearson@google.com, reillyg@google.com
Hi Mark, Could you please look at tools/metrics/actions/actions.xml Hi Reilly, Could you please look at chrome/browser/extensions/display_info_provider_chromeos.cc Thanks, Tao
The CQ bit was checked by wutao@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...
reillyg@chromium.org changed reviewers: + reillyg@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
https://codereview.chromium.org/2720913003/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2720913003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:763: Metric recorded when the user rotate the screen using keyboard shortcut. nit: rotate -> rotates nit: using -> "using a" or "using the" https://codereview.chromium.org/2720913003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:764: "Accel_Rotate_Window" also included screen rotation accelerator This sentence is unclear. Did you mean to put (a clearer version of it) in the Accel_Rotate_Window description?
wutao@chromium.org changed reviewers: - mpearson@google.com, reillyg@google.com
https://codereview.chromium.org/2720913003/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2720913003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:763: Metric recorded when the user rotate the screen using keyboard shortcut. On 2017/03/02 00:05:26, Mark P wrote: > nit: rotate -> rotates > nit: using -> "using a" or "using the" Done. https://codereview.chromium.org/2720913003/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:764: "Accel_Rotate_Window" also included screen rotation accelerator On 2017/03/02 00:05:26, Mark P wrote: > This sentence is unclear. Did you mean to put (a clearer version of it) in the > Accel_Rotate_Window description? No. The original rotating screen code used a wrong metric "Accel_Rotate_Window". I am correcting it. So I created a new record "Accel_Rotate_Screen" and the description is show that prior to M58, "Accel_Rotate_Window" metric included some of user action of screen rotation. How about I change it to: This record was created in M58. All user actions under this category prior to M58 were all included in "Accel_Rotate_Window"
Hi Mark, Could you please look at the actions.xml again. tools/metrics/actions/actions.xml Thanks, Tao
actions.xml lgtm, modulo one possible comment Thanks for getting this situation cleaned up properly! --mark https://codereview.chromium.org/2720913003/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2720913003/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:783: Metric recorded when the user rotates screen and active window using the nit: and -> or ?
On 2017/03/02 05:01:00, Mark P wrote: > actions.xml lgtm, modulo one possible comment > > Thanks for getting this situation cleaned up properly! > > --mark > > https://codereview.chromium.org/2720913003/diff/60001/tools/metrics/actions/a... > File tools/metrics/actions/actions.xml (right): > > https://codereview.chromium.org/2720913003/diff/60001/tools/metrics/actions/a... > tools/metrics/actions/actions.xml:783: Metric recorded when the user rotates > screen and active window using the > nit: and -> or ? Done.
The CQ bit was checked by wutao@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.
The CQ bit was checked by wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, oshima@chromium.org, bruthig@chromium.org, reillyg@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2720913003/#ps80001 (title: "Last patch for commit")
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": 80001, "attempt_start_ts": 1488474140079360,
"parent_rev": "142dd646d8d7b7db333114cb17f4757f3e9ecfd1", "commit_rev":
"4b326a561bf0feaae081fe6f2037cde4d25a8fd9"}
Message was sent while issue was closed.
Description was changed from ========== Calls display_configuration_controller to rotate screen in accelerator_controller_delegate_aura This unifies the code path calling ScreenRotationAnimator::Rotate through display_configuration_controller SetDisplayRotation. Further screen rotation animation improvement will base on this change. BUG=678763 TEST=manual, unit_tests R=bruthig@chromium.org, oshima@chromium.org ========== to ========== Calls display_configuration_controller to rotate screen in accelerator_controller_delegate_aura This unifies the code path calling ScreenRotationAnimator::Rotate through display_configuration_controller SetDisplayRotation. Further screen rotation animation improvement will base on this change. BUG=678763 TEST=manual, unit_tests R=bruthig@chromium.org, oshima@chromium.org Review-Url: https://codereview.chromium.org/2720913003 Cr-Commit-Position: refs/heads/master@{#454288} Committed: https://chromium.googlesource.com/chromium/src/+/4b326a561bf0feaae081fe6f2037... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4b326a561bf0feaae081fe6f2037... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
