cros: Treat set rotation for chrome.system.display API in touchview mode as if user rotation lock

cros: Treat set rotation for chrome.system.display API in touchview mode as if user rotation lock

Changes:
Expose an API SetLockToOrientation for chrome.system.display set rotation that will do the work like user rotation lock.

BUG=704931
TEST=test coverage

Review-Url: https://codereview.chromium.org/2784043002
Cr-Commit-Position: refs/heads/master@{#461559}
Committed: https://chromium.googlesource.com/chromium/src/+/c5bd9a545c026646b4f9bc35aba650ff01c87edd

[Qiang(Joe) Xu, 2017-03-29 20:57:08]

Description was changed from

==========
cros: Treat set rotation for chrome.system.display API in touchview mode as user
rotation lock

BUG=704931
TEST=test coverage
==========

to

==========
cros: Treat set rotation for chrome.system.display API in touchview mode as if
user rotation lock

Changes:
Expose an API SetLockToOrientation for chrome.system.display set rotation that
will do the work like user rotation lock.

BUG=704931
TEST=test coverage
==========

[Qiang(Joe) Xu, 2017-03-29 20:57:08]

warx@chromium.org changed reviewers:
+ oshima@chromium.org

[Qiang(Joe) Xu, 2017-03-29 20:59:33]

Hi oshima@, PTAL first, still just make sure if the idea is all right, then I
will add c/b/e owner reviewer, thanks!

[Qiang(Joe) Xu, 2017-03-29 23:15:11]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 23:16:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2017-03-30 00:44:37]

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
File ash/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
ash/display/screen_orientation_controller_chromeos.cc:242:
DCHECK(display::Display::HasInternalDisplay());
move SetLockToOrientation to private, and add
SetLockToRotation.

return if it doesn't have internal display like other API.

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
File ash/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
ash/display/screen_orientation_controller_chromeos.h:85: // Set Locked to the
given |orientation|.
mention that this will be saved

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
ash/display/screen_orientation_controller_chromeos.h:86: void
SetLockToOrientation(blink::WebScreenOrientationLockType orientation);
can you just take the rotation, and do conversion in cc?

[Qiang(Joe) Xu, 2017-03-30 01:14:23]

warx@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Qiang(Joe) Xu, 2017-03-30 01:15:14]

Updated based on feedback, PTAL, thanks!

+lazyboy@ for c/b/e owner review.

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
File ash/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
ash/display/screen_orientation_controller_chromeos.cc:242:
DCHECK(display::Display::HasInternalDisplay());
On 2017/03/30 00:44:37, oshima wrote:
> move SetLockToOrientation to private, and add
> SetLockToRotation.
> 
> return if it doesn't have internal display like other API.

Done.

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
File ash/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
ash/display/screen_orientation_controller_chromeos.h:85: // Set Locked to the
given |orientation|.
On 2017/03/30 00:44:37, oshima wrote:
> mention that this will be saved

Done.

https://codereview.chromium.org/2784043002/diff/1/ash/display/screen_orientat...
ash/display/screen_orientation_controller_chromeos.h:86: void
SetLockToOrientation(blink::WebScreenOrientationLockType orientation);
On 2017/03/30 00:44:37, oshima wrote:
> can you just take the rotation, and do conversion in cc?

Done.

[oshima, 2017-03-30 04:46:00]

https://codereview.chromium.org/2784043002/diff/20001/ash/display/screen_orie...
File ash/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/2784043002/diff/20001/ash/display/screen_orie...
ash/display/screen_orientation_controller_chromeos.cc:360:
blink::WebScreenOrientationLockType orientation) {
this should be either

SetLockToOrientation(orientation) {
user_locked_orientation_ = orientation;

....


or

SetLockToUserLockedOrientation() {
}

whichever you like.

https://codereview.chromium.org/2784043002/diff/20001/ash/display/screen_orie...
ash/display/screen_orientation_controller_chromeos.cc:362: return;
you need to do this in SetLockToRotation if you update the state there.

[Qiang(Joe) Xu, 2017-03-30 17:02:22]

Hi all, updated, PTAL, thanks

[oshima, 2017-04-03 19:59:13]

lgtm

https://codereview.chromium.org/2784043002/diff/40001/ash/display/screen_orie...
File ash/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/2784043002/diff/40001/ash/display/screen_orie...
ash/display/screen_orientation_controller_chromeos.cc:227:
user_locked_orientation_ = blink::WebScreenOrientationLockAny;
just keep the value on stack in this method, and be consistent with
SetLockToRotation.

[Qiang(Joe) Xu, 2017-04-03 20:35:07]

Upated. lazyboy@, can I also get the c/b/e owner's review?

Thanks!

https://codereview.chromium.org/2784043002/diff/40001/ash/display/screen_orie...
File ash/display/screen_orientation_controller_chromeos.cc (right):

https://codereview.chromium.org/2784043002/diff/40001/ash/display/screen_orie...
ash/display/screen_orientation_controller_chromeos.cc:227:
user_locked_orientation_ = blink::WebScreenOrientationLockAny;
On 2017/04/03 19:59:13, oshima wrote:
> just keep the value on stack in this method, and be consistent with
> SetLockToRotation.

done. I also made changes in else branch, which makes more sense to me.

[lazyboy, 2017-04-03 20:52:34]

https://codereview.chromium.org/2784043002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2784043002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:976:
->user_rotation_locked());
Do we want to cleanup afterwards, i.e. EnableMaximizeMode(false)?

Also for this test and SetRotationBeforeMaximizeMode, I'd separate the
enabler/disabler in a class so that we always cleanup, even if ASSERT fails.
e.g.
(you can suggest a better name of the class)
class Enabler {
  Enabler() { EnableMaximizeMode(true); }
  ~Enabler() { EnableMaximizeMode(false); }
};

and keep an instance of Enabler inside the test body.

[Qiang(Joe) Xu, 2017-04-03 21:08:08]

https://codereview.chromium.org/2784043002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2784043002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:976:
->user_rotation_locked());
On 2017/04/03 20:52:34, lazyboy wrote:
> Do we want to cleanup afterwards, i.e. EnableMaximizeMode(false)?
> 
> Also for this test and SetRotationBeforeMaximizeMode, I'd separate the
> enabler/disabler in a class so that we always cleanup, even if ASSERT fails.
> e.g.
> (you can suggest a better name of the class)
> class Enabler {
>   Enabler() { EnableMaximizeMode(true); }
>   ~Enabler() { EnableMaximizeMode(false); }
> };
> 
> and keep an instance of Enabler inside the test body.

It doesn't need the cleanup for EnableMaximizeMode(false). For AshTestBase setup
called for each test, maximize_mode_controller() is recreated and is always set
to non-maximize-mode, if we don't force the touchview mode [1].

The Enabler class may not be necessary. WDYT?

[1]
https://cs.chromium.org/chromium/src/ash/common/wm/maximize_mode/maximize_mod...

[lazyboy, 2017-04-03 21:23:30]

c/b/extensions lgtm.

https://codereview.chromium.org/2784043002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2784043002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:976:
->user_rotation_locked());
On 2017/04/03 21:08:08, Qiang(Joe) Xu wrote:
> On 2017/04/03 20:52:34, lazyboy wrote:
> > Do we want to cleanup afterwards, i.e. EnableMaximizeMode(false)?
> > 
> > Also for this test and SetRotationBeforeMaximizeMode, I'd separate the
> > enabler/disabler in a class so that we always cleanup, even if ASSERT fails.
> > e.g.
> > (you can suggest a better name of the class)
> > class Enabler {
> >   Enabler() { EnableMaximizeMode(true); }
> >   ~Enabler() { EnableMaximizeMode(false); }
> > };
> > 
> > and keep an instance of Enabler inside the test body.
> 
> It doesn't need the cleanup for EnableMaximizeMode(false). For AshTestBase
setup
> called for each test, maximize_mode_controller() is recreated and is always
set
> to non-maximize-mode,
Acknowledged.
 if we don't force the touchview mode [1].
> 
> The Enabler class may not be necessary. WDYT?
> 
> [1]
>
https://cs.chromium.org/chromium/src/ash/common/wm/maximize_mode/maximize_mod...

[Qiang(Joe) Xu, 2017-04-03 21:26:39]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-04-03 21:26:40]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2784043002/#ps60001
(title: "based on ps3's comments")

[commit-bot: I haz the power, 2017-04-03 21:27:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 22:30:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491254799779220,
"parent_rev": "e23ca12cab688382ed5a3e3350885bed0b9d39b4", "commit_rev":
"c5bd9a545c026646b4f9bc35aba650ff01c87edd"}

[commit-bot: I haz the power, 2017-04-03 22:31:22]

Description was changed from

==========
cros: Treat set rotation for chrome.system.display API in touchview mode as if
user rotation lock

Changes:
Expose an API SetLockToOrientation for chrome.system.display set rotation that
will do the work like user rotation lock.

BUG=704931
TEST=test coverage
==========

to

==========
cros: Treat set rotation for chrome.system.display API in touchview mode as if
user rotation lock

Changes:
Expose an API SetLockToOrientation for chrome.system.display set rotation that
will do the work like user rotation lock.

BUG=704931
TEST=test coverage

Review-Url: https://codereview.chromium.org/2784043002
Cr-Commit-Position: refs/heads/master@{#461559}
Committed:
https://chromium.googlesource.com/chromium/src/+/c5bd9a545c026646b4f9bc35aba6...
==========

[commit-bot: I haz the power, 2017-04-03 22:31:23]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c5bd9a545c026646b4f9bc35aba6...