Re-enable ScreenOrientationListenerTest.java and run locking on UI thread.

Re-enable ScreenOrientationListenerTest.java and run locking on UI thread.

BUG=356483, 354553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287112

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287225

[mlamouri (slow - plz ping), 2014-07-31 17:19:10]

Jared, I moved this out of the older CL so I can re-enable that test before
re-enabling the other.

WDYT?

[mlamouri (slow - plz ping), 2014-07-31 17:20:02]

On 2014/07/31 17:19:10, Mounir Lamouri wrote:
> Jared, I moved this out of the older CL so I can re-enable that test before
> re-enabling the other.
> 
> WDYT?

Also, I applied your comments except two: I'm not checking that the criteria
return true because there might be valid situation where it does not.

[jdduke (slow), 2014-07-31 17:37:11]

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

https://codereview.chromium.org/434693002/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:71:
final int orientationForThread = orientation;
I don't think you need this intermediate variable if the method argument is
already final.

https://codereview.chromium.org/434693002/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:82:
CriteriaHelper.pollForCriteria(criteria);
Will the criteria be satisifed synchronously with respect to the
|lockOrientation| call? If not, you'll probably want to use
runOnUiThreadBlocking(), then do pollForCriteria on the test thread (outside of
the Runnable).

[mlamouri (slow - plz ping), 2014-07-31 17:49:07]

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

https://codereview.chromium.org/434693002/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:71:
final int orientationForThread = orientation;
On 2014/07/31 17:37:11, jdduke wrote:
> I don't think you need this intermediate variable if the method argument is
> already final.

Done.

https://codereview.chromium.org/434693002/diff/1/content/public/android/javat...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:82:
CriteriaHelper.pollForCriteria(criteria);
On 2014/07/31 17:37:11, jdduke wrote:
> Will the criteria be satisifed synchronously with respect to the
> |lockOrientation| call? If not, you'll probably want to use
> runOnUiThreadBlocking(), then do pollForCriteria on the test thread (outside
of
> the Runnable).

I'm not sure I entirely understood what you meant. Let me know if the update
matches.

[jdduke (slow), 2014-07-31 17:56:32]

lgtm assuming it passes (modulo a couple thoughts) :)

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

https://codereview.chromium.org/434693002/diff/20001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:55:
ThreadUtils.runOnUiThreadBlocking(runnable);
This should work, though I don't believe the waitForIdleSync() call is necessary
given the criteria poller.

https://codereview.chromium.org/434693002/diff/20001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:71:
OrientationChangeObserverCriteria criteria =
Maybe move this down below where it's used?

[mlamouri (slow - plz ping), 2014-07-31 18:02:51]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-07-31 18:03:03]

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

https://codereview.chromium.org/434693002/diff/20001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:55:
ThreadUtils.runOnUiThreadBlocking(runnable);
On 2014/07/31 17:56:31, jdduke wrote:
> This should work, though I don't believe the waitForIdleSync() call is
necessary
> given the criteria poller.

Honestly, if that can decrease flakyness...

https://codereview.chromium.org/434693002/diff/20001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:71:
OrientationChangeObserverCriteria criteria =
On 2014/07/31 17:56:31, jdduke wrote:
> Maybe move this down below where it's used?

Done.

[commit-bot: I haz the power, 2014-07-31 18:03:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/434693002/40001

[commit-bot: I haz the power, 2014-07-31 19:17:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-07-31 20:28:16]

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

[commit-bot: I haz the power, 2014-07-31 20:28:17]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[mlamouri (slow - plz ping), 2014-08-01 10:12:18]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-08-01 10:12:30]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-08-01 16:40:14]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-08-01 16:42:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/434693002/80001

[mlamouri (slow - plz ping), 2014-08-01 19:00:50]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-08-01 19:01:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/434693002/100001

[commit-bot: I haz the power, 2014-08-01 22:48:43]

Change committed as 287112

[Peter Beverloo, 2014-08-02 12:32:11]

A revert of this CL has been created in
https://codereview.chromium.org/431083005/ by peter@chromium.org.

The reason for reverting is: This broke the following test on the
contentshell_instrumentation_tests
on the Android Tests (dbg):

ScreenOrientationListenerTest.testConfigurationListenerFlipLandscape

http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg....

[mlamouri (slow - plz ping), 2014-08-02 16:42:00]

The failure came from a 4.3 bot that was running the ConfigurationListener test
but got a result I would expect from a DisplayListener. What likely happened is
that the bot was already with a DisplayListener and injecting the other listener
did not work.

Given that:
- we have already wasted too much time on this;
- injecting a listener is only use for tests;
- the other listener will be able to pass those test when we will land the
"accurate" mode.

I think we should just no longer bother injecting listener and simplify all of
this.

Peter, can you have a look? Thanks :)

[Peter Beverloo, 2014-08-02 17:06:24]

lgtm % comments

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

https://codereview.chromium.org/434693002/diff/120001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:54:
private void runOnUiThreadAndWait(Runnable runnable) {
This is only used once, can we move it in lockOrientationAndWait?

https://codereview.chromium.org/434693002/diff/120001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:121:
public void testVariousChanges() throws Exception {
+configuration? could have a somewhat clearer name

https://codereview.chromium.org/434693002/diff/120001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:165:
lockOrientationAndWait(ActivityInfo.SCREEN_ORIENTATION_PORTRAIT);
Should we perhaps do this in setUp()?

[mlamouri (slow - plz ping), 2014-08-02 17:56:49]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-08-02 17:57:02]

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

https://codereview.chromium.org/434693002/diff/120001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:54:
private void runOnUiThreadAndWait(Runnable runnable) {
On 2014/08/02 17:06:24, Peter Beverloo wrote:
> This is only used once, can we move it in lockOrientationAndWait?

Done.

https://codereview.chromium.org/434693002/diff/120001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:121:
public void testVariousChanges() throws Exception {
On 2014/08/02 17:06:24, Peter Beverloo wrote:
> +configuration? could have a somewhat clearer name

Done.

https://codereview.chromium.org/434693002/diff/120001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationListenerTest.java:165:
lockOrientationAndWait(ActivityInfo.SCREEN_ORIENTATION_PORTRAIT);
On 2014/08/02 17:06:24, Peter Beverloo wrote:
> Should we perhaps do this in setUp()?

Done.

[commit-bot: I haz the power, 2014-08-02 17:57:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/434693002/140001

[commit-bot: I haz the power, 2014-08-02 22:21:17]

Change committed as 287225