Java side of purging BookmarkId from offline pages

Java side of purging BookmarkId from offline pages

BUG=NONE

Committed: https://crrev.com/98e033bf0dcfba78c8a022ca07af688fefb7cf22
Cr-Commit-Position: refs/heads/master@{#379662}

[bburns, 2016-02-26 22:53:04]

bburns@chromium.org changed reviewers:
+ dimich@google.com, fgorski@chromium.org

[bburns, 2016-02-26 22:53:05]

This builds off of https://codereview.chromium.org/1694863003/

Please only review the Java bits.

Thanks!
--brendan

[bburns, 2016-02-27 00:48:56]

Ok, I added the right diffbase.  Please review all of this code.

Thanks!
--brendan

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

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBookmarkRow.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBookmarkRow.java:94:
BookmarkModel.offlineClientIdForBookmarkId(bookmarkId));
Have you considered adding a small adapter for bookmarks, e.g.
BookmarkOfflinePageClient, that would hide the double calls and expose the
current Bookmark ID based interface?

I am not saying that is the best solution, but something worth looking at.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:29:
ClientId id = (ClientId) o;
otherId

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

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:90:
* @param bookmarkId A bookmark ID of the deleted offline page.
fix

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:243:
* @param bookmarkId Id of the bookmark associated with an offline page.
Fix

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:271:
* @param bookmarkId Id of the bookmark related to the offline page.
fix documentation

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:331:
Long offlineId = ids.iterator().next();
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.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:339:
* @param bookmarkIds A list of bookmark IDs for which the offline pages will be
deleted.
fix documentation

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:351:
public void deletePages(List<Long> offlineIds, DeletePageCallback callback) {
Does this need to be public?

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:476:
private ClientId getClientIdForOfflineId(long offlineId) {
nit: move above the first @CalledByNative

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageFreeUpSpaceDialog.java:102:
List<Long> clientIds = new ArrayList<>();
offlineIds

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageItem.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageItem.java:42:
@VisibleForTesting
nit: you can likely remove all the @visibleForTesting now that we use all of it
in production code... unless we don't.

I don't think you need to add the tag to getClientId below.

[Dmitry Titov, 2016-02-29 21:24:57]

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

[Dmitry Titov, 2016-02-29 21:24:58]

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:321:
return new ClientId(OfflinePageBridge.BOOKMARK_NAMESPACE,
Long.toString(id.getId()));
Perhaps should use BookmarkId.toString() here and then the
BookamarkId.getBookmarkIdFromString() later.

[bburns, 2016-02-29 21:41:21]

bburns@chromium.org changed reviewers:
+ ianwen@chromium.org

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

+ianwen: for OWNERS approval.

Comments addressed, please take another look.

Thanks!
--brendan

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBookmarkRow.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBookmarkRow.java:94:
BookmarkModel.offlineClientIdForBookmarkId(bookmarkId));
On 2016/02/29 18:42:28, fgorski wrote:
> Have you considered adding a small adapter for bookmarks, e.g.
> BookmarkOfflinePageClient, that would hide the double calls and expose the
> current Bookmark ID based interface?
> 
> I am not saying that is the best solution, but something worth looking at.

Hrm, I'm not sure I'm super happy about wrappers on top of wrappers.  I don't
think that this code is that bad, and its definitely more explicit about what is
going on underneath.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:321:
return new ClientId(OfflinePageBridge.BOOKMARK_NAMESPACE,
Long.toString(id.getId()));
On 2016/02/29 21:24:58, Dmitry Titov wrote:
> Perhaps should use BookmarkId.toString() here and then the
> BookamarkId.getBookmarkIdFromString() later.

Done.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:29:
ClientId id = (ClientId) o;
On 2016/02/29 18:42:28, fgorski wrote:
> otherId

Done.

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

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:90:
* @param bookmarkId A bookmark ID of the deleted offline page.
On 2016/02/29 18:42:29, fgorski wrote:
> fix

Done.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:243:
* @param bookmarkId Id of the bookmark associated with an offline page.
On 2016/02/29 18:42:28, fgorski wrote:
> Fix

Done.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:271:
* @param bookmarkId Id of the bookmark related to the offline page.
On 2016/02/29 18:42:29, fgorski wrote:
> fix documentation

Done.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:331:
Long offlineId = ids.iterator().next();
On 2016/02/29 18:42:29, fgorski wrote:
> 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.

Done.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:339:
* @param bookmarkIds A list of bookmark IDs for which the offline pages will be
deleted.
On 2016/02/29 18:42:28, fgorski wrote:
> fix documentation

Done.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:351:
public void deletePages(List<Long> offlineIds, DeletePageCallback callback) {
On 2016/02/29 18:42:28, fgorski wrote:
> Does this need to be public?

Moved to protected.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:476:
private ClientId getClientIdForOfflineId(long offlineId) {
On 2016/02/29 18:42:29, fgorski wrote:
> nit: move above the first @CalledByNative

Done.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageFreeUpSpaceDialog.java:102:
List<Long> clientIds = new ArrayList<>();
On 2016/02/29 18:42:29, fgorski wrote:
> offlineIds

Done.

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageItem.java
(right):

https://codereview.chromium.org/1739163005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageItem.java:42:
@VisibleForTesting
On 2016/02/29 18:42:29, fgorski wrote:
> nit: you can likely remove all the @visibleForTesting now that we use all of
it
> in production code... unless we don't.

Done.
 
> I don't think you need to add the tag to getClientId below.

Done.

[Dmitry Titov, 2016-02-29 21:50:20]

dimich@chromium.org changed reviewers:
- dimich@google.com

[Dmitry Titov, 2016-02-29 21:50:20]

-dimich@google.com (keeping dimich@chromium.org)

lgtm

[Ian Wen, 2016-02-29 22:11:26]

You should go a  git grep -i "bookmark"  in offline_page folder to see if you
are missing other points.

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:317:
return mOfflinePageBridge;
Do you guys have plan to remove offline page bridge from bookmark bridge?

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:320:
public static ClientId offlineClientIdForBookmarkId(BookmarkId id) {
1. This should really be a factory method in ClientId.
2. Public methods should always have javadoc.

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:324:
public static BookmarkId bookmarkIdForOfflineClientId(ClientId id) {
Rename to getBookmarkIdForOfflineClient()? or create...().

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

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:242:
* Gets an offline page associated with a provided bookmark ID.
s/bookmark ID/client id

[Ian Wen, 2016-02-29 22:12:57]

Also please sanitize the CL description and put a bug reference (if any). If
there is no bug, it should be BUG=NONE.

[bburns, 2016-03-01 17:23:48]

Description was changed from

==========
Java side of purging BookmarkId from offline pages


Address comments.


Add a unit test


Fix unit tests.


move offline id generation to C++, migrate old database entries on load


Address comments.


Address comments.


Addressed comments.


Address comments.


Refactor the offline page storage to include client namespace and id.

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

to

==========
Java side of purging BookmarkId from offline pages

BUG=NONE
==========

[bburns, 2016-03-01 18:06:08]

ianwen: comments addressed, please take another look.

Thanks!
--brendan

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:317:
return mOfflinePageBridge;
On 2016/02/29 22:11:26, Ian Wen wrote:
> Do you guys have plan to remove offline page bridge from bookmark bridge?

Yes, I think eventually it will be an Android Service, but that is in the
future.

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:320:
public static ClientId offlineClientIdForBookmarkId(BookmarkId id) {
On 2016/02/29 22:11:26, Ian Wen wrote:
> 1. This should really be a factory method in ClientId.
> 2. Public methods should always have javadoc.

Done.

https://codereview.chromium.org/1739163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:324:
public static BookmarkId bookmarkIdForOfflineClientId(ClientId id) {
On 2016/02/29 22:11:26, Ian Wen wrote:
> Rename to getBookmarkIdForOfflineClient()? or create...().

Done.

[Ian Wen, 2016-03-01 18:32:56]

Bookamrks lgtm

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

https://codereview.chromium.org/1739163005/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:324:
public static BookmarkId getBookmarkIdForOfflineClientId(ClientId id) {
Maybe assert here that the ClientId's namespace is
OfflinePage#BOOKMARK_NAMESPACE? Up to you though as we don't rely on java
asserts much.

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

https://codereview.chromium.org/1739163005/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:44:
* Create a client id for a bookmark
Creates a {@link ClientId} for a bookmark.

[fgorski, 2016-03-01 18:38:22]

lgtm

[bburns, 2016-03-01 20:29:11]

comments addressed.  thanks!

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

https://codereview.chromium.org/1739163005/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:324:
public static BookmarkId getBookmarkIdForOfflineClientId(ClientId id) {
On 2016/03/01 18:32:56, Ian Wen wrote:
> Maybe assert here that the ClientId's namespace is
> OfflinePage#BOOKMARK_NAMESPACE? Up to you though as we don't rely on java
> asserts much.

No Assert, but I return an illegal BookmarkID (-1)

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

https://codereview.chromium.org/1739163005/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:44:
* Create a client id for a bookmark
On 2016/03/01 18:32:56, Ian Wen wrote:
> Creates a {@link ClientId} for a bookmark.

Done.

[bburns, 2016-03-07 21:13:58]

The CQ bit was checked by bburns@chromium.org

[bburns, 2016-03-07 21:13:58]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
ianwen@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1739163005/#ps220001
(title: "Java side of offline id changes")

[commit-bot: I haz the power, 2016-03-07 21:15:13]

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

[commit-bot: I haz the power, 2016-03-07 22:07:17]

Description was changed from

==========
Java side of purging BookmarkId from offline pages

BUG=NONE
==========

to

==========
Java side of purging BookmarkId from offline pages

BUG=NONE
==========

[commit-bot: I haz the power, 2016-03-07 22:07:19]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-03-07 22:09:02]

Description was changed from

==========
Java side of purging BookmarkId from offline pages

BUG=NONE
==========

to

==========
Java side of purging BookmarkId from offline pages

BUG=NONE

Committed: https://crrev.com/98e033bf0dcfba78c8a022ca07af688fefb7cf22
Cr-Commit-Position: refs/heads/master@{#379662}
==========

[commit-bot: I haz the power, 2016-03-07 22:09:03]

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