[Offline Pages] Added OfflinePageEvaluationBridge for testing.

[Offline Pages] Added OfflinePageEvaluationBridge for testing.

Build another bridge for evaluation tests use only. This is part of a larger
change tracked in the bug. The bridge exposes some testing only interface and
implements another observer.
The bridge would not be compiled for official build, and currently there seems
no way to have testing-only JNI code without marking with build flags.

BUG=646947
Committed: https://crrev.com/3f2464af861c43de538fd23801b12931b93c004a
Cr-Commit-Position: refs/heads/master@{#425419}

[romax, 2016-09-29 02:21:13]

romax@chromium.org changed reviewers:
+ petewil@chromium.org

[romax, 2016-09-29 02:21:13]

Pete PTAL.
I also have some questions regarding build files, can we please have a chat?

THanks!

[Pete Williamson, 2016-10-04 23:50:55]

Sorry this took so long, here are my comments.  Let me know when you want to
talk about build files.

https://codereview.chromium.org/2378803003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
(right):

https://codereview.chromium.org/2378803003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:32:
private final ObserverList<OfflinePageEvaluationObserver> mObservers =
Just curious, why is this final?  It seems that observers are something that
normally come and go.

https://codereview.chromium.org/2378803003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
(right):

https://codereview.chromium.org/2378803003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:3:
// found in the LICENSE file.
Overall, the bridge looks good.  You didn't try to do too much in it.  I presume
you copied from existing bridges for things like format, order of functions in
the file, function naming, etc.

Where is the java side client for this bridge? I don't see it in this patch. 
Different patch?

https://codereview.chromium.org/2378803003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:32:
private final ObserverList<OfflinePageEvaluationObserver> mObservers =
It's a little odd that this is final.  Is that intended?  I suppose that you can
think of the list as final, and you are adding items to a list that can't be
reassigned, but it doesn't seem to convey the intent well.

https://codereview.chromium.org/2378803003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:126:
* @return True is the offline page model has fully loaded.
is -> if

https://codereview.chromium.org/2378803003/diff/20001/chrome/browser/android/...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2378803003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:186:
if (!request_coordinator_) {
How could we ever end up with no request coordinator?  We get one from the
profile when we create the bridge, right?  If there is none, we should fail at
creation time somehow.  We don't check this in other places.  Maybe have a
DCHECK instead of a runtime check.

https://codereview.chromium.org/2378803003/diff/20001/chrome/browser/android/...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h
(right):

https://codereview.chromium.org/2378803003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h:3:
// found in the LICENSE file.
Is it possible for this to live in a test directory somewhere so it doesn't get
compiled in with retail chrome?  Maybe it already compiles to a different
target, and I just didn't notice.

[agrieve, 2016-10-11 14:02:45]

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

[agrieve, 2016-10-11 14:02:45]

lgtm - but I think it'd be nice to add a comment to all of these spots says
"required to not ship test-only JNI to end users". or something along those
lines.

[romax, 2016-10-11 21:40:30]

Description was changed from

==========
[Offline Pages] Added OfflinePageEvaluationBridge for testing.

Build another bridge for evaluation tests use only. This is part of the
larger changed tracked in the bug. It expose more native side methods.

BUG=646947
==========

to

==========
[Offline Pages] Added OfflinePageEvaluationBridge for testing.

Build another bridge for evaluation tests use only. This is part of a larger
change tracked in the bug. The bridge exposes some testing only interface and
implements another observer.
The bridge would not be compiled for official build, and currently there seems
no way to have testing-only JNI code without marking with build flags.


BUG=646947
==========

[romax, 2016-10-11 21:46:29]

romax@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[romax, 2016-10-11 21:46:30]

petewil@ PTAL offline_pages related changes
dfalcantara@ PTAL chrome_jni_registrar.cc

please suggest if there's better way to make it test-only.
Thanks!

https://codereview.chromium.org/2378803003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
(right):

https://codereview.chromium.org/2378803003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:32:
private final ObserverList<OfflinePageEvaluationObserver> mObservers =
On 2016/10/04 23:50:55, Pete Williamson wrote:
> Just curious, why is this final?  It seems that observers are something that
> normally come and go.

Acknowledged.

https://codereview.chromium.org/2378803003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
(right):

https://codereview.chromium.org/2378803003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:3:
// found in the LICENSE file.
On 2016/10/04 23:50:55, Pete Williamson wrote:
> Overall, the bridge looks good.  You didn't try to do too much in it.  I
presume
> you copied from existing bridges for things like format, order of functions in
> the file, function naming, etc.
> 
> Where is the java side client for this bridge? I don't see it in this patch. 
> Different patch?

Yes I copied the methods which should be used in this bridge. And the client
would be in another patch.

https://codereview.chromium.org/2378803003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:32:
private final ObserverList<OfflinePageEvaluationObserver> mObservers =
On 2016/10/04 23:50:55, Pete Williamson wrote:
> It's a little odd that this is final.  Is that intended?  I suppose that you
can
> think of the list as final, and you are adding items to a list that can't be
> reassigned, but it doesn't seem to convey the intent well.

Done.

https://codereview.chromium.org/2378803003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:126:
* @return True is the offline page model has fully loaded.
On 2016/10/04 23:50:55, Pete Williamson wrote:
> is -> if

Done.

https://codereview.chromium.org/2378803003/diff/20001/chrome/browser/android/...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2378803003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:186:
if (!request_coordinator_) {
On 2016/10/04 23:50:55, Pete Williamson wrote:
> How could we ever end up with no request coordinator?  We get one from the
> profile when we create the bridge, right?  If there is none, we should fail at
> creation time somehow.  We don't check this in other places.  Maybe have a
> DCHECK instead of a runtime check.

Done.

https://codereview.chromium.org/2378803003/diff/20001/chrome/browser/android/...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h
(right):

https://codereview.chromium.org/2378803003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h:3:
// found in the LICENSE file.
On 2016/10/04 23:50:55, Pete Williamson wrote:
> Is it possible for this to live in a test directory somewhere so it doesn't
get
> compiled in with retail chrome?  Maybe it already compiles to a different
> target, and I just didn't notice.

I changed the build file so that it would not be compiled for retail chrome.

[Pete Williamson, 2016-10-11 22:25:18]

Changes look good.  One question.

Also, Doug thought of another approach - what if you had a Java app that used
the Chrome Custom Tabs API to ask for a page to be offlined, and got the
resulting page?  You could then have the custom app show pages it had previously
offlined so we can check for quality (or the pages could be copied to the
desktop and looked at there).

https://codereview.chromium.org/2378803003/diff/60001/chrome/android/java_sou...
File chrome/android/java_sources.gni (right):

https://codereview.chromium.org/2378803003/diff/60001/chrome/android/java_sou...
chrome/android/java_sources.gni:1086: if (!is_official_build) {
Does this mean that the code ends up in Chromium but not Chrome?  Ideally, the
code would not even end up in Chromium, but only in our tests.

[agrieve, 2016-10-12 01:50:50]

https://codereview.chromium.org/2378803003/diff/60001/chrome/android/java_sou...
File chrome/android/java_sources.gni (right):

https://codereview.chromium.org/2378803003/diff/60001/chrome/android/java_sou...
chrome/android/java_sources.gni:1086: if (!is_official_build) {
On 2016/10/11 22:25:18, Pete Williamson wrote:
> Does this mean that the code ends up in Chromium but not Chrome?  Ideally, the
> code would not even end up in Chromium, but only in our tests.

Chromium builds also set is_official_build. (it's different from
is_chrome_branded)

[Pete Williamson, 2016-10-12 17:47:05]

lgtm

Since this doesn't build in chromium, it should be fine to check in.

[gone, 2016-10-13 17:56:59]

lgtm % nits

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
(right):

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:54:
public abstract static class OfflinePageEvaluationObserver {
Declare inner classes at the top of the class.

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:98:
* list of pages.
nit: indent list under The

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/browser/a...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/browser/a...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:110:
ScopedJavaLocalRef<jobject> j_offline_page_evaluation_bridge =
You should just be able to feed this to .Reset() below without creating another
ScopedJavaLocalRef<jobject>.

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/browser/a...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:171:
j_result_ref.Reset(env, j_result_obj);
One of the constructors for ScopedJavaGlobalRef does this for you; you use it
below in PushRequestProcessing.

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/browser/a...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:182:
const base::android::JavaParamRef<jobject>& obj,
You're already "using" this; just remove base::android::

[romax, 2016-10-13 18:46:23]

Addressed comments, will run on try bots before commit.

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
(right):

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:54:
public abstract static class OfflinePageEvaluationObserver {
On 2016/10/13 17:56:58, dfalcantara wrote:
> Declare inner classes at the top of the class.

Done.

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:98:
* list of pages.
On 2016/10/13 17:56:58, dfalcantara wrote:
> nit: indent list under The

Done.

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/browser/a...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/browser/a...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:110:
ScopedJavaLocalRef<jobject> j_offline_page_evaluation_bridge =
On 2016/10/13 17:56:58, dfalcantara wrote:
> You should just be able to feed this to .Reset() below without creating
another
> ScopedJavaLocalRef<jobject>.

Done.

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/browser/a...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:171:
j_result_ref.Reset(env, j_result_obj);
On 2016/10/13 17:56:58, dfalcantara wrote:
> One of the constructors for ScopedJavaGlobalRef does this for you; you use it
> below in PushRequestProcessing.

Done.

https://chromiumcodereview.appspot.com/2378803003/diff/60001/chrome/browser/a...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:182:
const base::android::JavaParamRef<jobject>& obj,
On 2016/10/13 17:56:59, dfalcantara wrote:
> You're already "using" this; just remove base::android::

Done.

[romax, 2016-10-13 18:46:33]

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

[commit-bot: I haz the power, 2016-10-13 18:46:55]

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-10-13 18:51:24]

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

[commit-bot: I haz the power, 2016-10-13 18:51:26]

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...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[romax, 2016-10-13 19:16:13]

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

[commit-bot: I haz the power, 2016-10-13 19:16: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-10-13 19:40:14]

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

[commit-bot: I haz the power, 2016-10-13 19:40:15]

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

[romax, 2016-10-13 20:19:33]

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

[commit-bot: I haz the power, 2016-10-13 20:20:00]

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-10-13 22:27:33]

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

[commit-bot: I haz the power, 2016-10-13 22:27:35]

Dry run: This issue passed the CQ dry run.

[romax, 2016-10-13 22:34:20]

petewil@ can you please take another look? I've changed some files in Patch 6
and 7 because the try bot failed to compile.
Thanks!

[Pete Williamson, 2016-10-14 19:00:07]

lgtm

[romax, 2016-10-14 19:03:13]

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

[commit-bot: I haz the power, 2016-10-14 19:03:42]

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-10-14 19:09:31]

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

[commit-bot: I haz the power, 2016-10-14 19:09:32]

Dry run: This issue passed the CQ dry run.

[romax, 2016-10-14 19:10:00]

The CQ bit was checked by romax@chromium.org

[romax, 2016-10-14 19:10:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2378803003/#ps120001
(title: "Fixing build.")

[commit-bot: I haz the power, 2016-10-14 19:10:28]

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-10-14 19:17:32]

Description was changed from

==========
[Offline Pages] Added OfflinePageEvaluationBridge for testing.

Build another bridge for evaluation tests use only. This is part of a larger
change tracked in the bug. The bridge exposes some testing only interface and
implements another observer.
The bridge would not be compiled for official build, and currently there seems
no way to have testing-only JNI code without marking with build flags.


BUG=646947
==========

to

==========
[Offline Pages] Added OfflinePageEvaluationBridge for testing.

Build another bridge for evaluation tests use only. This is part of a larger
change tracked in the bug. The bridge exposes some testing only interface and
implements another observer.
The bridge would not be compiled for official build, and currently there seems
no way to have testing-only JNI code without marking with build flags.


BUG=646947
==========

[commit-bot: I haz the power, 2016-10-14 19:17:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-14 19:19:16]

Description was changed from

==========
[Offline Pages] Added OfflinePageEvaluationBridge for testing.

Build another bridge for evaluation tests use only. This is part of a larger
change tracked in the bug. The bridge exposes some testing only interface and
implements another observer.
The bridge would not be compiled for official build, and currently there seems
no way to have testing-only JNI code without marking with build flags.


BUG=646947
==========

to

==========
[Offline Pages] Added OfflinePageEvaluationBridge for testing.

Build another bridge for evaluation tests use only. This is part of a larger
change tracked in the bug. The bridge exposes some testing only interface and
implements another observer.
The bridge would not be compiled for official build, and currently there seems
no way to have testing-only JNI code without marking with build flags.

BUG=646947

Committed: https://crrev.com/3f2464af861c43de538fd23801b12931b93c004a
Cr-Commit-Position: refs/heads/master@{#425419}
==========

[commit-bot: I haz the power, 2016-10-14 19:19:17]

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