content_shell: Create the FIFOs needed for running Android layout tests

content_shell: Create the FIFOs needed for running Android layout tests

The Android layout test runner communicates with content_shell through
a set of three fifos, one for each of [stdout, stderr, stdin]. Create
those after basic initialization.

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

[bulach, 2013-06-19 14:42:14]

some suggestions below:

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

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:39: const char kAndroidFifoPath[] =
see below, looks like we can gather this from java..

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:77: }
how about moving this entire block to a shell_layout_tests_android.h/cc ?

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:186: CHECK(CreateAndroidFIFOs()) << "Unable
to initialize the Android FIFOs.";
since this one has JNI available, you can add a @CalledByNative in the java side
to return the directory name instead of hardcoding it here.

also, it seems that this block could be moved in the separate file, and here
it'd just call "InitializeShellForLayoutTests()" or something.. wdyt?

[jochen (gone - plz use gerrit), 2013-06-19 15:37:13]

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

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:39: const char kAndroidFifoPath[] =
On 2013/06/19 14:42:14, bulach wrote:
> see below, looks like we can gather this from java..

and it should be FILE_PATH_LITERAL(), no? also kAndroidLayoutTestPath

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:61: base::FilePath
stdout_fifo(files_dir.Append(base::FilePath("test.fifo")));
also FILE_PATH_LITERAL() instead of full filepath

[Peter Beverloo, 2013-06-19 17:40:28]

All amended, please take another look.  I also incorporated Dale's comments from
CL 16510004.

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

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:39: const char kAndroidFifoPath[] =
On 2013/06/19 15:37:13, jochen wrote:
> On 2013/06/19 14:42:14, bulach wrote:
> > see below, looks like we can gather this from java..
> 
> and it should be FILE_PATH_LITERAL(), no? also kAndroidLayoutTestPath

Done.

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:61: base::FilePath
stdout_fifo(files_dir.Append(base::FilePath("test.fifo")));
On 2013/06/19 15:37:13, jochen wrote:
> also FILE_PATH_LITERAL() instead of full filepath

Done.

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:77: }
On 2013/06/19 14:42:14, bulach wrote:
> how about moving this entire block to a shell_layout_tests_android.h/cc ?

Done.

https://codereview.chromium.org/17265005/diff/1/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:186: CHECK(CreateAndroidFIFOs()) << "Unable
to initialize the Android FIFOs.";
On 2013/06/19 14:42:14, bulach wrote:
> since this one has JNI available, you can add a @CalledByNative in the java
side
> to return the directory name instead of hardcoding it here.
> 
> also, it seems that this block could be moved in the separate file, and here
> it'd just call "InitializeShellForLayoutTests()" or something.. wdyt?

Done.

[bulach, 2013-06-19 17:57:42]

thanks, much cleaner! just a few more suggestions and nits..

https://codereview.chromium.org/17265005/diff/4001/content/shell/android/java...
File
content/shell/android/java/src/org/chromium/content_shell/ShellLayoutTestUtils.java
(right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellLayoutTestUtils.java:13:
* Utility methods used when using content_shell for running Blink's layout
s/used when using/used by/

https://codereview.chromium.org/17265005/diff/4001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellLayoutTestUtils.java:17:
public class ShellLayoutTestUtils {
nit: s/public//

https://codereview.chromium.org/17265005/diff/4001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellLayoutTestUtils.java:23:
public static String getApplicationFilesDirectory(Context appContext) {
nit: s/public/private/

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
File content/shell/shell_browser_main.cc (right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
content/shell/shell_browser_main.cc:68: }
nit: move this out to a separate function in the other file too, so the
constants won't need to be exposed either...
bool GetLayoutTestURL(GURL* url) {
  ...
}

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
content/shell/shell_browser_main.cc:133: EnsureCreateAndroidFIFOs();
nit: just expose one function, like "InitializeForLayoutTestsIfNeeded()"

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
File content/shell/shell_layout_tests_android.h (right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
content/shell/shell_layout_tests_android.h:9: extern const char
kAndroidLayoutTestBase[];
as above, replace those with a nice looking function. :)

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
content/shell/shell_layout_tests_android.h:19: void
EnsureCreateAndroidNestedMessagePump();
also, combine both functions here.

[jochen (gone - plz use gerrit), 2013-06-19 17:58:47]

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
File content/shell/shell_browser_main.h (right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
content/shell/shell_browser_main.h:16: const
scoped_ptr<content::BrowserMainRunner>& main_runner);
nit. if you indent the second parameter like this, also put the first parameter
on its own line with 4 space indent (you should give clang-format a try :) )

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
File content/shell/shell_layout_tests_android.h (right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
content/shell/shell_layout_tests_android.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
nit. the latest and greatest copyright header doesn't have a (c) in it

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
content/shell/shell_layout_tests_android.h:8: extern const char
kAndroidLayoutTestPath[];
content_shell is in the content namespace

[Peter Beverloo, 2013-06-19 19:48:32]

Thanks for the quick reviews!

https://codereview.chromium.org/17265005/diff/4001/content/shell/android/java...
File
content/shell/android/java/src/org/chromium/content_shell/ShellLayoutTestUtils.java
(right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellLayoutTestUtils.java:13:
* Utility methods used when using content_shell for running Blink's layout
On 2013/06/19 17:57:42, bulach wrote:
> s/used when using/used by/

Done.

https://codereview.chromium.org/17265005/diff/4001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellLayoutTestUtils.java:17:
public class ShellLayoutTestUtils {
On 2013/06/19 17:57:42, bulach wrote:
> nit: s/public//

Done.

https://codereview.chromium.org/17265005/diff/4001/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellLayoutTestUtils.java:23:
public static String getApplicationFilesDirectory(Context appContext) {
On 2013/06/19 17:57:42, bulach wrote:
> nit: s/public/private/

Done. Weird, though, since we access it from outside of this class (albeit
through JNI).

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
File content/shell/shell_browser_main.cc (right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
content/shell/shell_browser_main.cc:68: }
On 2013/06/19 17:57:42, bulach wrote:
> nit: move this out to a separate function in the other file too, so the
> constants won't need to be exposed either...
> bool GetLayoutTestURL(GURL* url) {
>   ...
> }

Done.

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
content/shell/shell_browser_main.cc:133: EnsureCreateAndroidFIFOs();
On 2013/06/19 17:57:42, bulach wrote:
> nit: just expose one function, like "InitializeForLayoutTestsIfNeeded()"

Done.

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
File content/shell/shell_browser_main.h (right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_browse...
content/shell/shell_browser_main.h:16: const
scoped_ptr<content::BrowserMainRunner>& main_runner);
On 2013/06/19 17:58:47, jochen wrote:
> nit. if you indent the second parameter like this, also put the first
parameter
> on its own line with 4 space indent (you should give clang-format a try :) )

Done.

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
File content/shell/shell_layout_tests_android.h (right):

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
content/shell/shell_layout_tests_android.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/06/19 17:58:47, jochen wrote:
> nit. the latest and greatest copyright header doesn't have a (c) in it

Done.

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
content/shell/shell_layout_tests_android.h:8: extern const char
kAndroidLayoutTestPath[];
ShellBrowserMain() is not.  I've changed it for these files.

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
content/shell/shell_layout_tests_android.h:9: extern const char
kAndroidLayoutTestBase[];
On 2013/06/19 17:57:42, bulach wrote:
> as above, replace those with a nice looking function. :)

Done.

https://codereview.chromium.org/17265005/diff/4001/content/shell/shell_layout...
content/shell/shell_layout_tests_android.h:19: void
EnsureCreateAndroidNestedMessagePump();
On 2013/06/19 17:57:42, bulach wrote:
> also, combine both functions here.

Done.

[bulach, 2013-06-20 08:48:40]

lgtm for android, thanks!

just tiny nits below. feel free to go ahead once jochen is happy.

https://codereview.chromium.org/17265005/diff/7003/content/shell/shell_layout...
File content/shell/shell_layout_tests_android.cc (right):

https://codereview.chromium.org/17265005/diff/7003/content/shell/shell_layout...
content/shell/shell_layout_tests_android.cc:25: std::string
GetTestFilesDirectory(JNIEnv* env) {
nit: return a FilePath directly

https://codereview.chromium.org/17265005/diff/7003/content/shell/shell_layout...
content/shell/shell_layout_tests_android.cc:57: void
EnsureInitializeForAndroidLayoutTests() {
nit: just in case, could do with:

CHECK(CommandLine::ForCurrentProcess()->HasSwitch(switches::kDumpRenderTree));

[jochen (gone - plz use gerrit), 2013-06-20 10:31:21]

lgtm

[Peter Beverloo, 2013-06-20 10:43:45]

Thanks for the reviews!  All addressed.

https://codereview.chromium.org/17265005/diff/7003/content/shell/shell_layout...
File content/shell/shell_layout_tests_android.cc (right):

https://codereview.chromium.org/17265005/diff/7003/content/shell/shell_layout...
content/shell/shell_layout_tests_android.cc:25: std::string
GetTestFilesDirectory(JNIEnv* env) {
On 2013/06/20 08:48:41, bulach wrote:
> nit: return a FilePath directly

Done.

https://codereview.chromium.org/17265005/diff/7003/content/shell/shell_layout...
content/shell/shell_layout_tests_android.cc:57: void
EnsureInitializeForAndroidLayoutTests() {
On 2013/06/20 08:48:41, bulach wrote:
> nit: just in case, could do with:
> 
> CHECK(CommandLine::ForCurrentProcess()->HasSwitch(switches::kDumpRenderTree));

Done.

[bulach, 2013-06-20 11:00:04]

lgtm, thanks!

[commit-bot: I haz the power, 2013-06-20 11:22:20]

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

[commit-bot: I haz the power, 2013-06-20 21:08:12]

Change committed as 207562