[Android] Redirect std{in,out,err} to sockets for layout tests.

[Android] Redirect std{in,out,err} to sockets for layout tests.

We've used FIFOs on the device to handle stdin, stdout, and stderr
in the past, with persistent adb shells writing to and read from
(respectively) those FIFOs. They're prone to deadlocks and other
unusual issues, though.

This CL instead redirects those three streams to different
sockets set up by run-webkit-tests and forwarded from the device
to the host. This should eliminate the occasional problems caused
by FIFOs and work more seamlessly with M and above.

BUG=567947
Review-Url: https://codereview.chromium.org/2540603004
Cr-Commit-Position: refs/heads/master@{#442340}
Committed: https://chromium.googlesource.com/chromium/src/+/2d40c48d8a1ba516c866d7d9a5e789eb9b011cb2

[jbudorick, 2016-12-01 14:53:30]

jbudorick@chromium.org changed reviewers:
+ dpranke@chromium.org, qyearsley@chromium.org

[jbudorick, 2016-12-01 18:58:11]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 18:58:34]

Dry run: 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, 2016-12-01 19:54:08]

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

[commit-bot: I haz the power, 2016-12-01 19:54:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbudorick, 2016-12-01 20:58:22]

On 2016/12/01 14:53:31, jbudorick wrote:

er, hold off on this until I look into the trybot errors. Not sure why
android_blink_rel when red in this patchset.

[jbudorick, 2016-12-02 02:00:53]

Patchset #5 (id:80001) has been deleted

[jbudorick, 2016-12-02 07:45:41]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 07:46:04]

Dry run: 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, 2016-12-02 10:05:06]

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

[commit-bot: I haz the power, 2016-12-02 10:05:07]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2016-12-02 17:17:25]

On 2016/12/01 20:58:22, jbudorick wrote:
> On 2016/12/01 14:53:31, jbudorick wrote:
> 
> er, hold off on this until I look into the trybot errors. Not sure why
> android_blink_rel when red in this patchset.

and we're back. It looks like there are issues w/ how AdbShellCommandFailedError
is pickled when not running inline, but those aren't new in this patch. I'll
look at those separately.

ptal

[qyearsley, 2016-12-02 23:35:22]

Note: The diagrams in
https://docs.google.com/a/chromium.org/document/d/1C2VRAyEjL-I0a4ZQqPQ7ZiWc7L...
are helpful when looking at this change.

This is pretty complex, and most of it also seems pretty hard to unit-test.

Are you testing this by running layout tests on an android device connected to
your workstation?

https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
(right):

https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:925: #
Read back the shell prompt to ensure adb shell ready.
s/ensure adb shell ready/ensure adb shell is ready/

https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:936: #
Start a process to read from the stderr fifo of the test driver and print to
stdout.
What is the meaning of fifo in this context?

[jbudorick, 2016-12-05 14:29:49]

On 2016/12/02 23:35:22, qyearsley wrote:
> Note: The diagrams in
>
https://docs.google.com/a/chromium.org/document/d/1C2VRAyEjL-I0a4ZQqPQ7ZiWc7L...
> are helpful when looking at this change.

Yeah, I should've mentioned this explicitly when I sent out the review. Thanks
for doing so.

> 
> This is pretty complex, and most of it also seems pretty hard to unit-test.
> 
> Are you testing this by running layout tests on an android device connected to
> your workstation?

That and the android_blink_rel trybot.

> 
>
https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
> File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
> (right):
> 
>
https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:925: #
> Read back the shell prompt to ensure adb shell ready.
> s/ensure adb shell ready/ensure adb shell is ready/
> 
>
https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:936: #
> Start a process to read from the stderr fifo of the test driver and print to
> stdout.
> What is the meaning of fifo in this context?

[jbudorick, 2016-12-05 14:30:38]

https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
(right):

https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:925: #
Read back the shell prompt to ensure adb shell ready.
On 2016/12/02 23:35:22, qyearsley wrote:
> s/ensure adb shell ready/ensure adb shell is ready/

Done.

https://codereview.chromium.org/2540603004/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:936: #
Start a process to read from the stderr fifo of the test driver and print to
stdout.
On 2016/12/02 23:35:22, qyearsley wrote:
> What is the meaning of fifo in this context?

It means that I didn't update this comment or the one above :|

Fixed.

[Dirk Pranke, 2016-12-06 02:38:49]

dpranke@chromium.org changed reviewers:
+ peter@chromium.org, tedchoc@chromium.org

[Dirk Pranke, 2016-12-06 02:38:50]

lgtm. 

I'd like qyearsley@ to be comfortable w/ the python code as well.

And you'll probably need a review from peter@ or tedchoc@ for the C++ and Java
code, since I'm not an OWNER of those.

[Peter Beverloo, 2016-12-06 19:24:06]

Thanks for doing this, John!

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_android.cc (right):

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:12: #include
"base/android/fifo_utils.h"
The entirety of fifo_utils (and implementation) are now unused! Mind deleting
them? :)

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:47:
ScopedAndroidConfiguration::ScopedAndroidConfiguration() : sockets_() {}
nit - no need to explicitly construct |sockets_|. The compiler does that for
you. I'd use:

  ScopedAndroidConfiguration::ScopedAndroidConfiguration() = default;

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:49: void
ConnectCompleted(base::Closure socket_connected, int rv) {
nit: please keep the functions that aren't declared in the header file in the
anonymous namespace. That avoids accidental namespace pollution.

(It's not unreasonable that somebody else may also try to define e.g.
content::RedirectStream(), causing odd linking errors.)

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:58:
base::Callback<void(int)> socket_connected) {
nit - avoid the base::Callback refcount churn by marking the base::Callback
parameters as const& (here and elsewhere)

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:70: result =
socket->Open(AF_INET);
nit - you can combine these lines:

  int result = socket->Open(AF_INET);

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:76: const int flags =
fcntl(socket->socket_fd(), F_GETFL);
nit - check for the error condition for the fcntl calls:

  CHECK_NE(flags, -1);

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:84: if (result !=
net::ERR_IO_PENDING) {
nit - what about other values for |result|? Should we CHECK that |result| is
either net::ERR_IO_PENDING or net::OK?

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:123:
content::ScopedAllowWaitForAndroidLayoutTests allow_wait;
micro nit (4x in this file) - you're in content::, no need to specifically
qualify that

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:131:
base::Unretained(this));
nit: add a comment explaining that it's safe to use base::Unretained() because
the code's executed "synchronously".

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_android.h (right):

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.h:8: #include <string>
micro nit:
  #include <memory>

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.h:39: void
EnsureInitializeForAndroidLayoutTests();
This function is now unused -- can we delete it?

If we can, you might want to rename the file to
c/s/b/l_t/scoped_android_configuration.h or similar, since that's it's sole
occupant at that point.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_browser_main.cc (right):

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_browser_main.cc:138:
android_configuration.RedirectStreams();
What's the reason that you're splitting up the constructor, Initialize() and
RedirectStreams()?

https://codereview.chromium.org/2540603004/diff/110001/content/shell/common/l...
File content/shell/common/layout_test/layout_test_switches.h (right):

https://codereview.chromium.org/2540603004/diff/110001/content/shell/common/l...
content/shell/common/layout_test/layout_test_switches.h:16: extern const char
kAndroidStderrPort[];
nit: it's common convention for switches that only work on particular platforms
to be guarded by an #ifdef -

  #if defined(OS_ANDROID)

[jbudorick, 2016-12-08 02:04:13]

My c++ is clearly a bit rusty. Thanks for the thorough review!

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_android.cc (right):

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:12: #include
"base/android/fifo_utils.h"
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> The entirety of fifo_utils (and implementation) are now unused! Mind deleting
> them? :)

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:47:
ScopedAndroidConfiguration::ScopedAndroidConfiguration() : sockets_() {}
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> nit - no need to explicitly construct |sockets_|. The compiler does that for
> you. I'd use:
> 
>   ScopedAndroidConfiguration::ScopedAndroidConfiguration() = default;

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:49: void
ConnectCompleted(base::Closure socket_connected, int rv) {
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> nit: please keep the functions that aren't declared in the header file in the
> anonymous namespace. That avoids accidental namespace pollution.
> 
> (It's not unreasonable that somebody else may also try to define e.g.
> content::RedirectStream(), causing odd linking errors.)

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:58:
base::Callback<void(int)> socket_connected) {
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> nit - avoid the base::Callback refcount churn by marking the base::Callback
> parameters as const& (here and elsewhere)

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:70: result =
socket->Open(AF_INET);
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> nit - you can combine these lines:
> 
>   int result = socket->Open(AF_INET);

I imagine there was some reason I had them split at some point ... at least, I
hope there was.

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:76: const int flags =
fcntl(socket->socket_fd(), F_GETFL);
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> nit - check for the error condition for the fcntl calls:
> 
>   CHECK_NE(flags, -1);

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:84: if (result !=
net::ERR_IO_PENDING) {
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> nit - what about other values for |result|? Should we CHECK that |result| is
> either net::ERR_IO_PENDING or net::OK?

These currently get checked in the callback s.t. they get handled in either
case.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:123:
content::ScopedAllowWaitForAndroidLayoutTests allow_wait;
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> micro nit (4x in this file) - you're in content::, no need to specifically
> qualify that

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.cc:131:
base::Unretained(this));
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> nit: add a comment explaining that it's safe to use base::Unretained() because
> the code's executed "synchronously".

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_android.h (right):

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.h:8: #include <string>
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> micro nit:
>   #include <memory>

Done.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_android.h:39: void
EnsureInitializeForAndroidLayoutTests();
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> This function is now unused -- can we delete it?

Done.

> 
> If we can, you might want to rename the file to
> c/s/b/l_t/scoped_android_configuration.h or similar, since that's it's sole
> occupant at that point.

Done in the second patchset.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_browser_main.cc (right):

https://codereview.chromium.org/2540603004/diff/110001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_browser_main.cc:138:
android_configuration.RedirectStreams();
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> What's the reason that you're splitting up the constructor, Initialize() and
> RedirectStreams()?

- Initialize has to run before main_runner->Initialize s.t. it can set up
NestedMessageLoopAndroid
- RedirectStreams has to run after main_runner->Initialize s.t. the thread task
runner to which it posts tasks has been initialized.

That said, the constructor and Initialize can be combined.

https://codereview.chromium.org/2540603004/diff/110001/content/shell/common/l...
File content/shell/common/layout_test/layout_test_switches.h (right):

https://codereview.chromium.org/2540603004/diff/110001/content/shell/common/l...
content/shell/common/layout_test/layout_test_switches.h:16: extern const char
kAndroidStderrPort[];
On 2016/12/06 19:24:06, Peter Beverloo wrote:
> nit: it's common convention for switches that only work on particular
platforms
> to be guarded by an #ifdef -
> 
>   #if defined(OS_ANDROID)

Done.

https://codereview.chromium.org/2540603004/diff/150001/testing/android/native...
File testing/android/native_test/native_test_launcher.cc (right):

https://codereview.chromium.org/2540603004/diff/150001/testing/android/native...
testing/android/native_test/native_test_launcher.cc:107: if
(freopen(stdout_file_path.value().c_str(), "a+", stdout) == NULL) {
This was the last remaining use of fifo_utils.

[Peter Beverloo, 2016-12-13 12:15:34]

lgtm, apologies for the delay and once again thanks for doing this! :)

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
File content/shell/browser/layout_test/scoped_android_configuration.cc (right):

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
micro nit: 2016

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.cc:45: <<
net::ErrorToString(rv);
nit: here and elsewhere -- we no longer use detailed messages for the CHECK()
macro.

In short, they get optimized out for official builds, which means that there's
very little benefit from the detailed messages in reality. This is subtly
different in this case, since layout tests will be ran in release mode, so maybe
use LOG(FATAL) instead of CHECK()?

See https://codereview.chromium.org/2561963002/ and https://crbug.com/672699

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.cc:61:
std::unique_ptr<net::SocketPosix> socket(new net::SocketPosix);
micro nit - prefer the following as it avoids the `new`:

  std::unique_ptr<net::SocketPosix> socket =
      base::MakeUnique<net::SocketPosix>();

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
File content/shell/browser/layout_test/scoped_android_configuration.h (right):

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
micro nit: 2016

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.h:33: };
micro nit:

  DISALLOW_COPY_AND_ASSIGN(ScopedAndroidConfiguration)

from base/macros.h

[jbudorick, 2016-12-14 02:21:59]

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
File content/shell/browser/layout_test/scoped_android_configuration.cc (right):

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2016/12/13 12:15:34, Peter Beverloo wrote:
> micro nit: 2016

Done.

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.cc:45: <<
net::ErrorToString(rv);
On 2016/12/13 12:15:34, Peter Beverloo wrote:
> nit: here and elsewhere -- we no longer use detailed messages for the CHECK()
> macro.
> 
> In short, they get optimized out for official builds, which means that there's
> very little benefit from the detailed messages in reality. This is subtly
> different in this case, since layout tests will be ran in release mode, so
maybe
> use LOG(FATAL) instead of CHECK()?
> 
> See https://codereview.chromium.org/2561963002/ and https://crbug.com/672699

Switched to LOG_IF(FATAL, condition) << "msg";

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.cc:61:
std::unique_ptr<net::SocketPosix> socket(new net::SocketPosix);
On 2016/12/13 12:15:34, Peter Beverloo wrote:
> micro nit - prefer the following as it avoids the `new`:
> 
>   std::unique_ptr<net::SocketPosix> socket =
>       base::MakeUnique<net::SocketPosix>();

Done.

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
File content/shell/browser/layout_test/scoped_android_configuration.h (right):

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2016/12/13 12:15:34, Peter Beverloo wrote:
> micro nit: 2016

Done.

https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
content/shell/browser/layout_test/scoped_android_configuration.h:33: };
On 2016/12/13 12:15:34, Peter Beverloo wrote:
> micro nit:
> 
>   DISALLOW_COPY_AND_ASSIGN(ScopedAndroidConfiguration)
> 
> from base/macros.h

Done.

[Ted C, 2016-12-14 19:22:59]

On 2016/12/14 02:21:59, jbudorick wrote:
>
https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
> File content/shell/browser/layout_test/scoped_android_configuration.cc
(right):
> 
>
https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
> content/shell/browser/layout_test/scoped_android_configuration.cc:1: //
> Copyright 2013 The Chromium Authors. All rights reserved.
> On 2016/12/13 12:15:34, Peter Beverloo wrote:
> > micro nit: 2016
> 
> Done.
> 
>
https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
> content/shell/browser/layout_test/scoped_android_configuration.cc:45: <<
> net::ErrorToString(rv);
> On 2016/12/13 12:15:34, Peter Beverloo wrote:
> > nit: here and elsewhere -- we no longer use detailed messages for the
CHECK()
> > macro.
> > 
> > In short, they get optimized out for official builds, which means that
there's
> > very little benefit from the detailed messages in reality. This is subtly
> > different in this case, since layout tests will be ran in release mode, so
> maybe
> > use LOG(FATAL) instead of CHECK()?
> > 
> > See https://codereview.chromium.org/2561963002/ and https://crbug.com/672699
> 
> Switched to LOG_IF(FATAL, condition) << "msg";
> 
>
https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
> content/shell/browser/layout_test/scoped_android_configuration.cc:61:
> std::unique_ptr<net::SocketPosix> socket(new net::SocketPosix);
> On 2016/12/13 12:15:34, Peter Beverloo wrote:
> > micro nit - prefer the following as it avoids the `new`:
> > 
> >   std::unique_ptr<net::SocketPosix> socket =
> >       base::MakeUnique<net::SocketPosix>();
> 
> Done.
> 
>
https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
> File content/shell/browser/layout_test/scoped_android_configuration.h (right):
> 
>
https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
> content/shell/browser/layout_test/scoped_android_configuration.h:1: //
Copyright
> 2013 The Chromium Authors. All rights reserved.
> On 2016/12/13 12:15:34, Peter Beverloo wrote:
> > micro nit: 2016
> 
> Done.
> 
>
https://codereview.chromium.org/2540603004/diff/150001/content/shell/browser/...
> content/shell/browser/layout_test/scoped_android_configuration.h:33: };
> On 2016/12/13 12:15:34, Peter Beverloo wrote:
> > micro nit:
> > 
> >   DISALLOW_COPY_AND_ASSIGN(ScopedAndroidConfiguration)
> > 
> > from base/macros.h
> 
> Done.

java - lgtm

[jbudorick, 2016-12-15 16:48:14]

On 2016/12/06 02:38:50, Dirk Pranke wrote:
> lgtm. 
> 
> I'd like qyearsley@ to be comfortable w/ the python code as well.

qyearsley: friendly ping

> 
> And you'll probably need a review from peter@ or tedchoc@ for the C++ and Java
> code, since I'm not an OWNER of those.

[qyearsley, 2016-12-15 19:21:02]

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
(right):

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:923:
check_return=True)
Maybe some parts of this large function would be easier to read if parts were
extracted to helper functions and given a name. Maybe this block above is a good
candidate for extraction?

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:927:
self._server_process.start()
What process does self._server_process refer to?

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:956:
self._device)
What happens when you "start the forwarder"? Does it start a sub-process? Are
there two or more host_forwarder processes started here, when Forwarder.Map is
called twice for different purposes?

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:959:
self._device)
It's not obvious from here that 0 means that a port of the device should be
allocated dynamically and can be obtained after allocation via
forwarder.Forwarder.DevicePortForHostPort; maybe a little comment would be
helpful.

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:1000: def
_start_netcat(self, server_name, first_port=10201, read_from_stdin=True):
Server name must always be one of {"ReadStderr", "ReadStdout", "WriteStdin"},
right?

Are these names arbitrary? (Could they just be changed in this file to something
else?) Or are these strings determined elsewhere?

Why is 10201 the first port to try?

What role does netcat play here?

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:1009:
return (proc, i)
What would happen hypothetically if this loop exits after counting to 65536 for
some reason?

[jbudorick, 2016-12-21 00:55:16]

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
(right):

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:923:
check_return=True)
On 2016/12/15 19:21:01, qyearsley wrote:
> Maybe some parts of this large function would be easier to read if parts were
> extracted to helper functions and given a name. Maybe this block above is a
good
> candidate for extraction?

Done.

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:927:
self._server_process.start()
On 2016/12/15 19:21:02, qyearsley wrote:
> What process does self._server_process refer to?

For Android, it's basically a host proxy process for the stdio streams to/from
ContentShell.apk. (I think it's an adb shell process for which we replace
std{in,out,err} after it starts up.)

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:956:
self._device)
On 2016/12/15 19:21:02, qyearsley wrote:
> What happens when you "start the forwarder"? Does it start a sub-process? Are
> there two or more host_forwarder processes started here, when Forwarder.Map is
> called twice for different purposes?

It starts a host_forwarder client process which then forks a daemon process if
one does not already exist. The call to Map waits for the client process to
terminate. One client process will be started (and finished) for each port being
mapped. Only one daemon process will be created, even if more than one port is
mapped.

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:959:
self._device)
On 2016/12/15 19:21:02, qyearsley wrote:
> It's not obvious from here that 0 means that a port of the device should be
> allocated dynamically and can be obtained after allocation via
> forwarder.Forwarder.DevicePortForHostPort; maybe a little comment would be
> helpful.

That or a constant in forwarder.py: https://codereview.chromium.org/2587153002/

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:1000: def
_start_netcat(self, server_name, first_port=10201, read_from_stdin=True):
On 2016/12/15 19:21:02, qyearsley wrote:
> Server name must always be one of {"ReadStderr", "ReadStdout", "WriteStdin"},
> right?

The three call sites provide those, but there's nothing that would break if we
renamed them.

> 
> Are these names arbitrary? (Could they just be changed in this file to
something
> else?) Or are these strings determined elsewhere?

Arbitrary.

> 
> Why is 10201 the first port to try?

No particular reason.

> 
> What role does netcat play here?

It opens a socket for ContentShell to connect to for the stdio streams and reads
from stdin + writes to stdout as appropriate.

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:1009:
return (proc, i)
On 2016/12/15 19:21:02, qyearsley wrote:
> What would happen hypothetically if this loop exits after counting to 65536
for
> some reason?

hmm, this would return None and we'd get a confusing-looking exception on trying
to unpack it.

Made this throw an Exception.

[jbudorick, 2017-01-04 15:27:56]

qyearsley: friendly ping

[qyearsley, 2017-01-04 19:00:32]

LGTM! Thanks for the explanations.

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
(right):

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:959:
self._device)
On 2016/12/21 at 00:55:16, jbudorick wrote:
> On 2016/12/15 19:21:02, qyearsley wrote:
> > It's not obvious from here that 0 means that a port of the device should be
> > allocated dynamically and can be obtained after allocation via
> > forwarder.Forwarder.DevicePortForHostPort; maybe a little comment would be
> > helpful.
> 
> That or a constant in forwarder.py:
https://codereview.chromium.org/2587153002/

I agree, that is good :-)

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:1000: def
_start_netcat(self, server_name, first_port=10201, read_from_stdin=True):
On 2016/12/21 at 00:55:16, jbudorick wrote:
> On 2016/12/15 19:21:02, qyearsley wrote:
> > Server name must always be one of {"ReadStderr", "ReadStdout",
"WriteStdin"},
> > right?
> 
> The three call sites provide those, but there's nothing that would break if we
renamed them.
> 
> > 
> > Are these names arbitrary? (Could they just be changed in this file to
something
> > else?) Or are these strings determined elsewhere?
> 
> Arbitrary.
> 
> > 
> > Why is 10201 the first port to try?
> 
> No particular reason.

Optionally, if you think it makes it clearer, we could add constants for the
server name strings and the first port to try, with a short comment saying that
they're arbitrary.

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:1009:
return (proc, i)
On 2016/12/21 at 00:55:16, jbudorick wrote:
> On 2016/12/15 19:21:02, qyearsley wrote:
> > What would happen hypothetically if this loop exits after counting to 65536
for
> > some reason?
> 
> hmm, this would return None and we'd get a confusing-looking exception on
trying to unpack it.
> 
> Made this throw an Exception.

SGTM

[jbudorick, 2017-01-04 19:50:15]

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
(right):

https://codereview.chromium.org/2540603004/diff/170001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py:1000: def
_start_netcat(self, server_name, first_port=10201, read_from_stdin=True):
On 2017/01/04 19:00:32, qyearsley wrote:
> On 2016/12/21 at 00:55:16, jbudorick wrote:
> > On 2016/12/15 19:21:02, qyearsley wrote:
> > > Server name must always be one of {"ReadStderr", "ReadStdout",
> "WriteStdin"},
> > > right?
> > 
> > The three call sites provide those, but there's nothing that would break if
we
> renamed them.
> > 
> > > 
> > > Are these names arbitrary? (Could they just be changed in this file to
> something
> > > else?) Or are these strings determined elsewhere?
> > 
> > Arbitrary.
> > 
> > > 
> > > Why is 10201 the first port to try?
> > 
> > No particular reason.
> 
> Optionally, if you think it makes it clearer, we could add constants for the
> server name strings and the first port to try, with a short comment saying
that
> they're arbitrary.

I think it helps for the port and have added a constant for that. I don't think
it does for the server names, though.

[jbudorick, 2017-01-04 19:50:30]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2017-01-04 19:50:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, peter@chromium.org, tedchoc@chromium.org,
qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/2540603004/#ps210001
(title: "qyearsley comments 3")

[commit-bot: I haz the power, 2017-01-04 19:51:05]

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-01-04 20:00:18]

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

[commit-bot: I haz the power, 2017-01-04 20:00:19]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jbudorick, 2017-01-04 21:52:26]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 21:53:06]

Dry run: 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-01-04 23:41:24]

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

[commit-bot: I haz the power, 2017-01-04 23:41:25]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2017-01-04 23:43:24]

+agrieve for base/BUILD.gn and base/android/* owners

[jbudorick, 2017-01-04 23:43:43]

jbudorick@chromium.org changed reviewers:
+ agrieve@chromium.org

[jbudorick, 2017-01-04 23:43:44]

...er, +agrieve for real

[agrieve, 2017-01-05 01:25:34]

On 2017/01/04 23:43:44, jbudorick wrote:
> ...er, +agrieve for real

lgtm. yay deleting!

[jbudorick, 2017-01-09 19:15:06]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 19:15:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbudorick, 2017-01-09 20:00:10]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2017-01-09 20:00:11]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2017-01-09 20:00:11]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dpranke@chromium.org, tedchoc@chromium.org, qyearsley@chromium.org,
agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2540603004/#ps250001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-09 20:00:47]

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-01-09 21:05:05]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1483992011265030,
"parent_rev": "d7a52b3918a150a715c181ef108da515d0577db8", "commit_rev":
"2d40c48d8a1ba516c866d7d9a5e789eb9b011cb2"}

[commit-bot: I haz the power, 2017-01-09 21:05:36]

Description was changed from

==========
[Android] Redirect std{in,out,err} to sockets for layout tests.

We've used FIFOs on the device to handle stdin, stdout, and stderr
in the past, with persistent adb shells writing to and read from
(respectively) those FIFOs. They're prone to deadlocks and other
unusual issues, though.

This CL instead redirects those three streams to different
sockets set up by run-webkit-tests and forwarded from the device
to the host. This should eliminate the occasional problems caused
by FIFOs and work more seamlessly with M and above.

BUG=567947
==========

to

==========
[Android] Redirect std{in,out,err} to sockets for layout tests.

We've used FIFOs on the device to handle stdin, stdout, and stderr
in the past, with persistent adb shells writing to and read from
(respectively) those FIFOs. They're prone to deadlocks and other
unusual issues, though.

This CL instead redirects those three streams to different
sockets set up by run-webkit-tests and forwarded from the device
to the host. This should eliminate the occasional problems caused
by FIFOs and work more seamlessly with M and above.

BUG=567947

Review-Url: https://codereview.chromium.org/2540603004
Cr-Commit-Position: refs/heads/master@{#442340}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d40c48d8a1ba516c866d7d9a5e7...
==========

[commit-bot: I haz the power, 2017-01-09 21:05:37]

Committed patchset #13 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/2d40c48d8a1ba516c866d7d9a5e7...