Allow only one bookmark to be added for multiple fast starring

Allow only one bookmark to be added for multiple fast starring

A quick user might be able to save duplicated bookmarks when he clicks
the star button before the bookmark model is loaded yet. This CL forces
that only one operation can be done before the model is loaded.

BUG=516658
Committed: https://crrev.com/584de1b73f811bcdb98eae1fb0d92b2b7df04fc3
Cr-Commit-Position: refs/heads/master@{#341976}

[Ian Wen, 2015-08-04 21:00:03]

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

[Kibeom Kim (inactive), 2015-08-04 21:15:16]

lgtm

https://codereview.chromium.org/1263223005/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1263223005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:950:
final long bookmarkId = tabToBookmark.getUserBookmarkId();
Hmm.. I wonder if tabToBookmark.getUserBookmarkId() work even if bookmark model
is not loaded? Or it always returns INVALID_BOOKMARK_ID if not loaded?

(not a blocker, unrelated to your fix)

[Ian Wen, 2015-08-04 22:37:59]

https://codereview.chromium.org/1263223005/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1263223005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:950:
final long bookmarkId = tabToBookmark.getUserBookmarkId();
On 2015/08/04 21:15:16, Kibeom Kim wrote:
> Hmm.. I wonder if tabToBookmark.getUserBookmarkId() work even if bookmark
model
> is not loaded? Or it always returns INVALID_BOOKMARK_ID if not loaded?
> 
> (not a blocker, unrelated to your fix)

Did a stacktrace analysis. BookmarkModel is initialized when chrome creates a
profile, which happens very early (the first thing chrome does natively?). And
when the user clicks the star button, empirically the model is already loaded.
Yet bookmark model's loading is always async and bookmark_storage initializes
itself in io thread, there is no theoretical guarantee.

For enhanced bookmark, the reason why our loading call takes seconds is that it
waits for the partner bookmark shim to load, yet for tab_android, it does not
care about partner bookmarks.

[newt (away), 2015-08-05 18:34:42]

lgtm with a suggestion

https://codereview.chromium.org/1263223005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
(right):

https://codereview.chromium.org/1263223005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:66:
if (idToAdd == ChromeBrowserProviderClient.INVALID_BOOKMARK_ID) {
suggestion: use an early return in the case of a valid ID, e.g.

    if (idToAdd != ChromeBrowserProviderClient.INVALID_BOOKMARK_ID) {
        startEditActivity(activity, new BookmarkId(idToAdd,
BookmarkType.NORMAL));
        return;
    }

    final BookmarkId enhancedId = bookmarkModel.addBookmark(...)
    ...

[Ian Wen, 2015-08-05 20:33:05]

https://codereview.chromium.org/1263223005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
(right):

https://codereview.chromium.org/1263223005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:66:
if (idToAdd == ChromeBrowserProviderClient.INVALID_BOOKMARK_ID) {
On 2015/08/05 18:34:41, newt wrote:
> suggestion: use an early return in the case of a valid ID, e.g.
> 
>     if (idToAdd != ChromeBrowserProviderClient.INVALID_BOOKMARK_ID) {
>         startEditActivity(activity, new BookmarkId(idToAdd,
> BookmarkType.NORMAL));
>         return;
>     }
> 
>     final BookmarkId enhancedId = bookmarkModel.addBookmark(...)
>     ...

Good suggestion. Done.

[Ian Wen, 2015-08-05 20:37:07]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-08-05 20:37:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkimlabs@chromium.org, newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1263223005/#ps20001
(title: "respond to comments")

[commit-bot: I haz the power, 2015-08-05 20:39:46]

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

[commit-bot: I haz the power, 2015-08-05 21:21:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-05 21:21:33]

Patchset 2 (id:??) landed as
https://crrev.com/584de1b73f811bcdb98eae1fb0d92b2b7df04fc3
Cr-Commit-Position: refs/heads/master@{#341976}