content_shell: Move BrowserTestSystemMessageHandler and use it in layout tests for Android

content_shell: Move BrowserTestSystemMessageHandler and use it in layout tests
for Android

Android needs to use a nested message loop in order to execute the
layout tests. We have an implementation of this for content_browsertests
as BrowserTestSystemMessageHandler, but this lives in /content/test/
which content_shell cannot depend on.

Move it to /content/public/test/ as NestedSystemMessageHandler and
update content_browsertests to depend on that instead. For content_shell,
implement support of starting the nested loop, and use it when running
layout tests.

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

[Peter Beverloo, 2013-06-17 14:50:29]

+jochen for content
+bulach for Android/general feedback

[nilesh, 2013-06-17 17:37:27]

https://codereview.chromium.org/17076008/diff/1/content/public/test/browser_t...
File content/public/test/browser_test_message_pump_android.h (right):

https://codereview.chromium.org/17076008/diff/1/content/public/test/browser_t...
content/public/test/browser_test_message_pump_android.h:14: // MessgePump to be
used for content browsertests on Android.
I think we should rename this to NestedMessagePumpAndroid (as this is no longer
browsertest specific). WDYT?
Also fix the comment here.

[bulach, 2013-06-17 17:41:22]

lgtm, but please make nilesh and bots happy :)

small suggestions below:

https://codereview.chromium.org/17076008/diff/1/content/public/test/android/j...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/BrowserTestSystemMessageHandler.java
(right):

https://codereview.chromium.org/17076008/diff/1/content/public/test/android/j...
content/public/test/android/javatests/src/org/chromium/content/browser/test/BrowserTestSystemMessageHandler.java:27:
class BrowserTestSystemMessageHandler {
this also could do with some renaming..

https://codereview.chromium.org/17076008/diff/1/content/public/test/browser_t...
File content/public/test/browser_test_message_pump_android.h (right):

https://codereview.chromium.org/17076008/diff/1/content/public/test/browser_t...
content/public/test/browser_test_message_pump_android.h:14: // MessgePump to be
used for content browsertests on Android.
On 2013/06/17 17:37:28, nilesh wrote:
> I think we should rename this to NestedMessagePumpAndroid (as this is no
longer
> browsertest specific). WDYT?
> Also fix the comment here.

+1

https://codereview.chromium.org/17076008/diff/1/content/shell/shell_browser_m...
File content/shell/shell_browser_main.cc (right):

https://codereview.chromium.org/17076008/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:214: // base::RunLoop which is closer to
what's being used.
I guess this deserves a slightly more detailed explanation, something along the
lines of:

// The android system manages the main message loop
// in the java side, and it doesn't provide any blocking
// Run() method.
// When running layout tests, create a nested message loop
// and use base::RunLoop() so that it can block until Quit().


on a tangential note, unrelated to your patch: it'd be nice to have separate
methods for all these chunks "if (layout_test_mode) { ... }", so that it's
clearer this won't affect content shell in "regular" mode..

[Peter Beverloo, 2013-06-17 18:36:29]

content_shell: Move BrowserTestSystemMessageHandler and use it in layout tests
for Android

Android needs to use a nested message loop in order to execute the
layout tests. We have an implementation of this for content_browsertests
as BrowserTestSystemMessageHandler, but this lives in /content/test/
which content_shell cannot depend on.

Move it to /content/public/test/ as NestedSystemMessageHandler and
update content_browsertests to depend on that instead. For content_shell,
implement support of starting the nested loop, and use it when running
layout tests.

BUG=232044

[Peter Beverloo, 2013-06-17 18:41:56]

Thank you for the quick looks!  I addressed your comments, please take another
look Nilesh.

https://codereview.chromium.org/17076008/diff/1/content/public/test/android/j...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/BrowserTestSystemMessageHandler.java
(right):

https://codereview.chromium.org/17076008/diff/1/content/public/test/android/j...
content/public/test/android/javatests/src/org/chromium/content/browser/test/BrowserTestSystemMessageHandler.java:27:
class BrowserTestSystemMessageHandler {
On 2013/06/17 17:41:22, bulach wrote:
> this also could do with some renaming..

Done.

https://codereview.chromium.org/17076008/diff/1/content/public/test/browser_t...
File content/public/test/browser_test_message_pump_android.h (right):

https://codereview.chromium.org/17076008/diff/1/content/public/test/browser_t...
content/public/test/browser_test_message_pump_android.h:14: // MessgePump to be
used for content browsertests on Android.
On 2013/06/17 17:41:22, bulach wrote:
> On 2013/06/17 17:37:28, nilesh wrote:
> > I think we should rename this to NestedMessagePumpAndroid (as this is no
> longer
> > browsertest specific). WDYT?
> > Also fix the comment here.
> 
> +1

Done.

https://codereview.chromium.org/17076008/diff/1/content/shell/shell_browser_m...
File content/shell/shell_browser_main.cc (right):

https://codereview.chromium.org/17076008/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:214: // base::RunLoop which is closer to
what's being used.
I clarified the comment.  I also agree that this code could benefit from some
refactoring, let me do that in a follow-up patch.

[nilesh, 2013-06-17 20:31:14]

On 2013/06/17 18:41:56, Peter Beverloo wrote:
> Thank you for the quick looks!  I addressed your comments, please take another
> look Nilesh.
> 

lgtm, Please make sure you run content_browsertests (using android_fyi_dbg try
bot). I added the trybot for this patch.

[Peter Beverloo, 2013-06-18 10:44:04]

On 2013/06/17 20:31:14, nilesh wrote:
> lgtm, Please make sure you run content_browsertests (using android_fyi_dbg try
> bot). I added the trybot for this patch.

Thanks! I added a dependency from content_browsertests_apk to our
content_java_test_support target to make sure the necessary Java files were
being included in the APK. All test-suites work again, I launched try-jobs to
verify.

[jochen (gone - plz use gerrit), 2013-06-18 10:46:24]

lgtm

[Peter Beverloo, 2013-06-18 15:56:55]

Cheers for the reviews! android_fyi_dbg_triggered_tests is now happy, so putting
this on the queue again.  The android_dbg_triggered_tests bot was happy on the
previous run, so this seems like a flake.  mac_asan and win_rel failures aren't
related.

[commit-bot: I haz the power, 2013-06-18 15:57:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/17076008/45001

[commit-bot: I haz the power, 2013-06-18 16:08:00]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Peter Beverloo, 2013-06-18 16:18:34]

+phajdan for content/test and content/public/test OWNERS.

[Paweł Hajdan Jr., 2013-06-18 16:48:50]

It's not obvious to me why you're moving things to content/public - it looks
like all the places this CL modifies could just use content/test .

I defer to John Abd-El-Malek for specifics here (please get his review before
committing) - my understanding is we're trying to limit interfaces exposed by
content/public.

[Peter Beverloo, 2013-06-18 16:52:44]

content/shell/ uses it, which may only depend on /content/public/test/ (thus not
/content/test/).  See the dependency rules in /content/shell/DEPS.

[Paweł Hajdan Jr., 2013-06-18 17:39:36]

On 2013/06/18 16:52:44, Peter Beverloo wrote:
> content/shell/ uses it, which may only depend on /content/public/test/ (thus
not
> /content/test/).  See the dependency rules in /content/shell/DEPS.

Ah, I see. Still, let's see what John says.

[jam, 2013-06-18 17:58:59]

lgtm

[Paweł Hajdan Jr., 2013-06-18 18:13:36]

Just in case, LGTM .

[commit-bot: I haz the power, 2013-06-18 23:40:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/17076008/45001

[commit-bot: I haz the power, 2013-06-19 02:47:05]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-19 10:40:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/17076008/45001

[commit-bot: I haz the power, 2013-06-19 12:01:44]

Change committed as 207215