Move CreateFIFO() and RedirectStream() from testing/android/ to base/android/.

Move CreateFIFO() and RedirectStream() from testing/android/ to base/android/.

They will be used by content_shell to create the necessary fifos when running layout tests.

BUG=232044
TBR=darin,yfriedman

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

[scherkus (not reviewing), 2013-06-07 21:30:57]

nileshagrawal, peter: please take a look at everything

https://codereview.chromium.org/16599008/diff/7001/base/android/fifo_utils.h
File base/android/fifo_utils.h (right):

https://codereview.chromium.org/16599008/diff/7001/base/android/fifo_utils.h#...
base/android/fifo_utils.h:21: BASE_EXPORT bool CreateFIFO(const FilePath& path,
int mode);
I felt that the base/ variants of the tests functions a tad more general. In
other words, CreateFIFO() won't attempt to unlink() and both functions will not
log + crash if they fail.

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
File testing/android/native_test_launcher.cc (right):

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
testing/android/native_test_launcher.cc:81: void AndroidLog(int priority, const
char* format, ...) {
nileshagrawal: any reason why we can't use LOG()? these seems somewhat
duplicated from base/logging.cc

FWIW, I tested it out and it works.

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
testing/android/native_test_launcher.cc:98: exit(EXIT_FAILURE);
nileshagrawal: ditto here -- any reason we can't use CHECK() for these types of
helper functions?

[nilesh, 2013-06-08 02:11:03]

Marcus do you know if it is safe to use LOG here?

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
File testing/android/native_test_launcher.cc (right):

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
testing/android/native_test_launcher.cc:81: void AndroidLog(int priority, const
char* format, ...) {
On 2013/06/07 21:30:57, scherkus wrote:
> nileshagrawal: any reason why we can't use LOG()? these seems somewhat
> duplicated from base/logging.cc
> 
> FWIW, I tested it out and it works.

This was because logging::InitLogging in not called yet.
It is called later in TestSuite::Initialize

I have to look more closely to figure out if it is safe to use LOG here. bulach
may know better.

[bulach, 2013-06-10 13:18:40]

thanks both! some clarifications inline. bots are complaining about
content_browsertests, please make them happy ;)

to be clear: I don't think any of the problems I mentioned are hard
requirements, but we need to ensure all the test suites will be happy with this
change.

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
File testing/android/native_test_launcher.cc (right):

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
testing/android/native_test_launcher.cc:81: void AndroidLog(int priority, const
char* format, ...) {
On 2013/06/08 02:11:03, nilesh wrote:
> On 2013/06/07 21:30:57, scherkus wrote:
> > nileshagrawal: any reason why we can't use LOG()? these seems somewhat
> > duplicated from base/logging.cc
> > 
> > FWIW, I tested it out and it works.
> 
> This was because logging::InitLogging in not called yet.
> It is called later in TestSuite::Initialize
> 
> I have to look more closely to figure out if it is safe to use LOG here.
bulach
> may know better. 

that's right... the FIFO need(ed) to be setup as soon as possible so that test
harness in the host can capture it.. InitLogging and all other initializations
were done later on...

to be clear, this is not a hard-depedency, it can certainly be changed.. :) but
it may not be trivial.

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
testing/android/native_test_launcher.cc:92: file_util::Delete(path, false);
afaict, this has a ThreadRestrictions::AssertIOAllowed, which may kill the test
suite.. the previous unlink wouldn't have this issue..

again, not a hard-depedency, but it may fail on some test suites.

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
testing/android/native_test_launcher.cc:98: exit(EXIT_FAILURE);
On 2013/06/07 21:30:57, scherkus wrote:
> nileshagrawal: ditto here -- any reason we can't use CHECK() for these types
of
> helper functions?

as above, it's a chicken-and-egg problem :) we wanted to setup the environment
and start communicating with the harness as soon as possible... this requires
logging, command line, etc.., to be fully initialized before the we can start
communicating with the harness..

[scherkus (not reviewing), 2013-06-14 23:19:33]

PTAL

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
File testing/android/native_test_launcher.cc (right):

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
testing/android/native_test_launcher.cc:81: void AndroidLog(int priority, const
char* format, ...) {
On 2013/06/10 13:18:40, bulach wrote:
> On 2013/06/08 02:11:03, nilesh wrote:
> > On 2013/06/07 21:30:57, scherkus wrote:
> > > nileshagrawal: any reason why we can't use LOG()? these seems somewhat
> > > duplicated from base/logging.cc
> > > 
> > > FWIW, I tested it out and it works.
> > 
> > This was because logging::InitLogging in not called yet.
> > It is called later in TestSuite::Initialize
> > 
> > I have to look more closely to figure out if it is safe to use LOG here.
> bulach
> > may know better. 
> 
> that's right... the FIFO need(ed) to be setup as soon as possible so that test
> harness in the host can capture it.. InitLogging and all other initializations
> were done later on...
> 
> to be clear, this is not a hard-depedency, it can certainly be changed.. :)
but
> it may not be trivial.

Gotcha -- I didn't notice before that the RunTests() code actually runs before
main()

https://codereview.chromium.org/16599008/diff/7001/testing/android/native_tes...
testing/android/native_test_launcher.cc:92: file_util::Delete(path, false);
On 2013/06/10 13:18:40, bulach wrote:
> afaict, this has a ThreadRestrictions::AssertIOAllowed, which may kill the
test
> suite.. the previous unlink wouldn't have this issue..
> 
> again, not a hard-depedency, but it may fail on some test suites.

If my understanding of when/how RunTests() is executed, there shouldn't have
been any ThreadRestrictions set on the thread by time we execute this code.

In any case, I'll get it a whirl on the try bots

https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
File content/shell/android/browsertests_apk/content_browser_tests_android.cc
(right):

https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
content/shell/android/browsertests_apk/content_browser_tests_android.cc:51:
static void RunTests(JNIEnv* env,
nilesh/bulach: this looks largely duplicated from the one inside of
testing/android/native_test_launcher.cc, modulo the activity name and some extra
command line flags

do you think there's the possibility of refactoring + reusing the RunTests()
from native_test_launcher.cc?

[bulach, 2013-06-17 16:34:50]

lgtm, although I'd have a minor preference of "unlink" rather than relying on
base and the fact the thread restriction are not set at that moment.. feel free
to go ahead once nilesh is happy! thanks!

https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
File content/shell/android/browsertests_apk/content_browser_tests_android.cc
(right):

https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
content/shell/android/browsertests_apk/content_browser_tests_android.cc:51:
static void RunTests(JNIEnv* env,
On 2013/06/14 23:19:33, scherkus wrote:
> nilesh/bulach: this looks largely duplicated from the one inside of
> testing/android/native_test_launcher.cc, modulo the activity name and some
extra
> command line flags
> 
> do you think there's the possibility of refactoring + reusing the RunTests()
> from native_test_launcher.cc?

I hope so :) but nilesh can give more details...
(I think what happened was that content_browser_tests turned out to be simpler /
more similar to other tests than we originally thought...)

[nilesh, 2013-06-17 17:04:39]

https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
File content/shell/android/browsertests_apk/content_browser_tests_android.cc
(right):

https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
content/shell/android/browsertests_apk/content_browser_tests_android.cc:51:
static void RunTests(JNIEnv* env,
On 2013/06/17 16:34:50, bulach wrote:
> On 2013/06/14 23:19:33, scherkus wrote:
> > nilesh/bulach: this looks largely duplicated from the one inside of
> > testing/android/native_test_launcher.cc, modulo the activity name and some
> extra
> > command line flags
> > 
> > do you think there's the possibility of refactoring + reusing the RunTests()
> > from native_test_launcher.cc?
> 
> I hope so :) but nilesh can give more details...
> (I think what happened was that content_browser_tests turned out to be simpler
/
> more similar to other tests than we originally thought...)

This code is similar (and simpler as it is tailored for content_browsertests).
We can move some more code to native_test_util.

[nilesh, 2013-06-17 22:36:08]

On 2013/06/17 17:04:39, nilesh wrote:
>
https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
> File content/shell/android/browsertests_apk/content_browser_tests_android.cc
> (right):
> 
>
https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
> content/shell/android/browsertests_apk/content_browser_tests_android.cc:51:
> static void RunTests(JNIEnv* env,
> On 2013/06/17 16:34:50, bulach wrote:
> > On 2013/06/14 23:19:33, scherkus wrote:
> > > nilesh/bulach: this looks largely duplicated from the one inside of
> > > testing/android/native_test_launcher.cc, modulo the activity name and some
> > extra
> > > command line flags
> > > 
> > > do you think there's the possibility of refactoring + reusing the
RunTests()
> > > from native_test_launcher.cc?
> > 
> > I hope so :) but nilesh can give more details...
> > (I think what happened was that content_browser_tests turned out to be
simpler
> /
> > more similar to other tests than we originally thought...)
> 
> This code is similar (and simpler as it is tailored for content_browsertests).
> We can move some more code to native_test_util.

lgtm

[scherkus (not reviewing), 2013-06-17 22:44:04]

On 2013/06/17 16:34:50, bulach wrote:
> lgtm, although I'd have a minor preference of "unlink" rather than relying on
> base and the fact the thread restriction are not set at that moment.. feel
free
> to go ahead once nilesh is happy! thanks!

Yeah ... given that most of the Chromium environment is uninitialized at this
point I switched to unlink()

[scherkus (not reviewing), 2013-06-17 22:44:10]

https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
File content/shell/android/browsertests_apk/content_browser_tests_android.cc
(right):

https://codereview.chromium.org/16599008/diff/25001/content/shell/android/bro...
content/shell/android/browsertests_apk/content_browser_tests_android.cc:51:
static void RunTests(JNIEnv* env,
On 2013/06/17 17:04:39, nilesh wrote:
> On 2013/06/17 16:34:50, bulach wrote:
> > On 2013/06/14 23:19:33, scherkus wrote:
> > > nilesh/bulach: this looks largely duplicated from the one inside of
> > > testing/android/native_test_launcher.cc, modulo the activity name and some
> > extra
> > > command line flags
> > > 
> > > do you think there's the possibility of refactoring + reusing the
RunTests()
> > > from native_test_launcher.cc?
> > 
> > I hope so :) but nilesh can give more details...
> > (I think what happened was that content_browser_tests turned out to be
simpler
> /
> > more similar to other tests than we originally thought...)
> 
> This code is similar (and simpler as it is tailored for content_browsertests).
> We can move some more code to native_test_util.
>  

Added TODO

[commit-bot: I haz the power, 2013-06-17 22:49:10]

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

[commit-bot: I haz the power, 2013-06-17 23:04:13]

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

[scherkus (not reviewing), 2013-06-17 23:08:15]

darin: OWNERS for base/base.gypi
yfriedman: OWNERS for content/shell/android/

[scherkus (not reviewing), 2013-06-18 17:21:35]

ping darin/yfriedman on OWNERS

[scherkus (not reviewing), 2013-06-18 17:24:46]

TBR'ing!

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

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

[commit-bot: I haz the power, 2013-06-18 19:34:36]

Change committed as 207060