Add TestRules for content shell test and refactor test bases

Add TestRules for content shell test and refactor test bases

Similar to crrev/2632043002/, this CL create TestRule for each existing test base
class (ContentDetectionTestBase, JavaBridgeTestBase, NativeLibraryTestBase).
It refactors these test base classes and new TestRules on top of test common layer.
It also remove any inheritance field reference for mTestCallbackHelperContainer to
calling the new API getTestCallbackHelperContainer() for the purpose of common
implementation.

BUG=640116
Review-Url: https://codereview.chromium.org/2739603003
Cr-Commit-Position: refs/heads/master@{#455755}
Committed: https://chromium.googlesource.com/chromium/src/+/a4394cde973e9e7e564185b560a229ba8f59889d

[the real yoland, 2017-03-08 01:44:57]

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

[commit-bot: I haz the power, 2017-03-08 01:45:35]

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

[the real yoland, 2017-03-08 02:05:02]

The CQ bit was unchecked by yolandyan@chromium.org

[the real yoland, 2017-03-08 02:07:55]

Patchset #1 (id:1) has been deleted

[the real yoland, 2017-03-08 02:10:29]

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

[commit-bot: I haz the power, 2017-03-08 02:11:52]

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

[the real yoland, 2017-03-08 02:16:56]

Description was changed from

==========
Add TestRules for content shell test and refactor test bases

BUG=640116
==========

to

==========
Add TestRules for content shell test and refactor test bases

Similar to crrev/2632043002/, this CL create TestRule for each existing test
base class (ContentDetectionTestBase, JavaBridgeTestBase,
NativeLibraryTestBase). It refactor these test bases classes and new TestRules
on top of test common classes' implementations. It also remove any inheritance
field reference for mTestCallbackHelperContainer to calling the new API
getTestCallbackHelperContainer() for the purpose of common implementation.

BUG=640116
==========

[the real yoland, 2017-03-08 02:16:56]

yolandyan@chromium.org changed reviewers:
+ boliu@chromium.org, jbudorick@chromium.org

[commit-bot: I haz the power, 2017-03-08 03:51:31]

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

[commit-bot: I haz the power, 2017-03-08 03:51:32]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[the real yoland, 2017-03-08 18:22:17]

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

[commit-bot: I haz the power, 2017-03-08 18:22:59]

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

[the real yoland, 2017-03-08 18:23:14]

Description was changed from

==========
Add TestRules for content shell test and refactor test bases

Similar to crrev/2632043002/, this CL create TestRule for each existing test
base class (ContentDetectionTestBase, JavaBridgeTestBase,
NativeLibraryTestBase). It refactor these test bases classes and new TestRules
on top of test common classes' implementations. It also remove any inheritance
field reference for mTestCallbackHelperContainer to calling the new API
getTestCallbackHelperContainer() for the purpose of common implementation.

BUG=640116
==========

to

==========
Add TestRules for content shell test and refactor test bases

Similar to crrev/2632043002/, this CL create TestRule for each existing test
base
class (ContentDetectionTestBase, JavaBridgeTestBase, NativeLibraryTestBase).
It refactors these test base classes and new TestRules on top of test common
layer.
It also remove any inheritance field reference for mTestCallbackHelperContainer
to
calling the new API getTestCallbackHelperContainer() for the purpose of common
implementation.

BUG=640116
==========

[the real yoland, 2017-03-08 18:34:13]

Patchset #2 (id:40001) has been deleted

[the real yoland, 2017-03-08 18:34:17]

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

[commit-bot: I haz the power, 2017-03-08 18:35:01]

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

[the real yoland, 2017-03-08 18:58:46]

The CQ bit was unchecked by yolandyan@chromium.org

[the real yoland, 2017-03-08 18:58:51]

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

[commit-bot: I haz the power, 2017-03-08 18:59:49]

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-03-08 19:36:10]

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

[commit-bot: I haz the power, 2017-03-08 19:36:11]

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

[boliu, 2017-03-08 21:40:18]

https://codereview.chromium.org/2739603003/diff/60001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionActivityTestRule.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionActivityTestRule.java:35:
protected void afterActivityLaunched() {
super?

https://codereview.chromium.org/2739603003/diff/60001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:20:
public class JavaBridgeActivityTestRule
drop the "Activity" part? there's no such thing as JavaBridgeActivity

https://codereview.chromium.org/2739603003/diff/60001/content/public/test/and...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/public/test/and...
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java:16:
public class NativeLibraryTestRule implements TestRule {
why does this not inherit from android base class? whatever the explanation,
probably worth a comment?

https://codereview.chromium.org/2739603003/diff/60001/content/public/test/and...
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java:35:
public Statement apply(final Statement base, Description description) {
just return base?

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
File
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:194:
ContentViewCore getContentViewCoreForTestCommon();
this assumes test has only one CVC? that seems too specific for all test cases
and can probably be structured differently

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:196:
WebContents getWebContentsForTestCommon();
ditto

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:197:
void waitForActiveShellToBeDoneLoadingForTestCommon();
ditto

[the real yoland, 2017-03-08 22:53:53]

https://codereview.chromium.org/2739603003/diff/60001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionActivityTestRule.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionActivityTestRule.java:35:
protected void afterActivityLaunched() {
On 2017/03/08 at 21:40:18, boliu wrote:
> super?

Done

https://codereview.chromium.org/2739603003/diff/60001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:20:
public class JavaBridgeActivityTestRule
On 2017/03/08 at 21:40:18, boliu wrote:
> drop the "Activity" part? there's no such thing as JavaBridgeActivity

The naming because it extends ActivityTestRule

https://codereview.chromium.org/2739603003/diff/60001/content/public/test/and...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/public/test/and...
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java:16:
public class NativeLibraryTestRule implements TestRule {
On 2017/03/08 at 21:40:18, boliu wrote:
> why does this not inherit from android base class? whatever the explanation,
probably worth a comment?

Done

https://codereview.chromium.org/2739603003/diff/60001/content/public/test/and...
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java:35:
public Statement apply(final Statement base, Description description) {
On 2017/03/08 at 21:40:18, boliu wrote:
> just return base?

Oops, done

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
File
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:194:
ContentViewCore getContentViewCoreForTestCommon();
On 2017/03/08 at 21:40:18, boliu wrote:
> this assumes test has only one CVC? that seems too specific for all test cases
and can probably be structured differently

One alternative is to create another interface (e.g. TestCommonCallback2),
declare these methods, and implement them in TestRule/TestBase that needs them.
but that will end up having both ContentDetectionTestBase/TestRule and
JavaBridgeTestBase/TestRule implement both TestCommonCallback and
TestCommonCallback2. And the worst part is in TestCommon class, it will have two
callback object that are essentially the same object calling 2 different set of
APIs

Or maybe I can name this more specifically or avoid the complication. Because
what this is doing is just exposing an API that is previously inherited to a
now-composed object.

[boliu, 2017-03-08 23:03:05]

lgtm

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
File
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:194:
ContentViewCore getContentViewCoreForTestCommon();
On 2017/03/08 22:53:53, the real yoland wrote:
> On 2017/03/08 at 21:40:18, boliu wrote:
> > this assumes test has only one CVC? that seems too specific for all test
cases
> and can probably be structured differently
> 
> One alternative is to create another interface (e.g. TestCommonCallback2),
> declare these methods, and implement them in TestRule/TestBase that needs
them.
> but that will end up having both ContentDetectionTestBase/TestRule and
> JavaBridgeTestBase/TestRule implement both TestCommonCallback and
> TestCommonCallback2. And the worst part is in TestCommon class, it will have
two
> callback object that are essentially the same object calling 2 different set
of
> APIs
> 
> Or maybe I can name this more specifically or avoid the complication. Because
> what this is doing is just exposing an API that is previously inherited to a
> now-composed object.

Actually, content tests don't generally use multiple CVCs, so this might be ok.

But if this was a long term thing, and if we really wanted to fix it, it would
be passing CVC directly into the calls that need it.

[the real yoland, 2017-03-08 23:07:10]

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

[commit-bot: I haz the power, 2017-03-08 23:07:39]

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

[the real yoland, 2017-03-08 23:52:45]

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
File
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java
(right):

https://codereview.chromium.org/2739603003/diff/60001/content/shell/android/j...
content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:194:
ContentViewCore getContentViewCoreForTestCommon();
On 2017/03/08 at 23:03:05, boliu wrote:
> On 2017/03/08 22:53:53, the real yoland wrote:
> > On 2017/03/08 at 21:40:18, boliu wrote:
> > > this assumes test has only one CVC? that seems too specific for all test
cases
> > and can probably be structured differently
> > 
> > One alternative is to create another interface (e.g. TestCommonCallback2),
> > declare these methods, and implement them in TestRule/TestBase that needs
them.
> > but that will end up having both ContentDetectionTestBase/TestRule and
> > JavaBridgeTestBase/TestRule implement both TestCommonCallback and
> > TestCommonCallback2. And the worst part is in TestCommon class, it will have
two
> > callback object that are essentially the same object calling 2 different set
of
> > APIs
> > 
> > Or maybe I can name this more specifically or avoid the complication.
Because
> > what this is doing is just exposing an API that is previously inherited to a
> > now-composed object.
> 
> Actually, content tests don't generally use multiple CVCs, so this might be
ok.
> 
> But if this was a long term thing, and if we really wanted to fix it, it would
be passing CVC directly into the calls that need it.

ack

[commit-bot: I haz the power, 2017-03-09 00:04:54]

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

[commit-bot: I haz the power, 2017-03-09 00:04:55]

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

[the real yoland, 2017-03-09 15:50:23]

The CQ bit was checked by yolandyan@chromium.org

[commit-bot: I haz the power, 2017-03-09 15:50:45]

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-03-09 16:08:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489074623612480,
"parent_rev": "1aa57ebd97f6f68fdbf29d44588af7b32a13c249", "commit_rev":
"a4394cde973e9e7e564185b560a229ba8f59889d"}

[commit-bot: I haz the power, 2017-03-09 16:08:59]

Description was changed from

==========
Add TestRules for content shell test and refactor test bases

Similar to crrev/2632043002/, this CL create TestRule for each existing test
base
class (ContentDetectionTestBase, JavaBridgeTestBase, NativeLibraryTestBase).
It refactors these test base classes and new TestRules on top of test common
layer.
It also remove any inheritance field reference for mTestCallbackHelperContainer
to
calling the new API getTestCallbackHelperContainer() for the purpose of common
implementation.

BUG=640116
==========

to

==========
Add TestRules for content shell test and refactor test bases

Similar to crrev/2632043002/, this CL create TestRule for each existing test
base
class (ContentDetectionTestBase, JavaBridgeTestBase, NativeLibraryTestBase).
It refactors these test base classes and new TestRules on top of test common
layer.
It also remove any inheritance field reference for mTestCallbackHelperContainer
to
calling the new API getTestCallbackHelperContainer() for the purpose of common
implementation.

BUG=640116

Review-Url: https://codereview.chromium.org/2739603003
Cr-Commit-Position: refs/heads/master@{#455755}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4394cde973e9e7e564185b560a2...
==========

[commit-bot: I haz the power, 2017-03-09 16:09:01]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a4394cde973e9e7e564185b560a2...