Refactor android_webview_shell and its tests

refactor android_webview_shell and add page cycler

The previous android_webview/toos/WebViewShell directory is now separated into:

android_webview/tools/WebViewShell/
android_webview/tools/WebViewShellTest/

with android_webview/tools/PageCycler/ added

The target name are changed to system_webview_shell_apk, system_webview_shell_layout_test_apk, and system_webview_shell_page_cycler_apk.

BUG=

Committed: https://crrev.com/2a9e4d1100e6b15d591f193c152bb705774c9c38
Cr-Commit-Position: refs/heads/master@{#353105}

[Yoland Yan(Google), 2015-09-22 23:22:01]

yolandyan@google.com changed reviewers:
+ boliu@chromium.org, mikecase@chromium.org, timvolodine@chromium.org

[Yoland Yan(Google), 2015-09-22 23:23:06]

On 2015/09/22 at 23:22:01, yolandyan wrote:
> 

Guys, I think this is the last time I will come and bother you with page cyler.
Thanks for all the help!

Best,
Yoland

[boliu, 2015-09-22 23:25:29]

On 2015/09/22 23:23:06, yolandyan wrote:
> On 2015/09/22 at 23:22:01, yolandyan wrote:
> > 
> 
> Guys, I think this is the last time I will come and bother you with page
cyler.

Famous last words..

I'll let tim give this a first pass. Tim can own WebViewShellTest directory as
well probably

> Thanks for all the help!
> 
> Best,
> Yoland

[timvolodine, 2015-09-23 15:46:29]

nice! thanks Yoland.
a couple of questions below. with this we could probably also drop only the
'webview browser' part into AOSP android distribution.

yes, could you add me as OWNERs in WebViewShellTest/ please. and maybe yourself
in the pagecycler directory.

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
File android_webview/android_webview_shell.gyp (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:23: 'target_name':
'android_webview_shell_apk_java',
is this needed? can we just depend on android_webview_shell_apk in targets
below?

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:60: 'test_type': 'instrumentation',
add 'isolate_file' here as previously?

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
File android_webview/tools/WebViewShell/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/AndroidManifest.xml:93: <uses-library
android:name="android.test.runner" />
can this be removed as well?

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
File android_webview/tools/WebViewShellTest/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShellTest/AndroidManifest.xml:35:
android:label="Page cycler for org.chromium.webview_shell" />
-> android:label="Tests for org.chromium.webview_shell.shell"

[boliu, 2015-09-23 16:02:36]

Tim: does this need to be coordinated with the a bot side change for new
shell/test target etc? /me knows nothing about bots..

[timvolodine, 2015-09-23 16:07:57]

On 2015/09/23 16:02:36, boliu wrote:
> Tim: does this need to be coordinated with the a bot side change for new
> shell/test target etc? /me knows nothing about bots..

Bo: yes good point, I'll update the "Android WebView L" bot recipe once the
change lands

[boliu, 2015-09-23 16:59:07]

Add new targets to build/all.gyp, otherwise bots won't build new targets

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
File android_webview/android_webview_shell.gyp (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:23: 'target_name':
'android_webview_shell_apk_java',
On 2015/09/23 15:46:29, timvolodine wrote:
> is this needed? can we just depend on android_webview_shell_apk in targets
> below?

Yeah we looked this up. apk_fake_jar.gypi is a common pattern for splitting
foo_apk and foo_test_apk. apk_fake_jar.gypi pulls in some rules into dependent
targets, so it's not completley no-op.

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:42: 'android_manifest_path':
'tools/PageCycler/AndroidManifest.xml',
Let's nitpick about paths.. tools now feels really really wrong. Maybe have
something like
android_webview/WebViewShell/apk (or /Shell?)
android_webview/WebViewShell/LayoutTest
android_webview/WebViewShell/PageCycler

Tim: WDYT?

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/P...
File android_webview/tools/PageCycler/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/P...
android_webview/tools/PageCycler/AndroidManifest.xml:10:
package="org.chromium.webview_shell.test"
.page_cycler instead of .test?

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
File android_webview/tools/WebViewShell/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/AndroidManifest.xml:9:
package="org.chromium.webview_shell.shell"
is this needed? the extra .shell seems redundant

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/shell/PageCyclerTestActivity.java
(right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/shell/PageCyclerTestActivity.java:43:
mPageFinished = true;
this needs to be inside synchronized

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/shell/PageCyclerTestActivity.java:55:
mReceivedError = true;
ditto

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/shell/PageCyclerTestActivity.java:71:
synchronized (mLock) {
this belongs in the test apk, the activity can expose hooks to help the test,
like let the test set a callback for on page received, or just let the test set
its own WebViewClient. then all the threading/locking stuff can happen in the
test instead

also the thing you are looking for is CallbackHelper, see if you can reuse that
instead of rolling your own synchronization stuff

[timvolodine, 2015-09-23 17:34:31]

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
File android_webview/android_webview_shell.gyp (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:42: 'android_manifest_path':
'tools/PageCycler/AndroidManifest.xml',
On 2015/09/23 16:59:07, boliu wrote:
> Let's nitpick about paths.. tools now feels really really wrong. Maybe have
> something like
> android_webview/WebViewShell/apk (or /Shell?)
> android_webview/WebViewShell/LayoutTest
> android_webview/WebViewShell/PageCycler
> 
> Tim: WDYT?

looking at the naming in android_webview/ is lowercase so it should probably be
android_webview/webview_shell/ (and hope it'll not get confused with
android_webview/test/shell/)

/apk naming sounds good.
/LayoutTests (probably better -> /layout_tests) -- there are a couple of 'not
really layout tests' in the blink sense, but I guess it's close enough to call
it like that.

[boliu, 2015-09-23 17:48:28]

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
File android_webview/android_webview_shell.gyp (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:42: 'android_manifest_path':
'tools/PageCycler/AndroidManifest.xml',
On 2015/09/23 17:34:31, timvolodine wrote:
> On 2015/09/23 16:59:07, boliu wrote:
> > Let's nitpick about paths.. tools now feels really really wrong. Maybe have
> > something like
> > android_webview/WebViewShell/apk (or /Shell?)
> > android_webview/WebViewShell/LayoutTest
> > android_webview/WebViewShell/PageCycler
> > 
> > Tim: WDYT?
> 
> looking at the naming in android_webview/ is lowercase so it should probably
be
> android_webview/webview_shell/ (and hope it'll not get confused with
> android_webview/test/shell/)
> 
> /apk naming sounds good.
> /LayoutTests (probably better -> /layout_tests) -- there are a couple of 'not
> really layout tests' in the blink sense, but I guess it's close enough to call
> it like that.

nitpick more... "webview_shell" vs "system_webview_shell", thoughts? If the
latter, then the target should renamed to "system_webview_shell_apk" to be
consistent. But on the other hand, the whole "system_webview" name is
unfortunate, because "android_webview" was already taken at the time.

Is the cost of renaming the shell target to system_webview high? I mean this
patch already needs bot-side changs.

[timvolodine, 2015-09-23 18:07:26]

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
File android_webview/android_webview_shell.gyp (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:42: 'android_manifest_path':
'tools/PageCycler/AndroidManifest.xml',
On 2015/09/23 17:48:28, boliu wrote:
> On 2015/09/23 17:34:31, timvolodine wrote:
> > On 2015/09/23 16:59:07, boliu wrote:
> > > Let's nitpick about paths.. tools now feels really really wrong. Maybe
have
> > > something like
> > > android_webview/WebViewShell/apk (or /Shell?)
> > > android_webview/WebViewShell/LayoutTest
> > > android_webview/WebViewShell/PageCycler
> > > 
> > > Tim: WDYT?
> > 
> > looking at the naming in android_webview/ is lowercase so it should probably
> be
> > android_webview/webview_shell/ (and hope it'll not get confused with
> > android_webview/test/shell/)
> > 
> > /apk naming sounds good.
> > /LayoutTests (probably better -> /layout_tests) -- there are a couple of
'not
> > really layout tests' in the blink sense, but I guess it's close enough to
call
> > it like that.
> 
> nitpick more... "webview_shell" vs "system_webview_shell", thoughts? If the
> latter, then the target should renamed to "system_webview_shell_apk" to be
> consistent. But on the other hand, the whole "system_webview" name is
> unfortunate, because "android_webview" was already taken at the time.
> 
> Is the cost of renaming the shell target to system_webview high? I mean this
> patch already needs bot-side changs.

Right, I thought about that as well ;)

My initial impression was that system_webview_shell is slightly too verbose, but
it does seem to distinguish it from 'non-system' webview i.e.
android_webview_apk, so would be more precise because we have the
system_webview_apk target. My concern is that system_webview_shell can be
interpreted as a 'system' webview_shell (instead of 'system_webview' shell), as
some kind of shell running in the system.. maybe that's too far fetched though
;)

yeah don't think renaming cost should be a concern in this case.

[Yoland Yan(Google), 2015-09-23 19:51:52]

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
File android_webview/android_webview_shell.gyp (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:42: 'android_manifest_path':
'tools/PageCycler/AndroidManifest.xml',
On 2015/09/23 at 18:07:26, timvolodine wrote:
> On 2015/09/23 17:48:28, boliu wrote:
> > On 2015/09/23 17:34:31, timvolodine wrote:
> > > On 2015/09/23 16:59:07, boliu wrote:
> > > > Let's nitpick about paths.. tools now feels really really wrong. Maybe
have
> > > > something like
> > > > android_webview/WebViewShell/apk (or /Shell?)
> > > > android_webview/WebViewShell/LayoutTest
> > > > android_webview/WebViewShell/PageCycler
> > > > 
> > > > Tim: WDYT?
> > > 
> > > looking at the naming in android_webview/ is lowercase so it should
probably
> > be
> > > android_webview/webview_shell/ (and hope it'll not get confused with
> > > android_webview/test/shell/)
> > > 
> > > /apk naming sounds good.
> > > /LayoutTests (probably better -> /layout_tests) -- there are a couple of
'not
> > > really layout tests' in the blink sense, but I guess it's close enough to
call
> > > it like that.
> > 
> > nitpick more... "webview_shell" vs "system_webview_shell", thoughts? If the
> > latter, then the target should renamed to "system_webview_shell_apk" to be
> > consistent. But on the other hand, the whole "system_webview" name is
> > unfortunate, because "android_webview" was already taken at the time.
> > 
> > Is the cost of renaming the shell target to system_webview high? I mean this
> > patch already needs bot-side changs.
> 
> Right, I thought about that as well ;)
> 
> My initial impression was that system_webview_shell is slightly too verbose,
but it does seem to distinguish it from 'non-system' webview i.e.
android_webview_apk, so would be more precise because we have the
system_webview_apk target. My concern is that system_webview_shell can be
interpreted as a 'system' webview_shell (instead of 'system_webview' shell), as
some kind of shell running in the system.. maybe that's too far fetched though
;)
> 
> yeah don't think renaming cost should be a concern in this case.

Done

https://codereview.chromium.org/1364593002/diff/20001/android_webview/android...
android_webview/android_webview_shell.gyp:60: 'test_type': 'instrumentation',
On 2015/09/23 at 15:46:29, timvolodine wrote:
> add 'isolate_file' here as previously?

Done

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
File android_webview/tools/WebViewShell/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/AndroidManifest.xml:93: <uses-library
android:name="android.test.runner" />
On 2015/09/23 at 15:46:29, timvolodine wrote:
> can this be removed as well?

Done

https://codereview.chromium.org/1364593002/diff/80001/android_webview/android...
File android_webview/android_webview_shell.gyp (right):

https://codereview.chromium.org/1364593002/diff/80001/android_webview/android...
android_webview/android_webview_shell.gyp:7: 'target_name':
'android_webview_shell_apk',
should the name also change to system_webview_shell_apk?

https://codereview.chromium.org/1364593002/diff/80001/android_webview/android...
android_webview/android_webview_shell.gyp:10: 'apk_name': 'AndroidWebViewShell',
ditto

https://codereview.chromium.org/1364593002/diff/80001/android_webview/android...
android_webview/android_webview_shell.gyp:23: 'target_name':
'android_webview_shell_apk_java',
ditto

https://codereview.chromium.org/1364593002/diff/80001/android_webview/android...
android_webview/android_webview_shell.gyp:31: 'target_name':
'android_webview_shell_page_cycler_apk',
ditto

https://codereview.chromium.org/1364593002/diff/80001/android_webview/android...
android_webview/android_webview_shell.gyp:40: 'apk_name':
'AndroidWebViewShellPageCycler',
ditto

https://codereview.chromium.org/1364593002/diff/80001/android_webview/android...
android_webview/android_webview_shell.gyp:52: 'target_name':
'android_webview_shell_layout_test_apk',
ditto

https://codereview.chromium.org/1364593002/diff/80001/android_webview/android...
android_webview/android_webview_shell.gyp:61: 'apk_name':
'AndroidWebViewShellLayoutTest',
ditto

[Yoland Yan(Google), 2015-09-24 02:04:22]

Hi guys, 

I am separating the renaming cl from this cl so it will be easier for you guys
to review the java and xml file.

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/shell/PageCyclerTestActivity.java
(right):

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/shell/PageCyclerTestActivity.java:43:
mPageFinished = true;
On 2015/09/23 at 16:59:07, boliu wrote:
> this needs to be inside synchronized

Done

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/shell/PageCyclerTestActivity.java:55:
mReceivedError = true;
On 2015/09/23 at 16:59:07, boliu wrote:
> ditto

Done

https://codereview.chromium.org/1364593002/diff/20001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/shell/PageCyclerTestActivity.java:71:
synchronized (mLock) {
On 2015/09/23 at 16:59:07, boliu wrote:
> this belongs in the test apk, the activity can expose hooks to help the test,
like let the test set a callback for on page received, or just let the test set
its own WebViewClient. then all the threading/locking stuff can happen in the
test instead
> 
> also the thing you are looking for is CallbackHelper, see if you can reuse
that instead of rolling your own synchronization stuff

Done

[boliu, 2015-09-24 22:35:31]

much better, and let's decide on the name/move thing a bit later

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
File android_webview/tools/PageCycler/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/AndroidManifest.xml:10:
package="org.chromium.webview_shell.test"
Carrying over old comment

On 2015/09/23 16:59:07, boliu wrote:
> .page_cycler instead of .test?

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
File
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java
(right):

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:38:
mTestActivity = (PageCyclerTestActivity) getActivity();
this cast should not be needed

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:43:
mTestActivity.finish();
Wah? Why? And wrong thread

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:96:
private class PageCyclerWebViewClient extends WebViewClient {
static

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:103:
errorCallback = new CallbackHelper();
What's errorCallback? Does this actually compile?

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:128:
final WebView view = mTestActivity.getWebView();
Wrong thread. It's debatable whether it's thread safe or not, but I'd rather not
have that debate and just code it correctly.

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:143:
private void initializeSettings(WebSettings settings) {
nothing calls this

[Yoland Yan(Google), 2015-09-24 23:30:07]

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
File android_webview/tools/PageCycler/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/AndroidManifest.xml:10:
package="org.chromium.webview_shell.test"
On 2015/09/24 at 22:35:31, boliu wrote:
> Carrying over old comment
> 
> On 2015/09/23 16:59:07, boliu wrote:
> > .page_cycler instead of .test?

My bad, done

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
File
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java
(right):

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:38:
mTestActivity = (PageCyclerTestActivity) getActivity();
On 2015/09/24 at 22:35:31, boliu wrote:
> this cast should not be needed

Done

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:43:
mTestActivity.finish();
On 2015/09/24 at 22:35:31, boliu wrote:
> Wah? Why? And wrong thread

Done

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:96:
private class PageCyclerWebViewClient extends WebViewClient {
On 2015/09/24 at 22:35:31, boliu wrote:
> static

Done

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:103:
errorCallback = new CallbackHelper();
On 2015/09/24 at 22:35:31, boliu wrote:
> What's errorCallback? Does this actually compile?

Damn...my bad
Done

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:128:
final WebView view = mTestActivity.getWebView();
On 2015/09/24 at 22:35:31, boliu wrote:
> Wrong thread. It's debatable whether it's thread safe or not, but I'd rather
not have that debate and just code it correctly.

Done

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:128:
final WebView view = mTestActivity.getWebView();
On 2015/09/24 at 22:35:31, boliu wrote:
> Wrong thread. It's debatable whether it's thread safe or not, but I'd rather
not have that debate and just code it correctly.

Done

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/test/PageCyclerTest.java:143:
private void initializeSettings(WebSettings settings) {
On 2015/09/24 at 22:35:31, boliu wrote:
> nothing calls this

Done

[boliu, 2015-09-25 00:02:25]

I had a comment somewhere about adding new targets to build/all.gyp, which is
still not done.


Ok, so let's discuss renaming / moving things.

Moving files is transparent to bots, so I vote for doing that in a follow up.

But creating new targets (which this CL is doing) and renaming targets need to
be coordinated with bots, so I vote for doing the target renames in this CL to
minimize bot disruption.

Tim: Sounds good? If so, maybe you (or the infra folks here) can start preparing
the bot change so not to break the bot for too long?

[timvolodine, 2015-09-25 13:50:49]

On 2015/09/25 00:02:25, boliu wrote:
> I had a comment somewhere about adding new targets to build/all.gyp, which is
> still not done.
> 
> 
> Ok, so let's discuss renaming / moving things.
> 
> Moving files is transparent to bots, so I vote for doing that in a follow up.
> 
> But creating new targets (which this CL is doing) and renaming targets need to
> be coordinated with bots, so I vote for doing the target renames in this CL to
> minimize bot disruption.
> 
> Tim: Sounds good? If so, maybe you (or the infra folks here) can start
preparing
> the bot change so not to break the bot for too long?

sgtm
yes this patch will impact our WebViewL bot and probably our perf team as well
(I've cc'ed on the thread). So if we are going for the system_webview_shell
renaming it should happen in this patch right?
I'll have a look at the recipe, I think a couple of infra people are visiting
London next week so should have a quick turnaround either way.

[Yoland Yan(Google), 2015-09-25 23:05:12]

====RENAMING PATCH====

1. rename: tools/WebViewShell -> tools/system_webview/apk
2. rename: tools/WebViewShell/src/.../WebViewLayoutTest.java 
-> tools/system_webview/layout_tests/...

[Yoland Yan(Google), 2015-09-25 23:05:12]

====RENAMING PATCH====

1. rename: tools/WebViewShell -> tools/system_webview/apk
2. rename: tools/WebViewShell/src/.../WebViewLayoutTest.java 
-> tools/system_webview/layout_tests/...

[Yoland Yan(Google), 2015-09-25 23:05:13]

====RENAMING PATCH====

1. rename: tools/WebViewShell -> tools/system_webview/apk
2. rename: tools/WebViewShell/src/.../WebViewLayoutTest.java 
-> tools/system_webview/layout_tests/...

[Yoland Yan(Google), 2015-09-25 23:05:13]

====RENAMING PATCH====

1. rename: tools/WebViewShell -> tools/system_webview/apk
2. rename: tools/WebViewShell/src/.../WebViewLayoutTest.java 
-> tools/system_webview/layout_tests/...

[Yoland Yan(Google), 2015-09-25 23:05:13]

====RENAMING PATCH====

1. rename: tools/WebViewShell -> tools/system_webview/apk
2. rename: tools/WebViewShell/src/.../WebViewLayoutTest.java 
-> tools/system_webview/layout_tests/...

[Yoland Yan(Google), 2015-09-25 23:11:46]

On 2015/09/25 at 23:05:13, yolandyan wrote:
> ====RENAMING PATCH====
> 
> 1. rename: tools/WebViewShell -> tools/system_webview/apk
> 2. rename: tools/WebViewShell/src/.../WebViewLayoutTest.java 
> -> tools/system_webview/layout_tests/...

Sorry about the message spam!

[Yoland Yan(Google), 2015-09-25 23:58:57]

====Rename target====

changed target name from android_webview_shell_apk to system_webview_shell_apk,
and added new targets system_webview_shell_layout_test_apk, and
system_webview_shell_page_cycler_apk.

[boliu, 2015-09-26 00:07:53]

lgtm

please coordinate with tim/perezju/infra etc to make sure there are minimal bot
downtime

[mikecase (-- gone --), 2015-09-28 08:36:35]

You'll need to change the apk names and gyp targets in the files below and
commit the changes at the same time you commit this CL,

https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...

You'll need to checkout the Chromium infra repo to change these files, which I
can help with.

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
File android_webview/tools/WebViewShell/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/100001/android_webview/tools/...
android_webview/tools/WebViewShell/AndroidManifest.xml:88:
android:name="org.chromium.webview_shell.PageCyclerTestActivity"
I think you moved the test activities to a separate apk, so you should delete
these two <activity> entries (.PageCyclerTestActivity and
.WebViewLayoutTestActivity)

[timvolodine, 2015-09-28 11:59:17]

great, lgtm modulo nits

https://codereview.chromium.org/1364593002/diff/160001/android_webview/tools/...
File
android_webview/tools/PageCycler/src/org/chromium/webview_shell/page_cycler/PageCyclerTest.java
(right):

https://codereview.chromium.org/1364593002/diff/160001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/page_cycler/PageCyclerTest.java:145:
TIMEOUT_IN_SECS, TimeUnit.SECONDS);
nit: will this timeout when there is network error? or will it fail sooner? i.e.
we don't have to wait for page finished when there is error callback. (but I am
fine with this approach if it would complicate the code too much).

https://codereview.chromium.org/1364593002/diff/160001/android_webview/tools/...
File android_webview/tools/WebViewShellTest/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/160001/android_webview/tools/...
android_webview/tools/WebViewShellTest/AndroidManifest.xml:35:
android:label="Page cycler for org.chromium.webview_shell" />
"page cycler" -> layout tests

[timvolodine, 2015-09-28 12:19:24]

+cc: perezju@ making sure you see this: the AndroidWebViewShell.apk is changing
to SystemWebViewShell.apk

[Yoland Yan(Google), 2015-09-28 16:15:02]

yolandyan@google.com changed reviewers:
+ jbudorick@chromium.org

[Yoland Yan(Google), 2015-09-28 16:15:02]

Needs +jbudorick lgtm for build/all.gyp

https://codereview.chromium.org/1364593002/diff/160001/android_webview/tools/...
File
android_webview/tools/PageCycler/src/org/chromium/webview_shell/page_cycler/PageCyclerTest.java
(right):

https://codereview.chromium.org/1364593002/diff/160001/android_webview/tools/...
android_webview/tools/PageCycler/src/org/chromium/webview_shell/page_cycler/PageCyclerTest.java:145:
TIMEOUT_IN_SECS, TimeUnit.SECONDS);
On 2015/09/28 at 11:59:17, timvolodine wrote:
> nit: will this timeout when there is network error? or will it fail sooner?
i.e. we don't have to wait for page finished when there is error callback. (but
I am fine with this approach if it would complicate the code too much).

When there is a network error, I think pageFinishedCallback actually gets called
as well. Because when I tested with invalid url scheme, invalid url, or wifi
disabled. The pageFinishedCallback didn't wait for the time out.

https://codereview.chromium.org/1364593002/diff/160001/android_webview/tools/...
File android_webview/tools/WebViewShellTest/AndroidManifest.xml (right):

https://codereview.chromium.org/1364593002/diff/160001/android_webview/tools/...
android_webview/tools/WebViewShellTest/AndroidManifest.xml:35:
android:label="Page cycler for org.chromium.webview_shell" />
On 2015/09/28 at 11:59:17, timvolodine wrote:
> "page cycler" -> layout tests

Done

[timvolodine, 2015-09-30 18:19:26]

+cc:paulmiller@

[timvolodine, 2015-10-02 11:22:39]

https://codereview.chromium.org/1364593002/diff/200001/android_webview/androi...
File android_webview/android_webview_shell.gyp (right):

https://codereview.chromium.org/1364593002/diff/200001/android_webview/androi...
android_webview/android_webview_shell.gyp:66: 'isolate_file':
'android_webview_shell_test_apk.isolate',
please rename to correspond with the bot CL

[jbudorick, 2015-10-02 11:25:13]

Can webview be built by GN yet?

[boliu, 2015-10-02 15:52:20]

On 2015/10/02 11:25:13, jbudorick wrote:
> Can webview be built by GN yet?

Zero GN support. No .gn files in android_webview.

[jbudorick, 2015-10-02 15:53:10]

On 2015/10/02 15:52:20, boliu wrote:
> On 2015/10/02 11:25:13, jbudorick wrote:
> > Can webview be built by GN yet?
> 
> Zero GN support. No .gn files in android_webview.

womp womp. Q4?

regardless, build/ lgtm

[Yoland Yan(Google), 2015-10-02 19:52:38]

The CQ bit was checked by yolandyan@google.com

[Yoland Yan(Google), 2015-10-02 19:52:38]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/1364593002/#ps200001
(title: "minor fixs")

[commit-bot: I haz the power, 2015-10-02 19:54:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1364593002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1364593002/200001

[commit-bot: I haz the power, 2015-10-02 19:58:16]

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

[commit-bot: I haz the power, 2015-10-02 19:58:17]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Yoland Yan(Google), 2015-10-07 18:10:49]

The CQ bit was checked by yolandyan@google.com

[Yoland Yan(Google), 2015-10-07 18:10:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, boliu@chromium.org, timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/1364593002/#ps220001
(title: "rebase to origin/master")

[commit-bot: I haz the power, 2015-10-07 18:14:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1364593002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1364593002/220001

[commit-bot: I haz the power, 2015-10-07 19:34:00]

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

[commit-bot: I haz the power, 2015-10-07 19:34:01]

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

[Yoland Yan(Google), 2015-10-08 00:45:40]

yolandyan@google.com changed reviewers:
- boliu@chromium.org, timvolodine@chromium.org

[Yoland Yan(Google), 2015-10-08 00:45:41]

+jbudorick, I added a DEPS file under android_webview/tools/PageCycler/ to patch
12's failure on linux_chromium_rel_ng. Can you take a quick look?

[jbudorick, 2015-10-08 00:49:36]

On 2015/10/08 00:45:41, yolandyan wrote:
> +jbudorick, I added a DEPS file under android_webview/tools/PageCycler/ to
patch
> 12's failure on linux_chromium_rel_ng. Can you take a quick look?

mentioned offline, I'm not the right person to approve that.

[Yoland Yan(Google), 2015-10-08 00:55:33]

yolandyan@google.com changed reviewers:
+ timvolodine@chromium.org
- jbudorick@chromium.org

[Yoland Yan(Google), 2015-10-08 00:55:34]

+timvolodine, I added a DEPS file under android_webview/tools/PageCycler/ to fix
patch 12's failure on linux_chromium_rel_ng
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...).
Can you take a quick look before I commit?

[timvolodine, 2015-10-08 09:53:35]

On 2015/10/08 00:55:34, yolandyan wrote:
> +timvolodine, I added a DEPS file under android_webview/tools/PageCycler/ to
fix
> patch 12's failure on linux_chromium_rel_ng
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...).
> Can you take a quick look before I commit?

looks ok to me, since you depend on
content_shell_and_tests.gyp:content_java_test_support anyway. thanks!

[Yoland Yan(Google), 2015-10-08 16:27:26]

The CQ bit was checked by yolandyan@google.com

[Yoland Yan(Google), 2015-10-08 16:27:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, boliu@chromium.org, timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/1364593002/#ps260001
(title: " ")

[Yoland Yan(Google), 2015-10-08 16:27:36]

On 2015/10/08 at 09:53:35, timvolodine wrote:
> On 2015/10/08 00:55:34, yolandyan wrote:
> > +timvolodine, I added a DEPS file under android_webview/tools/PageCycler/ to
fix
> > patch 12's failure on linux_chromium_rel_ng
> >
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...).
> > Can you take a quick look before I commit?
> 
> looks ok to me, since you depend on
content_shell_and_tests.gyp:content_java_test_support anyway. thanks!

Thank you :)

[commit-bot: I haz the power, 2015-10-08 16:27:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1364593002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1364593002/260001

[commit-bot: I haz the power, 2015-10-08 16:41:25]

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

[commit-bot: I haz the power, 2015-10-08 16:41:26]

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

[Yoland Yan(Google), 2015-10-08 17:32:15]

yolandyan@google.com changed reviewers:
+ miguelg@chromium.org
- mikecase@chromium.org, timvolodine@chromium.org

[Miguel Garcia, 2015-10-08 17:32:29]

miguelg@chromium.org changed reviewers:
+ mikecase@chromium.org, timvolodine@chromium.org

[Miguel Garcia, 2015-10-08 17:32:30]

lgtm for the new page cycler DEPS

[Miguel Garcia, 2015-10-08 17:32:36]

lgtm for the new page cycler DEPS

[Yoland Yan(Google), 2015-10-08 17:37:05]

The CQ bit was checked by yolandyan@google.com

[commit-bot: I haz the power, 2015-10-08 17:40:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1364593002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1364593002/260001

[commit-bot: I haz the power, 2015-10-08 18:42:55]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-10-08 18:43:27]

Patchset 14 (id:??) landed as
https://crrev.com/2a9e4d1100e6b15d591f193c152bb705774c9c38
Cr-Commit-Position: refs/heads/master@{#353105}