[Offline pages] Detecting missing offline copy

chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java

Index: chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java
index 8581b8ddd34c598c9e7ebffbd1058fa098160efb..36ee4828fa5c6e8fdf86da195fadebcc2473cc2f 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java
@@ -18,6 +18,7 @@ import org.chromium.chrome.browser.BookmarksBridge.BookmarkModelObserver;
 import org.chromium.chrome.browser.enhancedbookmarks.EnhancedBookmarkManager.UIState;
 import org.chromium.chrome.browser.enhancedbookmarks.EnhancedBookmarkPromoHeader.PromoHeaderShowingChangeListener;
 import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
+import org.chromium.chrome.browser.offlinepages.OfflinePageBridge.OfflinePageModelObserver;
 import org.chromium.chrome.browser.offlinepages.OfflinePageFreeUpSpaceCallback;
 import org.chromium.chrome.browser.offlinepages.OfflinePageFreeUpSpaceDialog;
 import org.chromium.chrome.browser.offlinepages.OfflinePageStorageSpaceHeader;
@@ -77,6 +78,8 @@ class EnhancedBookmarkItemsAdapter extends RecyclerView.Adapter<RecyclerView.Vie
         }
     };
 
+    private OfflinePageModelObserver mOfflinePageModelObserver;
+
     EnhancedBookmarkItemsAdapter(Context context) {
         mContext = context;
 
@@ -272,6 +275,19 @@ class EnhancedBookmarkItemsAdapter extends RecyclerView.Adapter<RecyclerView.Vie
         mPromoHeaderManager = new EnhancedBookmarkPromoHeader(mContext, this);
         OfflinePageBridge offlinePageBridge = mDelegate.getModel().getOfflinePageBridge();
         if (offlinePageBridge != null) {
+            mOfflinePageModelObserver = new OfflinePageModelObserver() {
+                @Override
+                public void offlinePageDeleted(BookmarkId bookmarkId) {
+                    if (mDelegate.getCurrentState() == UIState.STATE_FILTER) {
+                        int deletedPosition = getPositionForBookmark(bookmarkId);
+                        if (deletedPosition >= 0) {
+                            removeItem(deletedPosition);

[Ian Wen, 2015/10/14 23:57:22]

Q: if an offline page is deleted, does it mean the bookmark should be deleted?
Is it possible that the user does not want the offline page stored, but would
like to treat it as a simple bookmark?

[fgorski, 2015/10/15 19:12:29]

No. However, when user deletes bookmark, we make sure to remove the offline
page.
(that is the other patch that you're reviewing from Jian)

What is happening here is:
We know we are in "saved offline" view and we only show bookmarks with an
offline
copy, when we detect that a copy is missing, we remove the bookmark from the
view
without deleting the bookmark. It will be visible when you open "Mobile
bookmarks"
or "All bookmarks/saved pages".
Yes. That is our current implementation. If you go to edit bookmark view there
is
a row telling you what the size of the page is and a [remove] button, of course
for
bookmarks that have the offline copy.

+                        }
+                    }
+                }
+            };
+            offlinePageBridge.addObserver(mOfflinePageModelObserver);
+
             mOfflineStorageHeader = new OfflinePageStorageSpaceHeader(
                     mContext, offlinePageBridge, new OfflinePageFreeUpSpaceCallback() {
                         @Override
@@ -295,7 +311,10 @@ class EnhancedBookmarkItemsAdapter extends RecyclerView.Adapter<RecyclerView.Vie
         mDelegate.removeUIObserver(this);
         mDelegate.getModel().removeObserver(mBookmarkModelObserver);
         mPromoHeaderManager.destroy();
-        if (mOfflineStorageHeader != null) {
+
+        OfflinePageBridge offlinePageBridge = mDelegate.getModel().getOfflinePageBridge();
+        if (offlinePageBridge != null) {

[Ian Wen, 2015/10/14 23:57:22]

Sometimes we use null checks, some times we use OfflinePageBridge#isEnabled().
Is there a best practice to prefer one against the other?

[fgorski, 2015/10/15 19:12:29]

Here is a rule of thumb that we try to apply:
If we are going to be using offlinePageBridge, we check if it is not null,
otherwise, if we don't use it directly, we make a check for isEnabled.

+            offlinePageBridge.removeObserver(mOfflinePageModelObserver);
             mOfflineStorageHeader.destroy();
         }
     }
@@ -315,6 +334,7 @@ class EnhancedBookmarkItemsAdapter extends RecyclerView.Adapter<RecyclerView.Vie
     public void onFilterStateSet(EnhancedBookmarkFilter filter) {
         assert filter == EnhancedBookmarkFilter.OFFLINE_PAGES;
         setBookmarks(null, mDelegate.getModel().getBookmarkIDsByFilter(filter));
+        mDelegate.getModel().getOfflinePageBridge().checkOfflinePageMetadata();
     }
 
     @Override