Java side of purging BookmarkId from offline pages

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java

Index: chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
index 2f6223bffffa7e10a5e5e28df133014480fc4df4..8ae9108a2ee07c80116ada1aa7b95fd4636d9972 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
@@ -12,8 +12,6 @@ import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.profiles.Profile;
-import org.chromium.components.bookmarks.BookmarkId;
-import org.chromium.components.bookmarks.BookmarkType;
 import org.chromium.components.offlinepages.DeletePageResult;
 import org.chromium.components.offlinepages.FeatureMode;
 import org.chromium.components.offlinepages.SavePageResult;
@@ -88,11 +86,10 @@ public final class OfflinePageBridge {
 
         /**
          * Called when an offline page is deleted. This can be called as a result of
-         * TODO(bburns): Switch to offline id/client id
          * #checkOfflinePageMetadata().
          * @param bookmarkId A bookmark ID of the deleted offline page.

[fgorski, 2016/02/29 18:42:29]

fix

[bburns, 2016/02/29 21:41:22]

Done.

          */
-        public void offlinePageDeleted(BookmarkId id) {}
+        public void offlinePageDeleted(long offlineId, ClientId clientId) {}
     }
 
     private static long getTotalSize(List<OfflinePageItem> offlinePages) {
@@ -229,10 +226,10 @@ public final class OfflinePageBridge {
      * @return A list of all offline ids that match a particular
      * (namespace, client_id)
      */
-    private Set<Long> getOfflineIdsForClientId(String clientIdNamespace, String clientId) {
+    private Set<Long> getOfflineIdsForClientId(ClientId clientId) {
         assert mIsNativeOfflinePageModelLoaded;
         long[] offlineIds = nativeGetOfflineIdsForClientId(
-                mNativeOfflinePageBridge, clientIdNamespace, clientId);
+                mNativeOfflinePageBridge, clientId.getNamespace(), clientId.getId());
         Set<Long> result = new HashSet<>(offlineIds.length);
         for (long id : offlineIds) {
             result.add(id);
@@ -247,9 +244,8 @@ public final class OfflinePageBridge {
      * @return An {@link OfflinePageItem} matching the bookmark Id or <code>null</code> if none
      * exist.
      */
-    public OfflinePageItem getPageByBookmarkId(BookmarkId bookmarkId) {
-        Set<Long> ids =
-                getOfflineIdsForClientId(BOOKMARK_NAMESPACE, Long.toString(bookmarkId.getId()));
+    public OfflinePageItem getPageByClientId(ClientId clientId) {
+        Set<Long> ids = getOfflineIdsForClientId(clientId);
         if (ids.size() == 0) {
             return null;
         }
@@ -276,7 +272,7 @@ public final class OfflinePageBridge {
      * @param callback Interface that contains a callback.
      * @see SavePageCallback
      */
-    public void savePage(final WebContents webContents, final BookmarkId bookmarkId,
+    public void savePage(final WebContents webContents, final ClientId clientId,
             final SavePageCallback callback) {
         assert mIsNativeOfflinePageModelLoaded;
         assert webContents != null;
@@ -302,63 +298,62 @@ public final class OfflinePageBridge {
         };
         recordFreeSpaceHistograms(
                 "OfflinePages.SavePage.FreeSpacePercentage", "OfflinePages.SavePage.FreeSpaceMB");
-        String namespace = BOOKMARK_NAMESPACE;
-        String clientId = Long.toString(bookmarkId.getId());
 
-        nativeSavePage(mNativeOfflinePageBridge, callbackWrapper, webContents, namespace, clientId);
+        nativeSavePage(mNativeOfflinePageBridge, callbackWrapper, webContents,
+                clientId.getNamespace(), clientId.getId());
     }
 
     /**
      * Marks that an offline page related to a specified bookmark has been accessed.
      *
-     * @param bookmarkId Bookmark ID for which the offline copy will be deleted.
+     * @param offlineId offline ID for which the offline copy will be deleted.
      */
-    private void markPageAccessed(BookmarkId bookmarkId) {
+    private void markPageAccessed(long offlineId) {
         assert mIsNativeOfflinePageModelLoaded;
-        Set<Long> ids =
-                getOfflineIdsForClientId(BOOKMARK_NAMESPACE, Long.toString(bookmarkId.getId()));
-        Long offlineId = ids.iterator().next();
         nativeMarkPageAccessed(mNativeOfflinePageBridge, offlineId);
     }
 
     /**
      * Deletes an offline page related to a specified bookmark.
      *
-     * @param bookmarkId Bookmark ID for which the offline copy will be deleted.
+     * @param clientId Client ID for which the offline copy will be deleted.
      * @param callback Interface that contains a callback.
      * @see DeletePageCallback
      */
-    public void deletePage(final BookmarkId bookmarkId, DeletePageCallback callback) {
+    public void deletePage(final ClientId clientId, DeletePageCallback callback) {
         assert mIsNativeOfflinePageModelLoaded;
 
         recordFreeSpaceHistograms("OfflinePages.DeletePage.FreeSpacePercentage",
                 "OfflinePages.DeletePage.FreeSpaceMB");
 
         DeletePageCallback callbackWrapper = wrapCallbackWithHistogramReporting(callback);
-        Set<Long> ids =
-                getOfflineIdsForClientId(BOOKMARK_NAMESPACE, Long.toString(bookmarkId.getId()));
+        Set<Long> ids = getOfflineIdsForClientId(clientId);
         Long offlineId = ids.iterator().next();

[fgorski, 2016/02/29 18:42:29]

This is no longer valid. I think you should be deleting all pages matching
client ID.
Just call deletePages(List<long>, DPC)

Also I think Justin is close to removing deletePage
https://codereview.chromium.org/1739503002/diff/80001/chrome/browser/android/...
Please comment on his patch if you oppose, but I think it makes sense.

[bburns, 2016/02/29 21:41:23]

Done.

         nativeDeletePage(mNativeOfflinePageBridge, callbackWrapper, offlineId);
     }
 
     /**
-     * Deletes offline pages based on the list of provided bookamrk IDs. Calls the callback
+     * Deletes offline pages based on the list of provided client IDs. Calls the callback
      * when operation is complete. Requires that the model is already loaded.
      *
      * @param bookmarkIds A list of bookmark IDs for which the offline pages will be deleted.

[fgorski, 2016/02/29 18:42:28]

fix documentation

[bburns, 2016/02/29 21:41:23]

Done.

      * @param callback A callback that will be called once operation is completed.
      */
-    public void deletePages(List<BookmarkId> bookmarkIds, DeletePageCallback callback) {
+    public void deletePagesByClientId(List<ClientId> clientIds, DeletePageCallback callback) {
         assert mIsNativeOfflinePageModelLoaded;
-        List<Long> idList = new ArrayList<>(bookmarkIds.size());
-        for (int i = 0; i < bookmarkIds.size(); i++) {
-            idList.addAll(getOfflineIdsForClientId(
-                    BOOKMARK_NAMESPACE, Long.toString(bookmarkIds.get(i).getId())));
+        List<Long> idList = new ArrayList<>(clientIds.size());
+        for (ClientId clientId : clientIds) {
+            idList.addAll(getOfflineIdsForClientId(clientId));
         }
-        long[] ids = new long[idList.size()];
-        for (int i = 0; i < idList.size(); i++) {
-            ids[i] = idList.get(i);
+        deletePages(idList, callback);
+    }
+
+    public void deletePages(List<Long> offlineIds, DeletePageCallback callback) {

[fgorski, 2016/02/29 18:42:28]

Does this need to be public?

[bburns, 2016/02/29 21:41:22]

Moved to protected.

+        long[] ids = new long[offlineIds.size()];
+        for (int i = 0; i < offlineIds.size(); i++) {
+            ids[i] = offlineIds.get(i);
         }
+
         recordFreeSpaceHistograms("OfflinePages.DeletePage.FreeSpacePercentage",
                 "OfflinePages.DeletePage.FreeSpaceMB");
 
@@ -422,7 +417,7 @@ public final class OfflinePageBridge {
 
         // Mark that the offline page has been accessed, that will cause last access time and access
         // count being updated.
-        markPageAccessed(page.getBookmarkId());
+        markPageAccessed(page.getOfflineId());
 
         // Returns the offline URL for offline access.
         return page.getOfflineUrl();
@@ -472,26 +467,30 @@ public final class OfflinePageBridge {
 
     @CalledByNative
     private void offlinePageDeleted(long offlineId) {
-        BookmarkId id = new BookmarkId(offlineId, BookmarkType.NORMAL);
+        ClientId clientId = getClientIdForOfflineId(offlineId);
         for (OfflinePageModelObserver observer : mObservers) {
-            observer.offlinePageDeleted(id);
+            observer.offlinePageDeleted(offlineId, clientId);
         }
     }
 
+    private ClientId getClientIdForOfflineId(long offlineId) {

[fgorski, 2016/02/29 18:42:29]

nit: move above the first @CalledByNative

[bburns, 2016/02/29 21:41:23]

Done.

+        return nativeGetPageByOfflineId(mNativeOfflinePageBridge, offlineId).getClientId();
+    }
+
     @CalledByNative
     private static void createOfflinePageAndAddToList(List<OfflinePageItem> offlinePagesList,
-            String url, long offlineId, String offlineUrl, long fileSize, long creationTime,
-            int accessCount, long lastAccessTimeMs) {
-        offlinePagesList.add(createOfflinePageItem(
-                url, offlineId, offlineUrl, fileSize, creationTime, accessCount, lastAccessTimeMs));
+            String url, long offlineId, String clientNamespace, String clientId, String offlineUrl,
+            long fileSize, long creationTime, int accessCount, long lastAccessTimeMs) {
+        offlinePagesList.add(createOfflinePageItem(url, offlineId, clientNamespace, clientId,
+                offlineUrl, fileSize, creationTime, accessCount, lastAccessTimeMs));
     }
 
     @CalledByNative
     private static OfflinePageItem createOfflinePageItem(String url, long offlineId,
-            String offlineUrl, long fileSize, long creationTime, int accessCount,
-            long lastAccessTimeMs) {
-        return new OfflinePageItem(
-                url, offlineId, offlineUrl, fileSize, creationTime, accessCount, lastAccessTimeMs);
+            String clientNamespace, String clientId, String offlineUrl, long fileSize,
+            long creationTime, int accessCount, long lastAccessTimeMs) {
+        return new OfflinePageItem(url, offlineId, clientNamespace, clientId, offlineUrl, fileSize,
+                creationTime, accessCount, lastAccessTimeMs);
     }
 
     private static native int nativeGetFeatureMode();