Properly interpret orientation provider/listener test values.

Properly interpret orientation provider/listener test results.

This change makes sure orientation test results are properly interpreted
for devices which natural orientation is landscape.

Existing implementation assumed portrait as device natural orientation.

BUG=
TEST=Run ContentShell instrumentation test.
./build/android/test_runner.py instrumentation --test-apk=ContentShellTest --release --test-filter=ScreenOrientation*

Committed: https://crrev.com/d8b82f91d5658b31eb6139d5e9173b91ab566156
Cr-Commit-Position: refs/heads/master@{#297670}

[gordanac, 2014-09-25 17:50:05]

gordana.cmiljanovic@imgtec.com changed reviewers:
+ mlamouri@chromium.org

[gordanac, 2014-09-25 17:53:00]

Please take a look.

This change makes orientation tests pass on tablets as well.

[mlamouri (slow - plz ping), 2014-09-29 10:54:36]

Thanks for the patches! It should be good to go with the below comments applied.

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
File
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java
(right):

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:37:
private boolean checkOrientationForLock(int orientation) {
Please, add a comments explaining what this method is doing.

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:49:
return mObserver.mOrientation == orientationTypeToAngle(orientation);
This sounds overly complicated. What about:
int expectedOrientation = orientationToAngle(orientation);
int currentOrientation = mObserver.mOrientation;

if (expectedOrientation == currentOrientation)
  return true;
else if (expectedOrientation == 180 && currentOrientation == 0)
  return true;
return false;

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:51:
return !mObserver.mHasChanged;
Could you make this return false instead? It should fail.

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:90:
/**
nit: indentation

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:147:
sNaturalOrientation = getNaturalOrientation(activity);
Given that you set sNaturalOrientation in SetUp(), could you make it a
non-static member? Therefore, rename it mNaturalOrientation?

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
File
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationProviderTest.java
(right):

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationProviderTest.java:142:
sNaturalOrientation = getNaturalOrientation(activity);
Given that you set sNaturalOrientation in SetUp(), could you make it a
non-static member? Therefore, rename it mNaturalOrientation?

[gordanac, 2014-10-01 16:22:44]

Patch set 2 uploaded. Please take a look.

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
File
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java
(right):

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:37:
private boolean checkOrientationForLock(int orientation) {
On 2014/09/29 10:54:35, Mounir Lamouri wrote:
> Please, add a comments explaining what this method is doing. 

Done.

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:49:
return mObserver.mOrientation == orientationTypeToAngle(orientation);
On 2014/09/29 10:54:36, Mounir Lamouri wrote:
> This sounds overly complicated. What about:
Done. Just added ALLOW_0_FOR_180 to the check.

> int expectedOrientation = orientationToAngle(orientation);
> int currentOrientation = mObserver.mOrientation;
> 
> if (expectedOrientation == currentOrientation)
>   return true;
> else if (expectedOrientation == 180 && currentOrientation == 0)
>   return true;
> return false;

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:51:
return !mObserver.mHasChanged;
On 2014/09/29 10:54:35, Mounir Lamouri wrote:
> Could you make this return false instead? It should fail.

Done.

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:90:
/**
On 2014/09/29 10:54:35, Mounir Lamouri wrote:
> nit: indentation

Done.

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:147:
sNaturalOrientation = getNaturalOrientation(activity);
On 2014/09/29 10:54:36, Mounir Lamouri wrote:
> Given that you set sNaturalOrientation in SetUp(), could you make it a
> non-static member? Therefore, rename it mNaturalOrientation?

Done.

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
File
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationProviderTest.java
(right):

https://codereview.chromium.org/608503003/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationProviderTest.java:142:
sNaturalOrientation = getNaturalOrientation(activity);
On 2014/09/29 10:54:36, Mounir Lamouri wrote:
> Given that you set sNaturalOrientation in SetUp(), could you make it a
> non-static member? Therefore, rename it mNaturalOrientation?

Done.

[mlamouri (slow - plz ping), 2014-10-01 16:30:33]

lgtm with nits below

https://codereview.chromium.org/608503003/diff/20001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java
(right):

https://codereview.chromium.org/608503003/diff/20001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:35:
private int mNaturalOrientation = ActivityInfo.SCREEN_ORIENTATION_PORTRAIT;
Could you set it to SCREEN_ORIENTATION_UNSPECIFIED?

https://codereview.chromium.org/608503003/diff/20001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationProviderTest.java
(right):

https://codereview.chromium.org/608503003/diff/20001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationProviderTest.java:36:
private int mNaturalOrientation = ActivityInfo.SCREEN_ORIENTATION_PORTRAIT;
ditto

[gordanac, 2014-10-01 16:44:22]

https://codereview.chromium.org/608503003/diff/20001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java
(right):

https://codereview.chromium.org/608503003/diff/20001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:35:
private int mNaturalOrientation = ActivityInfo.SCREEN_ORIENTATION_PORTRAIT;
On 2014/10/01 16:30:33, Mounir Lamouri wrote:
> Could you set it to SCREEN_ORIENTATION_UNSPECIFIED?

Done.

https://codereview.chromium.org/608503003/diff/20001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationProviderTest.java
(right):

https://codereview.chromium.org/608503003/diff/20001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationProviderTest.java:36:
private int mNaturalOrientation = ActivityInfo.SCREEN_ORIENTATION_PORTRAIT;
On 2014/10/01 16:30:33, Mounir Lamouri wrote:
> ditto

Done.

[gordanac, 2014-10-01 16:45:08]

The CQ bit was checked by gordana.cmiljanovic@imgtec.com

[commit-bot: I haz the power, 2014-10-01 16:46:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608503003/40001

[commit-bot: I haz the power, 2014-10-01 17:35:05]

Committed patchset #3 (id:40001) as 26c0b76d645b3654147c5848b29925b3c1d28816

[commit-bot: I haz the power, 2014-10-01 17:35:44]

Patchset 3 (id:??) landed as
https://crrev.com/d8b82f91d5658b31eb6139d5e9173b91ab566156
Cr-Commit-Position: refs/heads/master@{#297670}