Adds initial JNI bridge code for Background Scheduler implementation.

Adds initial JNI bridge code for Background Scheduler implementation.

The idea is that the Java side only has statics (no instance) but ended up with instance on C++ side (can be created by Coordinator) to be able to provide implementation instance of Scheduler interface for C++ to Java calls.

BUG=607346
Committed: https://crrev.com/9ca9a2703e0a762694f7940e9fea44ee42352916
Cr-Commit-Position: refs/heads/master@{#390703}

[dougarnett, 2016-04-27 23:10:31]

Description was changed from

==========
Adds initial JNI bridge code for Background Scheduler implementation.

BUG=607346
==========

to

==========
Adds initial JNI bridge code for Background Scheduler implementation.

The idea is that the Java side only has statics (no instance) but ended up with
instance on C++ side (can be created by Coordinator) to be able to provide
implementation instance of Scheduler interface for C++ to Java calls.


BUG=607346
==========

[dougarnett, 2016-04-27 23:10:31]

dougarnett@chromium.org changed reviewers:
+ dewittj@chromium.org, fgorski@chromium.org, petewil@chromium.org

[Pete Williamson, 2016-04-28 00:12:04]

Mostly looks good, but we need a BUILD.gn file change too.

https://codereview.chromium.org/1928813002/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1928813002/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1546:
'browser/android/offline_pages/background_scheduler_bridge.h',
Anytime we add files, we need to add them to both the .gypi and the BUILD.gn
files (buildbots will warn you of this when the gn build totally fails)

[fgorski, 2016-04-28 11:44:00]

https://codereview.chromium.org/1928813002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java
(right):

https://codereview.chromium.org/1928813002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:24:
void onResult();
Do we not want to pass anything for result? Even if that would mean passing a
boolean. (e.g. no more tasks left vs. still some work to do)

Also, as Justin pointed out to me yesterday, please check out
org.chromium.base.Callback

https://codereview.chromium.org/1928813002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:27:
public static boolean startProcessing(
documentation.

https://codereview.chromium.org/1928813002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:33:
private static void ensureScheduled() {
schedule/unschedule.

After all the semantics of being scheduled multiple times versus only once can
be hidden from all other classes. WDYT?

https://codereview.chromium.org/1928813002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_scheduler_bridge.cc
(right):

https://codereview.chromium.org/1928813002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:37: // and
call StartProcessing on it with Bind-ed j_callback_obj.
nit: bound?

[dewittj, 2016-04-28 16:08:02]

I'm worried about only static functions because it results in code that's harder
to test (no way to do dependency injection for example).  Can we do a static
instance with a getter that can create a new one?

[dewittj, 2016-04-28 17:30:18]

On 2016/04/28 16:08:02, dewittj wrote:
> I'm worried about only static functions because it results in code that's
harder
> to test (no way to do dependency injection for example).  Can we do a static
> instance with a getter that can create a new one?

We talked about this, and decided that it's probably best to wait on particular
implementation details until after this patch.

[dougarnett, 2016-04-28 22:45:23]

https://codereview.chromium.org/1928813002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java
(right):

https://codereview.chromium.org/1928813002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:24:
void onResult();
On 2016/04/28 11:44:00, fgorski wrote:
> Do we not want to pass anything for result? Even if that would mean passing a
> boolean. (e.g. no more tasks left vs. still some work to do)
> 
> Also, as Justin pointed out to me yesterday, please check out
> org.chromium.base.Callback

I think we will want to return boolean - possible use is return code to
GcmTaskService, UMA, and testing. Talked with Justin, might not be able
to use that Callback for CalledByNative

https://codereview.chromium.org/1928813002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:27:
public static boolean startProcessing(
On 2016/04/28 11:44:00, fgorski wrote:
> documentation.

Done.

https://codereview.chromium.org/1928813002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:33:
private static void ensureScheduled() {
On 2016/04/28 11:44:00, fgorski wrote:
> schedule/unschedule.
> 
> After all the semantics of being scheduled multiple times versus only once can
> be hidden from all other classes. WDYT?

As discussed, do like that better along with adding comments about their
separate idempotency.

https://codereview.chromium.org/1928813002/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1928813002/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1546:
'browser/android/offline_pages/background_scheduler_bridge.h',
On 2016/04/28 00:12:04, Pete Williamson wrote:
> Anytime we add files, we need to add them to both the .gypi and the BUILD.gn
> files (buildbots will warn you of this when the gn build totally fails)

Discussed in person wrt browser BUILD.gn actually consumes this sources list.

[Pete Williamson, 2016-04-28 22:51:26]

lgtm

[fgorski, 2016-04-29 04:50:41]

lgtm with comments. (mostly improve documentation)

https://codereview.chromium.org/1928813002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java
(right):

https://codereview.chromium.org/1928813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:29:
// expect a callback (once processing has completed or terminated).i
there might be a typo at the end.

https://codereview.chromium.org/1928813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:30:
// If processing was already active, returns false and this calling
would that be the only reason to not start the processing?

https://codereview.chromium.org/1928813002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_scheduler_bridge.h (right):

https://codereview.chromium.org/1928813002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.h:18: /**
// This is how documentation in c++ looks :)
// And this is line 2 :)

[dougarnett, 2016-04-29 15:59:08]

https://codereview.chromium.org/1928813002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java
(right):

https://codereview.chromium.org/1928813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:29:
// expect a callback (once processing has completed or terminated).i
On 2016/04/29 04:50:41, fgorski wrote:
> there might be a typo at the end.

Done.

https://codereview.chromium.org/1928813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:30:
// If processing was already active, returns false and this calling
On 2016/04/29 04:50:41, fgorski wrote:
> would that be the only reason to not start the processing?

Opened up to any undefined reason not accepted and added TODO about
adding the policy check api call.

https://codereview.chromium.org/1928813002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_scheduler_bridge.h (right):

https://codereview.chromium.org/1928813002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.h:18: /**
On 2016/04/29 04:50:41, fgorski wrote:
> // This is how documentation in c++ looks :)
> // And this is line 2 :)

oops, i may need a lint rule for this!

[dougarnett, 2016-04-29 15:59:52]

dougarnett@chromium.org changed reviewers:
+ tedchoc@chromium.org

[dougarnett, 2016-04-29 16:01:55]

Ted, are you able to do OWNERS review on
chrome/browser/android/chrome_jni_registrar.cc?

[dewittj, 2016-04-29 16:14:53]

dewittj@chromium.org changed reviewers:
- dewittj@chromium.org

[Ted C, 2016-04-29 17:44:53]

On 2016/04/29 16:01:55, dougarnett wrote:
> Ted, are you able to do OWNERS review on
> chrome/browser/android/chrome_jni_registrar.cc?

chrome_jni_registrar.cc - lgtm

[dougarnett, 2016-04-29 18:20:12]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-04-29 18:20:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1928813002/#ps60001
(title: "C++ style guide change")

[commit-bot: I haz the power, 2016-04-29 18:20:27]

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

[commit-bot: I haz the power, 2016-04-29 18:26:08]

Description was changed from

==========
Adds initial JNI bridge code for Background Scheduler implementation.

The idea is that the Java side only has statics (no instance) but ended up with
instance on C++ side (can be created by Coordinator) to be able to provide
implementation instance of Scheduler interface for C++ to Java calls.


BUG=607346
==========

to

==========
Adds initial JNI bridge code for Background Scheduler implementation.

The idea is that the Java side only has statics (no instance) but ended up with
instance on C++ side (can be created by Coordinator) to be able to provide
implementation instance of Scheduler interface for C++ to Java calls.


BUG=607346
==========

[commit-bot: I haz the power, 2016-04-29 18:26:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-30 17:27:30]

Patchset 4 (id:??) landed as
https://crrev.com/9ca9a2703e0a762694f7940e9fea44ee42352916
Cr-Commit-Position: refs/heads/master@{#390703}

[commit-bot: I haz the power, 2016-04-30 18:01:32]

Description was changed from

==========
Adds initial JNI bridge code for Background Scheduler implementation.

The idea is that the Java side only has statics (no instance) but ended up with
instance on C++ side (can be created by Coordinator) to be able to provide
implementation instance of Scheduler interface for C++ to Java calls.


BUG=607346
==========

to

==========
Adds initial JNI bridge code for Background Scheduler implementation.

The idea is that the Java side only has statics (no instance) but ended up with
instance on C++ side (can be created by Coordinator) to be able to provide
implementation instance of Scheduler interface for C++ to Java calls.

BUG=607346

Committed: https://crrev.com/9ca9a2703e0a762694f7940e9fea44ee42352916
Cr-Commit-Position: refs/heads/master@{#390703}
==========