Resolve current orientation after applying the previous orientation request.

Resolve current orientation after applying the previous orientation request.

An application may request to lock to portrait (primary/secondary), the
lock to specific orientation by sending CURRENT (no sensor/lock).
Current implement does not support this scenario because it looks to
the current device's orientation even if that orientation was for other apps.

This CL addresses this issue by waiting until the window is activated.

BUG=708202
TEST=covered by unit test, plus manual tests using sample app.

Review-Url: https://codereview.chromium.org/2792973002
Cr-Commit-Position: refs/heads/master@{#462206}
Committed: https://chromium.googlesource.com/chromium/src/+/4f5b7f72da4ca1534233252c673f9c162bdb25f9

[oshima, 2017-04-04 14:29:07]

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

[commit-bot: I haz the power, 2017-04-04 14:29:30]

Dry run: 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-04 14:52:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 14:52:15]

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_...)

[oshima, 2017-04-04 16:35:18]

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

[oshima, 2017-04-04 16:35:24]

Patchset #1 (id:1) has been deleted

[oshima, 2017-04-04 16:35:30]

Patchset #1 (id:20001) has been deleted

[oshima, 2017-04-04 16:35:35]

Patchset #1 (id:40001) has been deleted

[oshima, 2017-04-04 16:35:40]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-04-04 16:35:46]

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

[oshima, 2017-04-04 16:47:59]

Description was changed from

==========
LockToCurrent

BUG=
==========

to

==========
Resolve current orientation after applying the previous orientation request.

An application may request to lock to portrait (primary/secondary), the
lock to specific orientation by sending CURRENT (no sensor/lock).
Current implement does not support this scenario because it looks to
the current device's orientation even if that orientation was for other apps.

This CL addresses this issue by waiting until the window is activated.

BUG=708202
TEST=covered by unit test, plus manual tests using sample app.
==========

[oshima, 2017-04-04 16:47:59]

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

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

The CQ bit was unchecked by commit-bot@chromium.org

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

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_...)

[oshima, 2017-04-04 18:52:48]

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

[commit-bot: I haz the power, 2017-04-04 18:53:31]

Dry run: 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-04 19:48:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 19:48:42]

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_...)

[jonross, 2017-04-04 21:51:32]

A few questions about the api we're presenting for this split.

There also appears to be a test failing as a result of the change.

https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
File ash/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
ash/display/screen_orientation_controller_chromeos.h:63: bool lock_to_current);
So there is a split now in functionality between blink and arc for locks.

I wonder if it would make sense to have the resolution being done in
ArcAppWindowLauncherController here instead? One api for web, which doesn't have
this additional lock restriction. One for arc which can then apply it at window
activation?

If not, the nomenclature is slightly confusing. It makes me think that the lock
will be done immediately, vs waiting for activation. Especially with call sites
all documenting the purpose of the boolean.

Maybe an enum? Since this is more of a narrowing of the orientation request.
  RestrictOrientation {
    None,
    ToOrientationAtActivation
  }

https://codereview.chromium.org/2792973002/diff/90001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2792973002/diff/90001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:698:
lock_to_current_orientation = info->lock_to_current();
for Portrait/Landscape + lock_to_current, those requests would be received and
processes here?

[oshima, 2017-04-04 23:00:25]

https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
File ash/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
ash/display/screen_orientation_controller_chromeos.h:63: bool lock_to_current);
On 2017/04/04 21:51:32, jonross wrote:
> So there is a split now in functionality between blink and arc for locks.

Well, it's already different (for example, blink can lock only in fullscreen in
tablet mode,
while arc always can request orientation, and it'll be honored when switching to
tablet).
Note that it will not change the blink behavior.

There are lock concept that does not exit in html5 orientation spec. This is
ASH/ChromeOS,
so I don't think that's going to be an issue as long as we can cover both.

> 
> I wonder if it would make sense to have the resolution being done in
> ArcAppWindowLauncherController here instead? One api for web, which doesn't
have
> this additional lock restriction. One for arc which can' then apply it at
window
> activation?

The lock is determined when locked, so we can't implement there unless we add a
callback
when lock applied, which is IMO overkill.

> 
> If not, the nomenclature is slightly confusing. It makes me think that the
lock
> will be done immediately, vs waiting for activation. Especially with call
sites
> all documenting the purpose of the boolean.
> 
> Maybe an enum? Since this is more of a narrowing of the orientation request.
>   RestrictOrientation {
>     None,
>     ToOrientationAtActivation
>   }

Note that we always lock at activation, with or without this change. 
This is to lock to specific orientation after actually lock happened. So it's
more like

{
   Normal,
   ToFurtherLockToSpecificOrientation
}
or
{
  Normal,
  DisableSensorRotationAfterLock
}

maybe latter is easier to understand? WDYT?

[oshima, 2017-04-04 23:04:02]

On 2017/04/04 21:51:32, jonross wrote:
> There also appears to be a test failing as a result of the change.

Thanks, that was for the arc test, which now have different semantics. Will work
on a fix.

[jonross, 2017-04-04 23:49:25]

https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
File ash/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
ash/display/screen_orientation_controller_chromeos.h:63: bool lock_to_current);
On 2017/04/04 23:00:25, oshima wrote:
> Well, it's already different (for example, blink can lock only in fullscreen
in
> tablet mode,
> while arc always can request orientation, and it'll be honored when switching
to
> tablet).
> Note that it will not change the blink behavior.
> 
> There are lock concept that does not exit in html5 orientation spec. This is
> ASH/ChromeOS,
> so I don't think that's going to be an issue as long as we can cover both.
> 

Good point, I'm fine keeping this through the one api then.

 
> The lock is determined when locked, so we can't implement there unless we add
a
> callback
> when lock applied, which is IMO overkill.

Yeah that would be overkill, keeping it in its current implementation location
SGTM.

> 
> Note that we always lock at activation, with or without this change. 
> This is to lock to specific orientation after actually lock happened. So it's
> more like
> 
> {
>    Normal,
>    ToFurtherLockToSpecificOrientation
> }
> or
> {
>   Normal,
>   DisableSensorRotationAfterLock
> }
> 
> maybe latter is easier to understand? WDYT?

Yeah the latter is easier to understand. Can we switch to using that?

[oshima, 2017-04-05 00:53:29]

On 2017/04/04 23:49:25, jonross wrote:
>
https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
> File ash/display/screen_orientation_controller_chromeos.h (right):
> 
>
https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
> ash/display/screen_orientation_controller_chromeos.h:63: bool
lock_to_current);
> On 2017/04/04 23:00:25, oshima wrote:
> > Well, it's already different (for example, blink can lock only in fullscreen
> in
> > tablet mode,
> > while arc always can request orientation, and it'll be honored when
switching
> to
> > tablet).
> > Note that it will not change the blink behavior.
> > 
> > There are lock concept that does not exit in html5 orientation spec. This is
> > ASH/ChromeOS,
> > so I don't think that's going to be an issue as long as we can cover both.
> > 
> 
> Good point, I'm fine keeping this through the one api then.
> 
>  
> > The lock is determined when locked, so we can't implement there unless we
add
> a
> > callback
> > when lock applied, which is IMO overkill.
> 
> Yeah that would be overkill, keeping it in its current implementation location
> SGTM.
> 
> > 
> > Note that we always lock at activation, with or without this change. 
> > This is to lock to specific orientation after actually lock happened. So
it's
> > more like
> > 
> > {
> >    Normal,
> >    ToFurtherLockToSpecificOrientation
> > }
> > or
> > {
> >   Normal,
> >   DisableSensorRotationAfterLock
> > }
> > 
> > maybe latter is easier to understand? WDYT?
> 
> Yeah the latter is easier to understand. Can we switch to using that?

will do!

[oshima, 2017-04-05 14:48:58]

Patchset #4 (id:130001) has been deleted

[oshima, 2017-04-05 14:49:44]

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

[commit-bot: I haz the power, 2017-04-05 14:50:12]

Dry run: 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-05 15:27:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 15:27:27]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-04-05 18:19:30]

PTAL

I removed the
ChromeLauncherControllerOrientationTest.CurrentWithLandscapeDisplay because it
doesn't make sense to test there any longer.

It's now covered by new tests in ash.

https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
File ash/display/screen_orientation_controller_chromeos.h (right):

https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
ash/display/screen_orientation_controller_chromeos.h:63: bool lock_to_current);
On 2017/04/04 23:49:25, jonross wrote:
> On 2017/04/04 23:00:25, oshima wrote:
> > Well, it's already different (for example, blink can lock only in fullscreen
> in
> > tablet mode,
> > while arc always can request orientation, and it'll be honored when
switching
> to
> > tablet).
> > Note that it will not change the blink behavior.
> > 
> > There are lock concept that does not exit in html5 orientation spec. This is
> > ASH/ChromeOS,
> > so I don't think that's going to be an issue as long as we can cover both.
> > 
> 
> Good point, I'm fine keeping this through the one api then.
> 
>  
> > The lock is determined when locked, so we can't implement there unless we
add
> a
> > callback
> > when lock applied, which is IMO overkill.
> 
> Yeah that would be overkill, keeping it in its current implementation location
> SGTM.
> 
> > 
> > Note that we always lock at activation, with or without this change. 
> > This is to lock to specific orientation after actually lock happened. So
it's
> > more like
> > 
> > {
> >    Normal,
> >    ToFurtherLockToSpecificOrientation
> > }
> > or
> > {
> >   Normal,
> >   DisableSensorRotationAfterLock
> > }
> > 
> > maybe latter is easier to understand? WDYT?
> 
> Yeah the latter is easier to understand. Can we switch to using that?

Done.

[jonross, 2017-04-05 20:22:48]

On 2017/04/05 18:19:30, oshima wrote:
> PTAL
> 
> I removed the
> ChromeLauncherControllerOrientationTest.CurrentWithLandscapeDisplay because it
> doesn't make sense to test there any longer.
> 
> It's now covered by new tests in ash.
> 
>
https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
> File ash/display/screen_orientation_controller_chromeos.h (right):
> 
>
https://codereview.chromium.org/2792973002/diff/90001/ash/display/screen_orie...
> ash/display/screen_orientation_controller_chromeos.h:63: bool
lock_to_current);
> On 2017/04/04 23:49:25, jonross wrote:
> > On 2017/04/04 23:00:25, oshima wrote:
> > > Well, it's already different (for example, blink can lock only in
fullscreen
> > in
> > > tablet mode,
> > > while arc always can request orientation, and it'll be honored when
> switching
> > to
> > > tablet).
> > > Note that it will not change the blink behavior.
> > > 
> > > There are lock concept that does not exit in html5 orientation spec. This
is
> > > ASH/ChromeOS,
> > > so I don't think that's going to be an issue as long as we can cover both.
> > > 
> > 
> > Good point, I'm fine keeping this through the one api then.
> > 
> >  
> > > The lock is determined when locked, so we can't implement there unless we
> add
> > a
> > > callback
> > > when lock applied, which is IMO overkill.
> > 
> > Yeah that would be overkill, keeping it in its current implementation
location
> > SGTM.
> > 
> > > 
> > > Note that we always lock at activation, with or without this change. 
> > > This is to lock to specific orientation after actually lock happened. So
> it's
> > > more like
> > > 
> > > {
> > >    Normal,
> > >    ToFurtherLockToSpecificOrientation
> > > }
> > > or
> > > {
> > >   Normal,
> > >   DisableSensorRotationAfterLock
> > > }
> > > 
> > > maybe latter is easier to understand? WDYT?
> > 
> > Yeah the latter is easier to understand. Can we switch to using that?
> 
> Done.

LGTM

[oshima, 2017-04-05 20:31:56]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2017-04-05 20:33:37]

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-05 20:42:20]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1491424316676780,
"parent_rev": "ee3cd20ca02aa2b2e00e96f125c10bb84cfb92eb", "commit_rev":
"4f5b7f72da4ca1534233252c673f9c162bdb25f9"}

[commit-bot: I haz the power, 2017-04-05 20:43:15]

Description was changed from

==========
Resolve current orientation after applying the previous orientation request.

An application may request to lock to portrait (primary/secondary), the
lock to specific orientation by sending CURRENT (no sensor/lock).
Current implement does not support this scenario because it looks to
the current device's orientation even if that orientation was for other apps.

This CL addresses this issue by waiting until the window is activated.

BUG=708202
TEST=covered by unit test, plus manual tests using sample app.
==========

to

==========
Resolve current orientation after applying the previous orientation request.

An application may request to lock to portrait (primary/secondary), the
lock to specific orientation by sending CURRENT (no sensor/lock).
Current implement does not support this scenario because it looks to
the current device's orientation even if that orientation was for other apps.

This CL addresses this issue by waiting until the window is activated.

BUG=708202
TEST=covered by unit test, plus manual tests using sample app.

Review-Url: https://codereview.chromium.org/2792973002
Cr-Commit-Position: refs/heads/master@{#462206}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f5b7f72da4ca1534233252c673f...
==========

[commit-bot: I haz the power, 2017-04-05 20:43:17]

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