Makes the OfflinePageBridge.getAllPages method asynchronous.

Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully asynchronous operation.

Making this method async has a number of implications:
* Offline page storage space classes used to be synchronous.  Now the policy uses an async create method, and returns an immutable policy object that can answer questions about the state of the world when it was created.
* The bookmarks UI used to get the list of bookmarks in the same stack as the fiter changed event.  This is now asynchronous and clears all the bookmarks from view until the list is done loading.

BUG=589526
Committed: https://crrev.com/7c0ba2fd24cb3c7b6e47eccfc8405e96fa3bb09b
Cr-Commit-Position: refs/heads/master@{#386494}

[dewittj, 2016-02-25 23:33:05]

Description was changed from

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side.  Ports all Offline Pages code to use the asynchronous API, but
leaves all external code (Bookmarks) using the now-deprecated synchronous
API.  These consumers are to be ported as part of future CLs.

BUG=589526
==========

to

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

BUG=589526
==========

[dewittj, 2016-02-26 17:27:24]

Description was changed from

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

BUG=589526
==========

to

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

Making this method async has a number of implications:
* Offline page storage space classes used to be synchronous.  Now the policy
uses an async create method, and returns an immutable policy object that can
answer questions about the state of the world when it was created.
* The bookmarks UI used to get the list of bookmarks in the same stack as the
fiter changed event.  This is now asynchronous and clears all the bookmarks from
view until the list is done loading.
* The disk space histograms have a new wrapper class that performs asynchronous
operations before and after the disk-using operation.


BUG=589526
==========

[dewittj, 2016-02-26 17:45:57]

dewittj@chromium.org changed reviewers:
+ fgorski@chromium.org

[dewittj, 2016-02-26 17:46:31]

Patchset #1 (id:1) has been deleted

[dewittj, 2016-02-26 17:46:41]

Patchset #1 (id:20001) has been deleted

[dewittj, 2016-02-26 17:46:54]

Patchset #1 (id:40001) has been deleted

[dewittj, 2016-02-26 17:47:10]

Patchset #1 (id:60001) has been deleted

[dewittj, 2016-02-26 17:47:54]

fgorski: PTAL, I will add the bookmarks folks after your review is done.

[fgorski, 2016-02-26 21:31:10]

offline pages side lgtm, but address comments.

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:300:
public void onCreateDone(OfflinePageStorageSpacePolicy policy) {
can this callback happen after bookmarksItemAdapter is destroyed?

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:302:
new OfflinePageStorageSpaceHeader(mContext, offlinePageBridge,
put this in a separate method, it's cleaner

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:368:
mDelegate.getModel().getOfflinePageBridge().checkOfflinePageMetadata();
this does not need to wait for the read to be complete. you can probably call it
independently from onFilterStateSet

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:275:
OfflinePageFreeUpSpaceCallback callback =
extract callback creation from here.
As in earlier file, I don't think we should nest 2 times. It does not look nice.

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:114:
private abstract class DiskSpaceHistogramWrappedOperation implements Runnable {
Add a TODO for me to manage this piece of code out. ( mention bug 565571 which
should address this) 

I suspect we don't really use the the metrics that we are bending over backwards
to get here.

https://codereview.chromium.org/1739503002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1739503002/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:530:
DeletePagesByBookmarkId(std::vector<int64_t>(1, node->id()),
ugly, do we need this?
Will the offline_id patch not help us here?

[dewittj, 2016-03-01 22:28:52]

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:300:
public void onCreateDone(OfflinePageStorageSpacePolicy policy) {
On 2016/02/26 21:31:10, fgorski wrote:
> can this callback happen after bookmarksItemAdapter is destroyed?

Anonymous classes automatically reference the class just as an inner non-static
class does, so the adapter won't be destroyed until after the callback has run
and no more references to the callback exist.

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:302:
new OfflinePageStorageSpaceHeader(mContext, offlinePageBridge,
On 2016/02/26 21:31:10, fgorski wrote:
> put this in a separate method, it's cleaner

Done.

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:368:
mDelegate.getModel().getOfflinePageBridge().checkOfflinePageMetadata();
On 2016/02/26 21:31:10, fgorski wrote:
> this does not need to wait for the read to be complete. you can probably call
it
> independently from onFilterStateSet

Done.

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:275:
OfflinePageFreeUpSpaceCallback callback =
On 2016/02/26 21:31:10, fgorski wrote:
> extract callback creation from here.
> As in earlier file, I don't think we should nest 2 times. It does not look
nice.

Done.

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1739503002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:114:
private abstract class DiskSpaceHistogramWrappedOperation implements Runnable {
On 2016/02/26 21:31:10, fgorski wrote:
> Add a TODO for me to manage this piece of code out. ( mention bug 565571 which
> should address this) 
> 
> I suspect we don't really use the the metrics that we are bending over
backwards
> to get here.

Done.

https://codereview.chromium.org/1739503002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1739503002/diff/80001/components/offline_page...
components/offline_pages/offline_page_model.cc:530:
DeletePagesByBookmarkId(std::vector<int64_t>(1, node->id()),
On 2016/02/26 21:31:10, fgorski wrote:
> ugly, do we need this?
> Will the offline_id patch not help us here?

Done.

[dewittj, 2016-03-01 22:51:29]

dewittj@chromium.org changed reviewers:
+ ianwen@chromium.org, newt@chromium.org

[dewittj, 2016-03-01 22:51:30]

new reviewers, PTAL, thanks.

ianwen: bookmarks
newt: ChromeActivity.java

[newt (away), 2016-03-08 00:08:35]

ChromeActivity lgtm

[Ian Wen, 2016-03-08 19:09:14]

https://chromiumcodereview.appspot.com/1739503002/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://chromiumcodereview.appspot.com/1739503002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:349:
setBookmarks(null, bookmarkIds);
I wonder what the typical loading time is on svelte devices? Have you done any
experiments? If the loading time is long, how do you plan to indicate to the
user that we are doing something in the background?

I am thinking that maybe we can add this as a signal for bookmark model loading.
Currently  bookmark model only claims it is "loaded", if bookmark storage is
initialized and partner bookmark shim is ready. We can add more signals to it as
well if we would like to do that.

https://chromiumcodereview.appspot.com/1739503002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:364:
OfflinePageBridge offlinePageBridge, OfflinePageStorageSpacePolicy policy) {
I think the first parameter, offlinePageBridge, should be pushed to #363.

https://chromiumcodereview.appspot.com/1739503002/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://chromiumcodereview.appspot.com/1739503002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:54:
* Note: Takes ownership of bookmarkModel, and will call |destroy| on it when
finished.
BookmarkUtils has become a place where we put gigantic static methods and it is
very hard to understand this file.

I wonder if we could split the some of the methods, and only put bookmark
related code in this file and put offline-page related functions and if blocks
to offlinePageUtils.

For example, addOrEditBookmark() should become something like this:
{
  if (handledByOfflinePage()) {
    OfflinePageUtils.addOrEditBookmark();
  } else {
    // real bookmark code here.
  }
}

[dewittj, 2016-03-09 19:16:26]

I've made some changes, but please hold off on this patch until a few other
cleanup patches land, which will make this patch clearer:

https://codereview.chromium.org/1777943002
https://codereview.chromium.org/1773403004

https://codereview.chromium.org/1739503002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:349:
setBookmarks(null, bookmarkIds);
On 2016/03/08 19:09:14, Ian Wen wrote:
> I wonder what the typical loading time is on svelte devices? Have you done any
> experiments? If the loading time is long, how do you plan to indicate to the
> user that we are doing something in the background?
> 
> I am thinking that maybe we can add this as a signal for bookmark model
loading.
> Currently  bookmark model only claims it is "loaded", if bookmark storage is
> initialized and partner bookmark shim is ready. We can add more signals to it
as
> well if we would like to do that.

I have not done any experiments, as this is just updating the API; we have not
yet moved the backend to do this work async.  We expect not to load the entire
offline pages model into memory in the near future, so we don't pressure svelte
devices while we're not active.

Unfortunately, if we don't load the whole model into memory once at the
beginning, we also can't simply gate "load" on a single asynchronous offline
pages load event.

It would be interesting to explore some UI surface while the offline pages list
loads, but we haven't designed that yet.

https://codereview.chromium.org/1739503002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:364:
OfflinePageBridge offlinePageBridge, OfflinePageStorageSpacePolicy policy) {
On 2016/03/08 19:09:14, Ian Wen wrote:
> I think the first parameter, offlinePageBridge, should be pushed to #363.

According to "git cl format", this formatting is fine.  Would you rather I
manually move it to the previous line?

https://codereview.chromium.org/1739503002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1739503002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:54:
* Note: Takes ownership of bookmarkModel, and will call |destroy| on it when
finished.
On 2016/03/08 19:09:14, Ian Wen wrote:
> BookmarkUtils has become a place where we put gigantic static methods and it
is
> very hard to understand this file.
> 
> I wonder if we could split the some of the methods, and only put bookmark
> related code in this file and put offline-page related functions and if blocks
> to offlinePageUtils.
> 
> For example, addOrEditBookmark() should become something like this:
> {
>   if (handledByOfflinePage()) {
>     OfflinePageUtils.addOrEditBookmark();
>   } else {
>     // real bookmark code here.
>   }
> }

As a team we are moving away from having Bookmarks-specific code in the offline
pages trees.  So I'd be reluctant to move addOrEditBookmark over there. 
However, there are some methods that I did move onto OfflinePageBridge.

[dewittj, 2016-03-25 20:52:04]

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

[dewittj, 2016-03-25 21:11:51]

Patchset #6 (id:180001) has been deleted

[dewittj, 2016-03-25 21:58:06]

PTAL: I believe this is ready for re-review.  Removed newt as ChromeActivity was
done in a separate patch.

https://codereview.chromium.org/1739503002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:349:
setBookmarks(null, bookmarkIds);
On 2016/03/09 19:16:26, dewittj wrote:
> On 2016/03/08 19:09:14, Ian Wen wrote:
> > I wonder what the typical loading time is on svelte devices? Have you done
any
> > experiments? If the loading time is long, how do you plan to indicate to the
> > user that we are doing something in the background?
> > 
> > I am thinking that maybe we can add this as a signal for bookmark model
> loading.
> > Currently  bookmark model only claims it is "loaded", if bookmark storage is
> > initialized and partner bookmark shim is ready. We can add more signals to
it
> as
> > well if we would like to do that.
> 
> I have not done any experiments, as this is just updating the API; we have not
> yet moved the backend to do this work async.  We expect not to load the entire
> offline pages model into memory in the near future, so we don't pressure
svelte
> devices while we're not active.
> 
> Unfortunately, if we don't load the whole model into memory once at the
> beginning, we also can't simply gate "load" on a single asynchronous offline
> pages load event.
> 
> It would be interesting to explore some UI surface while the offline pages
list
> loads, but we haven't designed that yet.

I have opened crbug.com/598006 to start tracking latency on these callbacks.

[fgorski, 2016-03-28 17:58:46]

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:352:
bookmarkModel.getOfflineBookmarkIds(new
BookmarkModel.GetOfflineBookmarkIdsCallback() {
perhaps extract method?

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(left):

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:290:
public List<BookmarkId> getBookmarkIDsByFilter(BookmarkFilter filter) {
Why are you removing the filter related code?

You can easily do the same by adding the callback as a second parameter, that
does not mention offline IDs.
I defer to bookmarks' owners to decide what's best here.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:92:
public void onGetItems(List<OfflinePageItem> allOfflinePages);
nit: do you need public keyword?
nit: add documentation. (Will the list be empty or null when delivered?)

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:103:
public static class OfflinePageModelObserver {
why are you removing abstract?

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:124:
static long getTotalSize(List<OfflinePageItem> offlinePages) {
I think you can remove this method, as it is no longer used here.
It was probably missed by my or some other patch.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:241:
* the model is already loaded.
add assert if that is required.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:264:
public void onGetItems(List<OfflinePageItem> allPages) {
at least put a TODO that this has to be quickly fixed to not make a full read.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:412:
protected void deletePages(List<Long> offlineIds, final DeletePageCallback
callback) {
do you need final here? and next line?

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:544:
mObservers.clear();
we should probably add a TODO: create a model destroyed method on the observer
and send it out here.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageStorageSpacePolicy.java
(left):

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageStorageSpacePolicy.java:55:
private long getTotalSize(List<OfflinePageItem> offlinePages) {
keep this method here, as it is not needed on the bridge.

https://codereview.chromium.org/1739503002/diff/220001/components/offline_pag...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1739503002/diff/220001/components/offline_pag...
components/offline_pages/offline_page_model_unittest.cc:111:
std::vector<int64_t> bookmark_ids;
offline_ids ?

[dewittj, 2016-03-31 18:43:19]

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:352:
bookmarkModel.getOfflineBookmarkIds(new
BookmarkModel.GetOfflineBookmarkIdsCallback() {
On 2016/03/28 17:58:46, fgorski wrote:
> perhaps extract method?

This callback really only calls one method + UMA, so it seems silly to extract
it.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:92:
public void onGetItems(List<OfflinePageItem> allOfflinePages);
On 2016/03/28 17:58:46, fgorski wrote:
> nit: do you need public keyword?
> nit: add documentation. (Will the list be empty or null when delivered?)

Done.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:103:
public static class OfflinePageModelObserver {
On 2016/03/28 17:58:46, fgorski wrote:
> why are you removing abstract?

Done.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:124:
static long getTotalSize(List<OfflinePageItem> offlinePages) {
On 2016/03/28 17:58:46, fgorski wrote:
> I think you can remove this method, as it is no longer used here.
> It was probably missed by my or some other patch.

Done.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:241:
* the model is already loaded.
On 2016/03/28 17:58:46, fgorski wrote:
> add assert if that is required.

Done.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:264:
public void onGetItems(List<OfflinePageItem> allPages) {
On 2016/03/28 17:58:46, fgorski wrote:
> at least put a TODO that this has to be quickly fixed to not make a full read.

Done.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:412:
protected void deletePages(List<Long> offlineIds, final DeletePageCallback
callback) {
On 2016/03/28 17:58:46, fgorski wrote:
> do you need final here? and next line?

Done.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:544:
mObservers.clear();
On 2016/03/28 17:58:46, fgorski wrote:
> we should probably add a TODO: create a model destroyed method on the observer
> and send it out here.

Done.

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageStorageSpacePolicy.java
(left):

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageStorageSpacePolicy.java:55:
private long getTotalSize(List<OfflinePageItem> offlinePages) {
On 2016/03/28 17:58:46, fgorski wrote:
> keep this method here, as it is not needed on the bridge.

Done.

https://codereview.chromium.org/1739503002/diff/220001/components/offline_pag...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1739503002/diff/220001/components/offline_pag...
components/offline_pages/offline_page_model_unittest.cc:111:
std::vector<int64_t> bookmark_ids;
On 2016/03/28 17:58:46, fgorski wrote:
> offline_ids ?

Done.

[dewittj, 2016-03-31 18:43:43]

Description was changed from

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

Making this method async has a number of implications:
* Offline page storage space classes used to be synchronous.  Now the policy
uses an async create method, and returns an immutable policy object that can
answer questions about the state of the world when it was created.
* The bookmarks UI used to get the list of bookmarks in the same stack as the
fiter changed event.  This is now asynchronous and clears all the bookmarks from
view until the list is done loading.
* The disk space histograms have a new wrapper class that performs asynchronous
operations before and after the disk-using operation.


BUG=589526
==========

to

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

Making this method async has a number of implications:
* Offline page storage space classes used to be synchronous.  Now the policy
uses an async create method, and returns an immutable policy object that can
answer questions about the state of the world when it was created.
* The bookmarks UI used to get the list of bookmarks in the same stack as the
fiter changed event.  This is now asynchronous and clears all the bookmarks from
view until the list is done loading.



BUG=589526
==========

[dewittj, 2016-04-07 17:41:52]

friendly ping,
ianwen: ptal

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(left):

https://codereview.chromium.org/1739503002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:290:
public List<BookmarkId> getBookmarkIDsByFilter(BookmarkFilter filter) {
On 2016/03/28 17:58:46, fgorski wrote:
> Why are you removing the filter related code?
> 
> You can easily do the same by adding the callback as a second parameter, that
> does not mention offline IDs.
> I defer to bookmarks' owners to decide what's best here.

Done.

[Ian Wen, 2016-04-08 00:07:45]

https://chromiumcodereview.appspot.com/1739503002/diff/250001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://chromiumcodereview.appspot.com/1739503002/diff/250001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:315:
mDestroyed = true;
Instead of using mDestroyed, you could set mDelegate to be null and checks
mDelegate instead.

https://chromiumcodereview.appspot.com/1739503002/diff/250001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://chromiumcodereview.appspot.com/1739503002/diff/250001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:243:
OfflinePageBridge bridge = bookmarkModel.getOfflinePageBridge();
I am a bit wary here. Is it possible that we retain the previous way as much as
possible? getFirstUrlToLoad() is a handy function as the logic differes for
phone/tablet and for offline pages.

[dewittj, 2016-04-08 18:06:59]

https://codereview.chromium.org/1739503002/diff/250001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:315:
mDestroyed = true;
On 2016/04/08 00:07:45, Ian Wen wrote:
> Instead of using mDestroyed, you could set mDelegate to be null and checks
> mDelegate instead.

Done.  I annotated other methods that depend on mDelegate with either null
checks or asserts, based on whether it seemed possible that a method could be
called post-onDestroy().  What do you think?

https://codereview.chromium.org/1739503002/diff/250001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1739503002/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:243:
OfflinePageBridge bridge = bookmarkModel.getOfflinePageBridge();
On 2016/04/08 00:07:45, Ian Wen wrote:
> I am a bit wary here. Is it possible that we retain the previous way as much
as
> possible? getFirstUrlToLoad() is a handy function as the logic differes for
> phone/tablet and for offline pages.

I reorganized the patch so that the code is laid out in a similar way as before,
wdyt?

[Ian Wen, 2016-04-11 17:10:14]

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:367:
if (mDelegate == null) return;
move #367 to further above?

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:326:

Remove phantom empty line.

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:239:
ThreadUtils.postOnUiThread(new Runnable() {
Why posting it to UI thread? All the code in this class should be running in UI
thread already.

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:247:
bridge.hasPages(new OfflinePageBridge.HasPagesCallback() {
Ideally such simple method should have been made synchronous. Whether an offline
page model contains any offline pages should have been determined already when
we load the offline page. I see a lot of complexity in the frontend code here
just because we make every call to be async.

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:288:
public void onResult(Boolean result) {
rename "result" to showOfflinePage.

[dewittj, 2016-04-11 20:00:35]

Patchset #12 (id:310001) has been deleted

[dewittj, 2016-04-11 20:00:50]

Patchset #12 (id:330001) has been deleted

[dewittj, 2016-04-11 20:26:45]

I did make hasPages sync, since you're right that this should be trackable at
construction time.  This brings down the impact on BookmarkUtils.  WDYT now?

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java:367:
if (mDelegate == null) return;
On 2016/04/11 17:10:14, Ian Wen wrote:
> move #367 to further above?

Done.

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:326:

On 2016/04/11 17:10:14, Ian Wen wrote:
> Remove phantom empty line.

Done.

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:239:
ThreadUtils.postOnUiThread(new Runnable() {
On 2016/04/11 17:10:14, Ian Wen wrote:
> Why posting it to UI thread? All the code in this class should be running in
UI
> thread already.

In general, I try to ensure that a callback will /always/ be called in a
separate stack trace, since it might be unexpected to callers that callback
could be called before the original function returns.  This is moot since I
agreed with your next comment.

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:247:
bridge.hasPages(new OfflinePageBridge.HasPagesCallback() {
On 2016/04/11 17:10:14, Ian Wen wrote:
> Ideally such simple method should have been made synchronous. Whether an
offline
> page model contains any offline pages should have been determined already when
> we load the offline page. I see a lot of complexity in the frontend code here
> just because we make every call to be async.

I agree that in theory this should be possible to front-load.  It may become
buggy in the presence of deletes, but for the time being I added a sync version
of this.

https://codereview.chromium.org/1739503002/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:288:
public void onResult(Boolean result) {
On 2016/04/11 17:10:14, Ian Wen wrote:
> rename "result" to showOfflinePage.

Obsolete

[Ian Wen, 2016-04-11 20:27:48]

Wow you made bridge#hasPages() happen! Thank you!

lgtm

[dewittj, 2016-04-11 20:38:23]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-04-11 20:38:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1739503002/#ps350001
(title: "Fix some comments.")

[commit-bot: I haz the power, 2016-04-11 20:38:52]

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

[commit-bot: I haz the power, 2016-04-11 22:17:58]

Description was changed from

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

Making this method async has a number of implications:
* Offline page storage space classes used to be synchronous.  Now the policy
uses an async create method, and returns an immutable policy object that can
answer questions about the state of the world when it was created.
* The bookmarks UI used to get the list of bookmarks in the same stack as the
fiter changed event.  This is now asynchronous and clears all the bookmarks from
view until the list is done loading.



BUG=589526
==========

to

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

Making this method async has a number of implications:
* Offline page storage space classes used to be synchronous.  Now the policy
uses an async create method, and returns an immutable policy object that can
answer questions about the state of the world when it was created.
* The bookmarks UI used to get the list of bookmarks in the same stack as the
fiter changed event.  This is now asynchronous and clears all the bookmarks from
view until the list is done loading.



BUG=589526
==========

[commit-bot: I haz the power, 2016-04-11 22:18:00]

Committed patchset #12 (id:350001)

[commit-bot: I haz the power, 2016-04-11 22:19:40]

Description was changed from

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

Making this method async has a number of implications:
* Offline page storage space classes used to be synchronous.  Now the policy
uses an async create method, and returns an immutable policy object that can
answer questions about the state of the world when it was created.
* The bookmarks UI used to get the list of bookmarks in the same stack as the
fiter changed event.  This is now asynchronous and clears all the bookmarks from
view until the list is done loading.



BUG=589526
==========

to

==========
Makes the OfflinePageBridge.getAllPages method asynchronous.

This does not actually make the operation asynchronous on the native
side, but posts a task to the UI thread to ensure that callers expect a fully
asynchronous operation.

Making this method async has a number of implications:
* Offline page storage space classes used to be synchronous.  Now the policy
uses an async create method, and returns an immutable policy object that can
answer questions about the state of the world when it was created.
* The bookmarks UI used to get the list of bookmarks in the same stack as the
fiter changed event.  This is now asynchronous and clears all the bookmarks from
view until the list is done loading.

BUG=589526

Committed: https://crrev.com/7c0ba2fd24cb3c7b6e47eccfc8405e96fa3bb09b
Cr-Commit-Position: refs/heads/master@{#386494}
==========

[commit-bot: I haz the power, 2016-04-11 22:19:42]

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