Offline Page download bridge

Offline Page download bridge

* This creates a stub of the bridge, which is not yet wired to the back-end.
* It also provides Java side tests for the bridge functionality.

BUG=630817
Committed: https://crrev.com/68cf753deab115193dac19ba10aacf454f4641f0
Cr-Commit-Position: refs/heads/master@{#407916}

[Dmitry Titov, 2016-07-22 17:43:43]

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

[Dmitry Titov, 2016-07-22 17:43:43]

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:28:
public abstract class Observer {
Lets add OnLoaded()

For additional comment:
We also need to specify whether or not the Add/Update/Delete callbacks are
invoked before OnLoad is received by the observer. If the bridge is created when
the backend is already loaded, it's good to fire OnLoaded anyways.
Typically in OnLoaded the Java client synchronously calls GetAllItems().

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:30:
public void onDownloadItemUpdate(String guid) {}
Update -> Updated

Also it can just come with a OPDItem, to save mandatory call to getItem()

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:31:
public void onDownloadItemAdded(String guid) {}
Same as above, can just come with OPDItem

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:47:
public List<OfflinePageDownloadItem> getAllItems() {
This assumes the C++ side keeps cache in RAM. This is a reasonable model, can
save Java side from keeping the list of all items in RAM as well - any time the
item is updated they can just update UI elements. We might reflect this as a
comment.

[fgorski, 2016-07-22 20:19:32]

Updated.

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:28:
public abstract class Observer {
On 2016/07/22 17:43:43, Dmitry Titov wrote:
> Lets add OnLoaded()
> 
> For additional comment:
> We also need to specify whether or not the Add/Update/Delete callbacks are
> invoked before OnLoad is received by the observer. If the bridge is created
when
> the backend is already loaded, it's good to fire OnLoaded anyways.
> Typically in OnLoaded the Java client synchronously calls GetAllItems().

Done.

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:30:
public void onDownloadItemUpdate(String guid) {}
On 2016/07/22 17:43:43, Dmitry Titov wrote:
> Update -> Updated
> 
> Also it can just come with a OPDItem, to save mandatory call to getItem()

Done.

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:31:
public void onDownloadItemAdded(String guid) {}
On 2016/07/22 17:43:43, Dmitry Titov wrote:
> Same as above, can just come with OPDItem

Done.

https://codereview.chromium.org/2171963002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:47:
public List<OfflinePageDownloadItem> getAllItems() {
On 2016/07/22 17:43:43, Dmitry Titov wrote:
> This assumes the C++ side keeps cache in RAM. This is a reasonable model, can
> save Java side from keeping the list of all items in RAM as well - any time
the
> item is updated they can just update UI elements. We might reflect this as a
> comment.

Acknowledged.

[gone, 2016-07-23 23:52:06]

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

[gone, 2016-07-23 23:52:08]

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:30:
* Indicates that the bridge is loaded and consumers can call GetXXX methods on
it.
Indentation is off by one.

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java
(right):

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:37:
// TODO(fgorski): Title would be more meaningful to show in the Download UI,
where the local
Sync with talo@: I asked about showing something other than the filename here
and she said she wanted consistency with the other downloads.

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:44:
* multipart/related. */
Don't think we'll need a MIME type.  The only reason we use it in DownloadItem
is because there can be multiple file types and we need to know what to do for
each type.  If something is represented by an OfflinePageDownloadItem explicitly
then we'll always do the same things for it.

[fgorski, 2016-07-25 18:39:13]

Addressed Dan's comments.

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:30:
* Indicates that the bridge is loaded and consumers can call GetXXX methods on
it.
On 2016/07/23 23:52:07, dfalcantara wrote:
> Indentation is off by one.

Done.

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java
(right):

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:37:
// TODO(fgorski): Title would be more meaningful to show in the Download UI,
where the local
On 2016/07/23 23:52:07, dfalcantara wrote:
> Sync with talo@: I asked about showing something other than the filename here
> and she said she wanted consistency with the other downloads.

Acknowledged.

https://codereview.chromium.org/2171963002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java:44:
* multipart/related. */
On 2016/07/23 23:52:07, dfalcantara wrote:
> Don't think we'll need a MIME type.  The only reason we use it in DownloadItem
> is because there can be multiple file types and we need to know what to do for
> each type.  If something is represented by an OfflinePageDownloadItem
explicitly
> then we'll always do the same things for it.

Done.

[Dmitry Titov, 2016-07-25 19:37:17]

looks good. As per offline discussion, lets add a couple of simple junit tests
(item, observers) and land it.

[fgorski, 2016-07-25 22:52:03]

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

[commit-bot: I haz the power, 2016-07-25 22:52:25]

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

[fgorski, 2016-07-25 22:53:31]

Description was changed from

==========
Offline Page download bridge

BUG=
==========

to

==========
Offline Page download bridge

* This creates a stub of the bridge, which is not yet wired to the back-end.
* It also provides Java side tests for the bridge functionality.

BUG=630817
==========

[fgorski, 2016-07-25 22:53:45]

Added tests. PTAL

[Dmitry Titov, 2016-07-25 23:05:30]

lgtm

[commit-bot: I haz the power, 2016-07-25 23:51:34]

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

[commit-bot: I haz the power, 2016-07-25 23:51:35]

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

[fgorski, 2016-07-26 17:46:57]

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

[commit-bot: I haz the power, 2016-07-26 17:47:28]

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-07-26 18:49:58]

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

[commit-bot: I haz the power, 2016-07-26 18:49:58]

Dry run: This issue passed the CQ dry run.

[gone, 2016-07-26 19:03:23]

lgtm

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:23:
private final ObserverList<Observer> mObservers = new ObserverList<Observer>();
private final above the regular privates

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:25:
private static boolean sIsTesting = false;
I think we loosely follow this ordering convention:

public static class/interface

private static boolean
private final
private

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:36:
public void itemsLoaded() {}
nit: would go with onItemsLoaded, onItemAdded, onItemDeleted, onItemUpdated

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:62:
}
Maybe:
mNativeOfflinePageDownloadBridge = sIsTesting ? 0L : nativeInit(profile);

Otherwise, flip conditional so the positive condition is on top.

[fgorski, 2016-07-26 20:11:27]

all addressed. thanks for the review.

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:23:
private final ObserverList<Observer> mObservers = new ObserverList<Observer>();
On 2016/07/26 19:03:22, dfalcantara wrote:
> private final above the regular privates

Done.

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:25:
private static boolean sIsTesting = false;
On 2016/07/26 19:03:22, dfalcantara wrote:
> I think we loosely follow this ordering convention:
> 
> public static class/interface
> 
> private static boolean
> private final
> private

Done.

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:36:
public void itemsLoaded() {}
On 2016/07/26 19:03:22, dfalcantara wrote:
> nit: would go with onItemsLoaded, onItemAdded, onItemDeleted, onItemUpdated

Done.

https://codereview.chromium.org/2171963002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:62:
}
On 2016/07/26 19:03:22, dfalcantara wrote:
> Maybe:
> mNativeOfflinePageDownloadBridge = sIsTesting ? 0L : nativeInit(profile);
> 
> Otherwise, flip conditional so the positive condition is on top.

Done.

[fgorski, 2016-07-26 20:11:36]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-07-26 20:11:36]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2171963002/#ps130001
(title: "Addressing comments from Dan")

[commit-bot: I haz the power, 2016-07-26 20:12:03]

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-07-26 21:16:48]

Description was changed from

==========
Offline Page download bridge

* This creates a stub of the bridge, which is not yet wired to the back-end.
* It also provides Java side tests for the bridge functionality.

BUG=630817
==========

to

==========
Offline Page download bridge

* This creates a stub of the bridge, which is not yet wired to the back-end.
* It also provides Java side tests for the bridge functionality.

BUG=630817
==========

[commit-bot: I haz the power, 2016-07-26 21:16:50]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-07-26 21:21:26]

Description was changed from

==========
Offline Page download bridge

* This creates a stub of the bridge, which is not yet wired to the back-end.
* It also provides Java side tests for the bridge functionality.

BUG=630817
==========

to

==========
Offline Page download bridge

* This creates a stub of the bridge, which is not yet wired to the back-end.
* It also provides Java side tests for the bridge functionality.

BUG=630817

Committed: https://crrev.com/68cf753deab115193dac19ba10aacf454f4641f0
Cr-Commit-Position: refs/heads/master@{#407916}
==========

[commit-bot: I haz the power, 2016-07-26 21:21:27]

Patchset 8 (id:??) landed as
https://crrev.com/68cf753deab115193dac19ba10aacf454f4641f0
Cr-Commit-Position: refs/heads/master@{#407916}