Initial checkin of the Java OfflineContentProvider

Initial checkin of the Java OfflineContentProvider

This patch adds the Java counterpart to the C++ OfflineContentProvider.
This patch also connects the native and Java components with a Java/C++
bridge pair and adds the factory methods necessary to create and pass
OfflineItems from C++ to Java.

BUG=691805
Review-Url: https://codereview.chromium.org/2707003003
Cr-Commit-Position: refs/heads/master@{#455808}
Committed: https://chromium.googlesource.com/chromium/src/+/0317c3b457483a2b3521d60a27e96cad31aeb416

[David Trainor- moved to gerrit, 2017-02-22 08:20:18]

dtrainor@chromium.org changed reviewers:
+ dimich@chromium.org, fgorski@chromium.org, qinmin@chromium.org

[David Trainor- moved to gerrit, 2017-02-22 08:20:19]

ptal.  Follow up patch that adds the initial Java bridge.

[fgorski, 2017-02-23 17:13:51]

nice and clean!

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/ContentId.java
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/ContentId.java:14:
public String namespace;
do you plan to modify these after construction?

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItem.java
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItem.java:16:
public class OfflineItem {
same here: no accessors?

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc:61:
OfflineContentAggregatorBridge::~OfflineContentAggregatorBridge() {
On the offline side, with download UI adapter, we had this rule that we would
only keep items in memory if there was at least one observer.
I am not sure if you were planning for the same approach, but this pattern
defeats it I think.

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File components/offline_items_collection/core/android/offline_item_bridge.cc
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/offline_item_bridge.cc:59: for
(const auto& item : items) {
nit: you don't need braces

[Dmitry Titov, 2017-02-24 18:24:27]

lgtm with comments:

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItem.java
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItem.java:16:
public class OfflineItem {
On 2017/02/23 17:13:51, fgorski wrote:
> same here: no accessors?

I think David is making those as close as possible to C++ counterparts, which
are 'struct' with just data members. 

I don't have strong opinion on whether those should be immutable after creation,
but if this needs to change, it should change on both sides to be consistent.

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItemBridge.java
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItemBridge.java:42:
private static OfflineItem createOfflineItem(ArrayList<OfflineItem> list, String
nameSpace,
Should this also be createOfflineItemAndMaybeAddToList?

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File components/offline_items_collection/core/android/offline_item_bridge.cc
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/offline_item_bridge.cc:25:
ScopedJavaLocalRef<jobject> CreateOfflineItemAndAddToList(
CreateOfflineItemAndMaybeAddToList? 
If you like it better...

[qinmin, 2017-02-27 18:04:19]

lgtm

[David Trainor- moved to gerrit, 2017-03-03 01:10:09]

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/ContentId.java
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/ContentId.java:14:
public String namespace;
On 2017/02/23 17:13:51, fgorski wrote:
> do you plan to modify these after construction?

The goal was to keep this as a mirror of the native class.  The problem with
making it immutable is I either need a dirty constructor (easy for this class,
harder for OfflineItem)/a private constructor with a Builder.  I wanted to keep
it simple for now.  I'd be happy moving all of these to final/const in a
separate patch if you feel strongly or if it ends up causing issues, but I'd
like to do it for both Java and native at the same time :).

Added a TODO to investigate.

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItem.java
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItem.java:16:
public class OfflineItem {
On 2017/02/24 18:24:27, Dmitry Titov wrote:
> On 2017/02/23 17:13:51, fgorski wrote:
> > same here: no accessors?
> 
> I think David is making those as close as possible to C++ counterparts, which
> are 'struct' with just data members. 
> 
> I don't have strong opinion on whether those should be immutable after
creation,
> but if this needs to change, it should change on both sides to be consistent.

Yep.  Replied above!

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItemBridge.java
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/offline_items_collection/OfflineItemBridge.java:42:
private static OfflineItem createOfflineItem(ArrayList<OfflineItem> list, String
nameSpace,
On 2017/02/24 18:24:27, Dmitry Titov wrote:
> Should this also be createOfflineItemAndMaybeAddToList?

Done.

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc:61:
OfflineContentAggregatorBridge::~OfflineContentAggregatorBridge() {
On 2017/02/23 17:13:51, fgorski wrote:
> On the offline side, with download UI adapter, we had this rule that we would
> only keep items in memory if there was at least one observer.
> I am not sure if you were planning for the same approach, but this pattern
> defeats it I think.

I think the issue is that the notification system will have to basically always
register as an observer to be notified of any new items that need a notification
popped up.  I do agree we'll probably need some kind of cache management here. 
Downloads currently just loads the entire SQL database into memory... forever...
which isn't ideal :(.  I agree that we'll need to at least support this before
we push this out.

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File components/offline_items_collection/core/android/offline_item_bridge.cc
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/offline_item_bridge.cc:25:
ScopedJavaLocalRef<jobject> CreateOfflineItemAndAddToList(
On 2017/02/24 18:24:27, Dmitry Titov wrote:
> CreateOfflineItemAndMaybeAddToList? 
> If you like it better...

Done.

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/offline_item_bridge.cc:59: for
(const auto& item : items) {
On 2017/02/23 17:13:51, fgorski wrote:
> nit: you don't need braces

Done.

[fgorski, 2017-03-04 00:32:39]

lgtm

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc
(right):

https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc:61:
OfflineContentAggregatorBridge::~OfflineContentAggregatorBridge() {
On 2017/03/03 01:10:08, David Trainor wrote:
> On 2017/02/23 17:13:51, fgorski wrote:
> > On the offline side, with download UI adapter, we had this rule that we
would
> > only keep items in memory if there was at least one observer.
> > I am not sure if you were planning for the same approach, but this pattern
> > defeats it I think.
> 
> I think the issue is that the notification system will have to basically
always
> register as an observer to be notified of any new items that need a
notification
> popped up.  I do agree we'll probably need some kind of cache management here.

> Downloads currently just loads the entire SQL database into memory...
forever...
> which isn't ideal :(.  I agree that we'll need to at least support this before
> we push this out.

Interesting. I think we were able to handle that with relative ease, because we
separated notifications observing from downloads UI adapter initially.
I am OK with this as long as we work out that cache management at some point.

[David Trainor- moved to gerrit, 2017-03-07 21:49:53]

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

[commit-bot: I haz the power, 2017-03-07 21:51:26]

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-07 21:59:06]

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

[commit-bot: I haz the power, 2017-03-07 21:59:07]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Dmitry Titov, 2017-03-08 00:33:49]

On 2017/03/04 00:32:39, fgorski wrote:
> lgtm
> 
>
https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
> File
>
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc
> (right):
> 
>
https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
>
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc:61:
> OfflineContentAggregatorBridge::~OfflineContentAggregatorBridge() {
> On 2017/03/03 01:10:08, David Trainor wrote:
> > On 2017/02/23 17:13:51, fgorski wrote:
> > > On the offline side, with download UI adapter, we had this rule that we
> would
> > > only keep items in memory if there was at least one observer.
> > > I am not sure if you were planning for the same approach, but this pattern
> > > defeats it I think.
> > 
> > I think the issue is that the notification system will have to basically
> always
> > register as an observer to be notified of any new items that need a
> notification
> > popped up.  I do agree we'll probably need some kind of cache management
here.
> 
> > Downloads currently just loads the entire SQL database into memory...
> forever...
> > which isn't ideal :(.  I agree that we'll need to at least support this
before
> > we push this out.
> 
> Interesting. I think we were able to handle that with relative ease, because
we
> separated notifications observing from downloads UI adapter initially.
> I am OK with this as long as we work out that cache management at some point.

Indeed, the notifications need to always listen in case a download will start
(suddenly!). 
One way to avoid the cache (and collection returned by GetAllItems()) to be
loaded all the time
is to split Observer to "backend changes observer" (everything except
OnItemsAvailable) and "collection population observer"
(with only OnItemsAvailable). The presence of observers of the latter type can
control the 
cache, while the observers of the former type would not trigger loading of the
cache.

Still lgtm.

[David Trainor- moved to gerrit, 2017-03-08 00:44:45]

On 2017/03/08 00:33:49, Dmitry Titov wrote:
> On 2017/03/04 00:32:39, fgorski wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
> > File
> >
>
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2707003003/diff/1/components/offline_items_co...
> >
>
components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc:61:
> > OfflineContentAggregatorBridge::~OfflineContentAggregatorBridge() {
> > On 2017/03/03 01:10:08, David Trainor wrote:
> > > On 2017/02/23 17:13:51, fgorski wrote:
> > > > On the offline side, with download UI adapter, we had this rule that we
> > would
> > > > only keep items in memory if there was at least one observer.
> > > > I am not sure if you were planning for the same approach, but this
pattern
> > > > defeats it I think.
> > > 
> > > I think the issue is that the notification system will have to basically
> > always
> > > register as an observer to be notified of any new items that need a
> > notification
> > > popped up.  I do agree we'll probably need some kind of cache management
> here.
> > 
> > > Downloads currently just loads the entire SQL database into memory...
> > forever...
> > > which isn't ideal :(.  I agree that we'll need to at least support this
> before
> > > we push this out.
> > 
> > Interesting. I think we were able to handle that with relative ease, because
> we
> > separated notifications observing from downloads UI adapter initially.
> > I am OK with this as long as we work out that cache management at some
point.
> 
> Indeed, the notifications need to always listen in case a download will start
> (suddenly!). 
> One way to avoid the cache (and collection returned by GetAllItems()) to be
> loaded all the time
> is to split Observer to "backend changes observer" (everything except
> OnItemsAvailable) and "collection population observer"
> (with only OnItemsAvailable). The presence of observers of the latter type can
> control the 
> cache, while the observers of the former type would not trigger loading of the
> cache.
> 
> Still lgtm.

Yeah that's a good idea.  I'll look into it :).

[David Trainor- moved to gerrit, 2017-03-08 00:48:12]

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

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

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 01:49:15]

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

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

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...)

[David Trainor- moved to gerrit, 2017-03-09 17:12:16]

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

[commit-bot: I haz the power, 2017-03-09 17:12:46]

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-09 18:25:38]

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

[commit-bot: I haz the power, 2017-03-09 18:25:39]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-03-09 18:34:47]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-03-09 18:34:47]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
fgorski@chromium.org, dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2707003003/#ps100001
(title: "Fixed findbugs warning")

[commit-bot: I haz the power, 2017-03-09 18:35:32]

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 18:43:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489084487408060,
"parent_rev": "ce4a26c9271b0d16f4be5bb2433313113d9a2d24", "commit_rev":
"0317c3b457483a2b3521d60a27e96cad31aeb416"}

[commit-bot: I haz the power, 2017-03-09 18:44:33]

Description was changed from

==========
Initial checkin of the Java OfflineContentProvider

This patch adds the Java counterpart to the C++ OfflineContentProvider.
This patch also connects the native and Java components with a Java/C++
bridge pair and adds the factory methods necessary to create and pass
OfflineItems from C++ to Java.

BUG=691805
==========

to

==========
Initial checkin of the Java OfflineContentProvider

This patch adds the Java counterpart to the C++ OfflineContentProvider.
This patch also connects the native and Java components with a Java/C++
bridge pair and adds the factory methods necessary to create and pass
OfflineItems from C++ to Java.

BUG=691805

Review-Url: https://codereview.chromium.org/2707003003
Cr-Commit-Position: refs/heads/master@{#455808}
Committed:
https://chromium.googlesource.com/chromium/src/+/0317c3b457483a2b3521d60a27e9...
==========

[commit-bot: I haz the power, 2017-03-09 18:44:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0317c3b457483a2b3521d60a27e9...