Android screen orientation lock should lock/unlock the same activity

Android screen orientation lock should lock/unlock the same activity

Screen orientation lock used to lock/unlock the focused Activity, rather
than the Activity associated with the WebContents that triggered the
lock/unlock. If unlocking is triggered by a background WebContents, this
would result in the wrong Activity getting unlocked, and hence the
original Activity remaining in a permanently locked state!

This patch makes ScreenOrientationProvider always lock/unlock the
associated Activity, even if it's not the foreground one.

As well as a dedicated test (testUnlockWithDownloadViewerActivity in
VideoFullscreenOrientationLockChromeTest), some additional tests are
added to VideoFullscreenOrientationLockTest for cases that already
worked but didn't have test coverage.

And it fixes a null pointer exception that came up in Shell.java during some of the tests.

BUG=684497
Review-Url: https://codereview.chromium.org/2696303004
Cr-Commit-Position: refs/heads/master@{#453679}
Committed: https://chromium.googlesource.com/chromium/src/+/a2fe96582d0daf599ddcb5c4a655018de5c87755

[johnme, 2017-02-16 19:11:26]

Description was changed from

==========
Android screen orientation lock should lock/unlock the same activity

Screen orientation lock used to lock/unlock the focused Activity, rather
than the Activity associated with the WebContents that triggered the
lock/unlock. If unlocking is triggered by a background WebContents, this
would result in the wrong Activity getting unlocked, and hence the
original Activity remaining in a permanently locked state!

This patch makes ScreenOrientationProvider always lock/unlock the
associated Activity, even if it's not the foreground one.

As well as a dedicated test (testUnlockWithDownloadViewerActivity in
VideoFullscreenOrientationLockChromeTest), some additional tests are
added to VideoFullscreenOrientationLockTest for cases that already
worked but didn't have test coverage.

BUG=684497
==========

to

==========
Android screen orientation lock should lock/unlock the same activity

Screen orientation lock used to lock/unlock the focused Activity, rather
than the Activity associated with the WebContents that triggered the
lock/unlock. If unlocking is triggered by a background WebContents, this
would result in the wrong Activity getting unlocked, and hence the
original Activity remaining in a permanently locked state!

This patch makes ScreenOrientationProvider always lock/unlock the
associated Activity, even if it's not the foreground one.

As well as a dedicated test (testUnlockWithDownloadViewerActivity in
VideoFullscreenOrientationLockChromeTest), some additional tests are
added to VideoFullscreenOrientationLockTest for cases that already
worked but didn't have test coverage.

And it fixes a null pointer exception that came up in Shell.java during some of
the tests.

BUG=684497
==========

[johnme, 2017-02-16 19:16:47]

johnme@chromium.org changed reviewers:
+ mlamouri@chromium.org

[johnme, 2017-02-16 19:16:48]

mlamouri@chromium.org: Please review overall patch

[mlamouri (slow - plz ping), 2017-02-17 12:15:37]

Looks great, thanks! :)

https://codereview.chromium.org/2696303004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java
(right):

https://codereview.chromium.org/2696303004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java:78:
// Locking orientation is only supported for web contents that have an
associated activity.
nit: empty line between `Activity activity = ...` and the comment.

https://codereview.chromium.org/2696303004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java:97:
// Locking orientation is only supported for web contents that have an
associated activity.
ditto

[mlamouri (slow - plz ping), 2017-02-17 12:15:48]

hmm, I meant to lgtm :)

[mlamouri (slow - plz ping), 2017-02-17 12:17:39]

mlamouri@chromium.org changed reviewers:
+ tedchoc@chromium.org

[mlamouri (slow - plz ping), 2017-02-17 12:17:39]

+tedchoc@

[Ted C, 2017-02-23 18:30:23]

lgtm

Conceptually, you could lock the orientation of an activity containing a
webcontents, then reparent to a different activity and known unlocking wouldn't
be right.  But there isn't really a path where that would be feasible in the
current UI, so this is definitely an improvement over the previous behavior.

[johnme, 2017-02-23 18:41:37]

Addressed nits - thanks!

> Conceptually, you could lock the orientation of an activity containing a
> webcontents, then reparent to a different activity and known unlocking
wouldn't
> be right.  But there isn't really a path where that would be feasible in the
> current UI, so this is definitely an improvement over the previous behavior.

Yes, I suppose ideally it would listen for reparentings somehow and unlock or
transfer the lock.

https://codereview.chromium.org/2696303004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java
(right):

https://codereview.chromium.org/2696303004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java:78:
// Locking orientation is only supported for web contents that have an
associated activity.
On 2017/02/17 12:15:37, mlamouri wrote:
> nit: empty line between `Activity activity = ...` and the comment.

Done.

https://codereview.chromium.org/2696303004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java:97:
// Locking orientation is only supported for web contents that have an
associated activity.
On 2017/02/17 12:15:37, mlamouri wrote:
> ditto

Done.

[johnme, 2017-02-23 18:41:42]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-02-23 18:41:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2696303004/#ps20001
(title: "Address whitespace nits")

[commit-bot: I haz the power, 2017-02-23 18:42:30]

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-02-23 19:14:41]

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

[commit-bot: I haz the power, 2017-02-23 19:14:42]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[johnme, 2017-02-24 17:36:27]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-02-24 17:36:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2696303004/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-24 17:36:50]

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-02-24 20:05:17]

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

[commit-bot: I haz the power, 2017-02-24 20:05:18]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mlamouri (slow - plz ping), 2017-02-27 15:22:13]

Looks like your own test is failing johnme@ :(

[johnme, 2017-02-28 17:17:11]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-02-28 17:17:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2696303004/#ps60001
(title: "Add data deps to chrome_test_java")

[commit-bot: I haz the power, 2017-02-28 17:17:30]

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-02-28 19:56:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488302231942270,
"parent_rev": "da781ffa872f659a8f0de9a20b4166789a3f5409", "commit_rev":
"a2fe96582d0daf599ddcb5c4a655018de5c87755"}

[commit-bot: I haz the power, 2017-02-28 19:57:14]

Description was changed from

==========
Android screen orientation lock should lock/unlock the same activity

Screen orientation lock used to lock/unlock the focused Activity, rather
than the Activity associated with the WebContents that triggered the
lock/unlock. If unlocking is triggered by a background WebContents, this
would result in the wrong Activity getting unlocked, and hence the
original Activity remaining in a permanently locked state!

This patch makes ScreenOrientationProvider always lock/unlock the
associated Activity, even if it's not the foreground one.

As well as a dedicated test (testUnlockWithDownloadViewerActivity in
VideoFullscreenOrientationLockChromeTest), some additional tests are
added to VideoFullscreenOrientationLockTest for cases that already
worked but didn't have test coverage.

And it fixes a null pointer exception that came up in Shell.java during some of
the tests.

BUG=684497
==========

to

==========
Android screen orientation lock should lock/unlock the same activity

Screen orientation lock used to lock/unlock the focused Activity, rather
than the Activity associated with the WebContents that triggered the
lock/unlock. If unlocking is triggered by a background WebContents, this
would result in the wrong Activity getting unlocked, and hence the
original Activity remaining in a permanently locked state!

This patch makes ScreenOrientationProvider always lock/unlock the
associated Activity, even if it's not the foreground one.

As well as a dedicated test (testUnlockWithDownloadViewerActivity in
VideoFullscreenOrientationLockChromeTest), some additional tests are
added to VideoFullscreenOrientationLockTest for cases that already
worked but didn't have test coverage.

And it fixes a null pointer exception that came up in Shell.java during some of
the tests.

BUG=684497

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

[commit-bot: I haz the power, 2017-02-28 19:57:16]

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