Convert ChromeSyncShell to use ChromePublic infrastructure

Convert ChromeSyncShell to use ChromePublic infrastructure

Several test fixes are included to support this change.

Note: This CL only provides a GYP implementation (no GN). This is not a
regression from the previous state.

BUG=510211
Committed: https://crrev.com/62ca3f0326203cca221bba6f0ee038ff292ecc66
Cr-Commit-Position: refs/heads/master@{#344128}

[pval...(no longer on Chromium), 2015-08-11 01:45:41]

pvalenzuela@chromium.org changed reviewers:
+ newt@chromium.org

[pval...(no longer on Chromium), 2015-08-11 19:51:47]

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

[commit-bot: I haz the power, 2015-08-11 19:52:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1282603005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1282603005/1

[newt (away), 2015-08-11 21:25:30]

Looks generally good. A few comments inline.

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.gyp
File chrome/android/chrome_apk.gyp (right):

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.g...
chrome/android/chrome_apk.gyp:253: '../../base/base.gyp:base_java_test_support',
why are are all test-related deps required in ChromeSyncShell.apk? Shouldn't
they just be required in ChromeSyncShellTest.apk?

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.g...
chrome/android/chrome_apk.gyp:369:
'../../third_party/android_tools/android_tools.gyp:android_support_v7_appcompat_javalib',
remove this. shouldn't be needed.

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.g...
chrome/android/chrome_apk.gyp:370:
'../../ui/android/ui_android.gyp:ui_javatests',
Remove this dep. Including "ui_javatests" will cause all the ui javatests to be
included in ChromeShellSyncTest, which we don't need and will waste lots of bot
time.

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
File chrome/android/sync_shell/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
chrome/android/sync_shell/javatests/AndroidManifest.xml:22: <provider
android:name="org.chromium.chrome.test.TestContentProvider"
Remove all the providers, activities, receivers, etc that aren't used by the
tests in sync_shell. Having the same providers here as in ChromePublicTest, will
prevent ChromeShellTest.apk and ChromePublicTest.apk from being installed
side-by-side (which will break the bots).

Having duplicated activities and receivers won't cause problems, but it's unused
and unnecessary duplicated code.

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTestBase.java
(left):

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTestBase.java:45:
assertTrue("Clearing app data failed.", clearAppData());
Why remove clearAppData()? Clearing app data helps guarantee independence of
tests.

https://codereview.chromium.org/1282603005/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_sync_shell_main_delegate_staging_android.h
(right):

https://codereview.chromium.org/1282603005/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_sync_shell_main_delegate_staging_android.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
This could simply be called "chrome_main_delegate_sync_shell.h" or
"chrome_sync_shell_main_delegate.h".

In any case, remove all references to "staging". That's a now meaningless word
that's leftover from upstreaming and should be removed. (Merging
ChromeMainDelegateStagingAndroid into ChromeMainDelegateAndroid in on my TODO
list after you land this.) I think "android" can also be removed from the name,
given that "sync shell" doesn't exist on other platforms, and that this file
name doesn't clash with an existing file on other platforms.

[commit-bot: I haz the power, 2015-08-11 22:17:41]

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

[commit-bot: I haz the power, 2015-08-11 22:17:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[pval...(no longer on Chromium), 2015-08-12 23:25:59]

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.gyp
File chrome/android/chrome_apk.gyp (right):

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.g...
chrome/android/chrome_apk.gyp:253: '../../base/base.gyp:base_java_test_support',
On 2015/08/11 21:25:29, newt wrote:
> why are are all test-related deps required in ChromeSyncShell.apk? Shouldn't
> they just be required in ChromeSyncShellTest.apk?

I removed the test deps (see latest patchset), and now get this barrage Proguard
errors:

https://paste.googleplex.com/6085784813699072?raw

All of these look proto related. Obviously I can't add @VisibleForTesting in all
those spots. I tried disabling Proguard for this target and it didn't work. Is
there some known fix for this type of thing?

[newt (away), 2015-08-13 00:28:03]

On 2015/08/12 23:25:59, pvalenzuela wrote:
>
https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.gyp
> File chrome/android/chrome_apk.gyp (right):
> 
>
https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.g...
> chrome/android/chrome_apk.gyp:253:
'../../base/base.gyp:base_java_test_support',
> On 2015/08/11 21:25:29, newt wrote:
> > why are are all test-related deps required in ChromeSyncShell.apk? Shouldn't
> > they just be required in ChromeSyncShellTest.apk?
> 
> I removed the test deps (see latest patchset), and now get this barrage
Proguard
> errors:
> 
> https://paste.googleplex.com/6085784813699072?raw
> 
> All of these look proto related. Obviously I can't add @VisibleForTesting in
all
> those spots. I tried disabling Proguard for this target and it didn't work. Is
> there some known fix for this type of thing?

chrome_sync_shell_apk compiles in patch set 2, but chrome_sync_shell_test_apk
fails to compile. Looks like the problem is caused by
test_support_sync_proto_java, which is included only in the test APK, but which
depends on various methods in com.google.protobuf.nano that have been stripped
out from ChromeSyncShell.apk by proguard.

I see two options:
 1. Make chrome_sync_shell_apk depend on test_support_sync_proto_java (as in
patch set #1). This includes test-only code in the APK, which is not ideal, but
since ChromeSyncShell.apk exists for the sake of testing, it's perhaps
acceptable (and it's inline with including the fake sync server native code in
the APK).
 2. Add a proguard.flags file for sync shell, and add some rules in the proguard
file to prevent removing any methods in com.google.protobuf.nano. See
chrome/android/java/proguard.flags for some examples of how to do this.

I think 1 is probably OK, and easier, so I might just go with that. But if you
do, please add a comment to the build file explaining why you're including test
files in a non-test APK.

[pval...(no longer on Chromium), 2015-08-14 21:50:51]

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

[pval...(no longer on Chromium), 2015-08-14 21:51:11]

Please take another look.

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.gyp
File chrome/android/chrome_apk.gyp (right):

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.g...
chrome/android/chrome_apk.gyp:253: '../../base/base.gyp:base_java_test_support',
On 2015/08/12 23:25:58, pvalenzuela wrote:
> On 2015/08/11 21:25:29, newt wrote:
> > why are are all test-related deps required in ChromeSyncShell.apk? Shouldn't
> > they just be required in ChromeSyncShellTest.apk?
> 
> I removed the test deps (see latest patchset), and now get this barrage
Proguard
> errors:
> 
> https://paste.googleplex.com/6085784813699072?raw
> 
> All of these look proto related. Obviously I can't add @VisibleForTesting in
all
> those spots. I tried disabling Proguard for this target and it didn't work. Is
> there some known fix for this type of thing?

I went with approach #1 from your message (add the dep to the main APK with a
comment).

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.g...
chrome/android/chrome_apk.gyp:369:
'../../third_party/android_tools/android_tools.gyp:android_support_v7_appcompat_javalib',
On 2015/08/11 21:25:29, newt wrote:
> remove this. shouldn't be needed.

Done.

https://codereview.chromium.org/1282603005/diff/1/chrome/android/chrome_apk.g...
chrome/android/chrome_apk.gyp:370:
'../../ui/android/ui_android.gyp:ui_javatests',
On 2015/08/11 21:25:29, newt wrote:
> Remove this dep. Including "ui_javatests" will cause all the ui javatests to
be
> included in ChromeShellSyncTest, which we don't need and will waste lots of
bot
> time.

Done.

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
File chrome/android/sync_shell/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
chrome/android/sync_shell/javatests/AndroidManifest.xml:22: <provider
android:name="org.chromium.chrome.test.TestContentProvider"
On 2015/08/11 21:25:29, newt wrote:
> Remove all the providers, activities, receivers, etc that aren't used by the
> tests in sync_shell. Having the same providers here as in ChromePublicTest,
will
> prevent ChromeShellTest.apk and ChromePublicTest.apk from being installed
> side-by-side (which will break the bots).
> 
> Having duplicated activities and receivers won't cause problems, but it's
unused
> and unnecessary duplicated code.

Done. I think I had just copied over the ChromePublicTest manifest file (to get
things working initially) and never cleaned it back up.

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTestBase.java
(left):

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTestBase.java:45:
assertTrue("Clearing app data failed.", clearAppData());
On 2015/08/11 21:25:29, newt wrote:
> Why remove clearAppData()? Clearing app data helps guarantee independence of
> tests.

It's part of ChromeActivityTestCaseBase's setUp now:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/androi...

(see the call to ApplicationTestUtils.setUp)

https://codereview.chromium.org/1282603005/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_sync_shell_main_delegate_staging_android.h
(right):

https://codereview.chromium.org/1282603005/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_sync_shell_main_delegate_staging_android.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/08/11 21:25:29, newt wrote:
> This could simply be called "chrome_main_delegate_sync_shell.h" or
> "chrome_sync_shell_main_delegate.h".
> 
> In any case, remove all references to "staging". That's a now meaningless word
> that's leftover from upstreaming and should be removed. (Merging
> ChromeMainDelegateStagingAndroid into ChromeMainDelegateAndroid in on my TODO
> list after you land this.) I think "android" can also be removed from the
name,
> given that "sync shell" doesn't exist on other platforms, and that this file
> name doesn't clash with an existing file on other platforms.

Done (went with chrome_sync_shell_main_delegate.*)

[commit-bot: I haz the power, 2015-08-14 21:51:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1282603005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1282603005/40001

[commit-bot: I haz the power, 2015-08-14 21:53:31]

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

[commit-bot: I haz the power, 2015-08-14 21:53:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  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_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[newt (away), 2015-08-14 22:09:23]

lgtm. Thank you!

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTestBase.java
(left):

https://codereview.chromium.org/1282603005/diff/1/chrome/android/sync_shell/j...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTestBase.java:45:
assertTrue("Clearing app data failed.", clearAppData());
On 2015/08/14 21:51:10, pvalenzuela wrote:
> On 2015/08/11 21:25:29, newt wrote:
> > Why remove clearAppData()? Clearing app data helps guarantee independence of
> > tests.
> 
> It's part of ChromeActivityTestCaseBase's setUp now:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/androi...
> 
> (see the call to ApplicationTestUtils.setUp)

Ah, of course :)

[pval...(no longer on Chromium), 2015-08-17 18:29:35]

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

[commit-bot: I haz the power, 2015-08-17 18:29:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1282603005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1282603005/60001

[commit-bot: I haz the power, 2015-08-17 19:55:11]

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

[commit-bot: I haz the power, 2015-08-17 19:55:11]

Dry run: 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_...)

[pval...(no longer on Chromium), 2015-08-18 23:33:07]

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

[pval...(no longer on Chromium), 2015-08-18 23:33:07]

newt@, PTAL? I've fixed two problematic tests (see patch sets 5 and 6).

jbudorick@, adding you for OWNERS in build/. The target names here are the same,
but they have moved.

[jbudorick, 2015-08-19 00:06:21]

build/ lgtm

[newt (away), 2015-08-19 00:37:03]

lgtm

[pval...(no longer on Chromium), 2015-08-19 00:51:01]

The CQ bit was checked by pvalenzuela@chromium.org

[commit-bot: I haz the power, 2015-08-19 00:51:12]

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

[commit-bot: I haz the power, 2015-08-19 02:09:06]

Committed patchset #7 (id:120001)

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

Patchset 7 (id:??) landed as
https://crrev.com/62ca3f0326203cca221bba6f0ee038ff292ecc66
Cr-Commit-Position: refs/heads/master@{#344128}

[tkent, 2015-08-19 03:17:30]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1285373007/ by tkent@chromium.org.

The reason for reverting is: broke ChromeSyncShell build.
http://build.chromium.org/p/chromium.linux/builders/Android%20Arm64%20Builder...
.

[newt (away), 2015-08-25 18:03:57]

lgtm