[Offline pages] Wiring saving and opening offline page

[Offline pages] Wiring saving and opening offline page

Changes:
1) Added the logic in EnhancedBookmarksModel to wait for OfflinePagesBridge being loaded if it is enabled
2) Supported adding a new offline page. To do this, a new asynchronous method addBookmarkAsync is added to EnhancedBookmarksModel.
3) Supported launching an offline page. getBookmarkLaunchUrl, that returns the launch url, is added to EnhancedBookmarksModel.

BUG=491352
TEST=tests added and updated

Committed: https://crrev.com/ad61982619147865d7feb3b92b6c28d6487a58a2
Cr-Commit-Position: refs/heads/master@{#343531}

[jianli, 2015-08-12 23:46:16]

Patchset #1 (id:1) has been deleted

[jianli, 2015-08-12 23:48:23]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org, kkimlabs@chromium.org, mariakhomenko@chromium.org

[Maria, 2015-08-13 00:44:05]

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:552: *
Notifys the observer that bookmark model has been loaded.
Notifies*

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java
(right):

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:390:
if (url.isEmpty()) {
can url be null here?

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:391:
return;
for simple one line if statements we often use

if (condition) return;

style

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

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:73:
public void onBookmarkAdded(final BookmarkId enhancedId) {
@override?

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:45:
/** Called when the bookmark has been added.
/** should be on the line of its own

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:114:
if (mOfflinePageBridge.isOfflinePageModelLoaded()) {
if this is true, there's no need to add the observer, right?

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:128:
}
does this have to happen before the super call? having super in the middle seems
a bit odd.

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:140:
return mOfflinePageBridge == null ? true : mIsOfflinePageModelLoaded;
could also be:
return super.isBookmarkModelLoaded() && (mOfflinePageBridge == null ||
mIsOfflinePageModelLoaded);

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:150:
// If offfline pages feature is enabled and the loading is not done yet, wait.
offline*

[jianli, 2015-08-13 01:00:56]

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:552: *
Notifys the observer that bookmark model has been loaded.
On 2015/08/13 00:44:04, Maria wrote:
> Notifies*

Done.

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java
(right):

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:390:
if (url.isEmpty()) {
On 2015/08/13 00:44:04, Maria wrote:
> can url be null here?

Added check for null.

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:391:
return;
On 2015/08/13 00:44:04, Maria wrote:
> for simple one line if statements we often use
> 
> if (condition) return;
> 
> style

Done.

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

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:73:
public void onBookmarkAdded(final BookmarkId enhancedId) {
On 2015/08/13 00:44:04, Maria wrote:
> @override?

Done.

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:45:
/** Called when the bookmark has been added.
On 2015/08/13 00:44:04, Maria wrote:
> /** should be on the line of its own

Done.

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:114:
if (mOfflinePageBridge.isOfflinePageModelLoaded()) {
On 2015/08/13 00:44:04, Maria wrote:
> if this is true, there's no need to add the observer, right?

Done.

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:128:
}
On 2015/08/13 00:44:04, Maria wrote:
> does this have to happen before the super call? having super in the middle
seems
> a bit odd.

I think it would be better to do our own cleanup first before cleaning up super.
Changed.

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:140:
return mOfflinePageBridge == null ? true : mIsOfflinePageModelLoaded;
On 2015/08/13 00:44:04, Maria wrote:
> could also be:
> return super.isBookmarkModelLoaded() && (mOfflinePageBridge == null ||
> mIsOfflinePageModelLoaded);

Done.

https://codereview.chromium.org/1285313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:150:
// If offfline pages feature is enabled and the loading is not done yet, wait.
On 2015/08/13 00:44:04, Maria wrote:
> offline*

Done.

[fgorski, 2015-08-13 17:41:51]

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:571:
public void notifyBookmarkModelLoaded() {
this should be protected not public. I think leaving it public allows anyone to
call it, and that will trigger bookmarkModelLoaded on observers regardless if it
is actually loaded.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:73:
private final OfflinePageModelObserver mOfflinePageModelObserver =
Is there a reason we create the observer here, even thou offline pages feature
might be switched off completely? 

can this be moved to 112?

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:143:
public void notifyBookmarkModelLoaded() {
1) as per comment in BookmarksBridge 571 this should be protected.

2) Alternatively I think it is worth changing the if to:
if (isBookmarkModelLoaded())...

3) Another alternative:
in BookmarksBridge:
private void notifyBookmarkModelLoaded() {
  if (isBookmarkModelLoaded()) {
    for loop over observers
  }
}

while this override is gone completely.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:263:
url = mOfflinePageBridge.getPageByBookmarkId(bookmarkId).getOfflineUrl();
what if mOfflinePageBridge.getPageByBookmarkId(bookmarkId) returns null, because
the offline page was deleted... worth adding test if possible...
Code for delete is already in.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:43:
* @see OfflinePageBridge#savePage()
nit: you can move that to interface comment, as the whole interface applies to a
single method.
Same below.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java:229:
final AtomicReference<BookmarkId> result = new AtomicReference<BookmarkId>();
nice - I need something like that in our other tests.

[Kibeom Kim (inactive), 2015-08-13 20:43:49]

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:398:
if (url == null || url.isEmpty()) return;
nit: there is a handy util function for this pattern: TextUtils.isEmpty(url);

Q: when can this happen? And if it happens, do we want to notify user that the
launching is failed in some way?

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

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:112:
tab.getUrl(), tab.getWebContents(), callback);
Hmm... so are we saving offline always even on a bookmarking action(clicking the
star), if OfflinePageBridge.isEnabled() is true?

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:214:
* Add a new bookmark asynchronously.
Can't this also add a bookmark synchronously at #230?

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:259:
String url = getBookmarkById(bookmarkId).getUrl();
suggestion:

Since this doesn't need to be called if (mOfflinePageBridge != null), I'd do

if (mOfflinePageBridge == null) {
    return getBookmarkById(bookmarkId).getUrl();
} else {
    return ....;
}

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java:245:
if (semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
Personally I try not to introduce additional timeouts for test stability. I
think it should be fine without it as tests have default global timeout. What do
you think?

[jianli, 2015-08-13 22:37:19]

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:571:
public void notifyBookmarkModelLoaded() {
On 2015/08/13 17:41:50, fgorski wrote:
> this should be protected not public. I think leaving it public allows anyone
to
> call it, and that will trigger bookmarkModelLoaded on observers regardless if
it
> is actually loaded.

Done.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:398:
if (url == null || url.isEmpty()) return;
On 2015/08/13 20:43:48, Kibeom Kim wrote:
> nit: there is a handy util function for this pattern: TextUtils.isEmpty(url);
Done.

> 
> Q: when can this happen? And if it happens, do we want to notify user that the
> launching is failed in some way?
This could happen when the offline page metadata has been deleted or corrupted.
What do we want to notify the user? I think we're already hitting this for line
396. I added a TODO here.

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

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java:112:
tab.getUrl(), tab.getWebContents(), callback);
On 2015/08/13 20:43:48, Kibeom Kim wrote:
> Hmm... so are we saving offline always even on a bookmarking action(clicking
the
> star), if OfflinePageBridge.isEnabled() is true?

Yes when OfflinePageBridge.isEnabled() is true. This will only be launched to
limited set of countries for experiment purpose.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:73:
private final OfflinePageModelObserver mOfflinePageModelObserver =
On 2015/08/13 17:41:51, fgorski wrote:
> Is there a reason we create the observer here, even thou offline pages feature
> might be switched off completely? 
> 
> can this be moved to 112?

Done.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:143:
public void notifyBookmarkModelLoaded() {
On 2015/08/13 17:41:50, fgorski wrote:
> 1) as per comment in BookmarksBridge 571 this should be protected.
> 
> 2) Alternatively I think it is worth changing the if to:
> if (isBookmarkModelLoaded())...
> 
> 3) Another alternative:
> in BookmarksBridge:
> private void notifyBookmarkModelLoaded() {
>   if (isBookmarkModelLoaded()) {
>     for loop over observers
>   }
> }
> 
> while this override is gone completely.

Took the alternative you suggest. We still need to define
notifyBookmarkModelLoaded as protected since EnhancedBookmarksModel needs to
call it.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:214:
* Add a new bookmark asynchronously.
On 2015/08/13 20:43:48, Kibeom Kim wrote:
> Can't this also add a bookmark synchronously at #230?

Yes. But since we're calling a callback, it is still a async pattern.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:259:
String url = getBookmarkById(bookmarkId).getUrl();
On 2015/08/13 20:43:48, Kibeom Kim wrote:
> suggestion:
> 
> Since this doesn't need to be called if (mOfflinePageBridge != null), I'd do
> 
> if (mOfflinePageBridge == null) {
>     return getBookmarkById(bookmarkId).getUrl();
> } else {
>     return ....;
> }

Done.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:263:
url = mOfflinePageBridge.getPageByBookmarkId(bookmarkId).getOfflineUrl();
On 2015/08/13 17:41:51, fgorski wrote:
> what if mOfflinePageBridge.getPageByBookmarkId(bookmarkId) returns null,
because
> the offline page was deleted... worth adding test if possible...
> Code for delete is already in.

Done.

Will add more tests latet.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:43:
* @see OfflinePageBridge#savePage()
On 2015/08/13 17:41:51, fgorski wrote:
> nit: you can move that to interface comment, as the whole interface applies to
a
> single method.
> Same below.

I rather still keep it here since the comment for params still make more sense
to the individual method.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java:229:
final AtomicReference<BookmarkId> result = new AtomicReference<BookmarkId>();
On 2015/08/13 17:41:51, fgorski wrote:
> nice - I need something like that in our other tests.

Acknowledged.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java:245:
if (semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
On 2015/08/13 20:43:48, Kibeom Kim wrote:
> Personally I try not to introduce additional timeouts for test stability. I
> think it should be fine without it as tests have default global timeout. What
do
> you think?

Done.

[fgorski, 2015-08-13 22:58:52]

Mostly looks good.

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1285313003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:143:
public void notifyBookmarkModelLoaded() {
On 2015/08/13 22:37:18, jianli wrote:
> On 2015/08/13 17:41:50, fgorski wrote:
> > 1) as per comment in BookmarksBridge 571 this should be protected.
> > 
> > 2) Alternatively I think it is worth changing the if to:
> > if (isBookmarkModelLoaded())...
> > 
> > 3) Another alternative:
> > in BookmarksBridge:
> > private void notifyBookmarkModelLoaded() {
> >   if (isBookmarkModelLoaded()) {
> >     for loop over observers
> >   }
> > }
> > 
> > while this override is gone completely.
> 
> Took the alternative you suggest. We still need to define
> notifyBookmarkModelLoaded as protected since EnhancedBookmarksModel needs to
> call it.

Acknowledged.

https://codereview.chromium.org/1285313003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1285313003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:252:
return page == null ? "" : page.getOfflineUrl();
Roll back to the regular url in this case. You simply don't have an offline
copy, but you want to navigate.

if (mOfflinePageBridge != null) {
  page = getPage ...();
  if (page != null) return page.getOfflineUrl();
}

return getBookmarkById(bookmarkId).getUrl();

https://codereview.chromium.org/1285313003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java
(right):

https://codereview.chromium.org/1285313003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java:243:
if (semaphore.tryAcquire()) {
Here is the semantics of tryAcquire:
Acquires a permit from this semaphore, only if one is available at the time of
invocation.
Which means it will not wait for the callback, which is what you expect.

If you want to get rid of timeout, drop the if statement and call acquire() that
either succeeds on callback or times out the test, which causes failure... It
may no longer be a SmallTest though...

[jianli, 2015-08-13 23:45:24]

https://codereview.chromium.org/1285313003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1285313003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:252:
return page == null ? "" : page.getOfflineUrl();
On 2015/08/13 22:58:52, fgorski wrote:
> Roll back to the regular url in this case. You simply don't have an offline
> copy, but you want to navigate.
> 
> if (mOfflinePageBridge != null) {
>   page = getPage ...();
>   if (page != null) return page.getOfflineUrl();
> }
> 
> return getBookmarkById(bookmarkId).getUrl();

Done.

https://codereview.chromium.org/1285313003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java
(right):

https://codereview.chromium.org/1285313003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModelTest.java:243:
if (semaphore.tryAcquire()) {
On 2015/08/13 22:58:52, fgorski wrote:
> Here is the semantics of tryAcquire:
> Acquires a permit from this semaphore, only if one is available at the time of
> invocation.
> Which means it will not wait for the callback, which is what you expect.
> 
> If you want to get rid of timeout, drop the if statement and call acquire()
that
> either succeeds on callback or times out the test, which causes failure... It
> may no longer be a SmallTest though...

Thanks for the info. Switching back to using timeout.

[Kibeom Kim (inactive), 2015-08-14 00:23:34]

chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/* lgtm

[Maria, 2015-08-14 00:37:56]

lgtm

[fgorski, 2015-08-14 02:07:30]

lgtm

[jianli, 2015-08-14 20:36:44]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2015-08-14 20:37:22]

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

[commit-bot: I haz the power, 2015-08-14 20:46:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-14 20:46:25]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jianli, 2015-08-14 20:51:05]

jianli@chromium.org changed reviewers:
+ sky@chromium.org

[jianli, 2015-08-14 20:51:06]

sky for change to chrome/chrome.gyp

[sky, 2015-08-14 21:09:05]

https://codereview.chromium.org/1285313003/diff/100001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/1285313003/diff/100001/chrome/chrome.gyp#newc...
chrome/chrome.gyp:568: # GN: //chrome/android:chrome_java
Do you need to update gn too?

[jianli, 2015-08-14 21:18:48]

On 2015/08/14 21:09:05, sky wrote:
> https://codereview.chromium.org/1285313003/diff/100001/chrome/chrome.gyp
> File chrome/chrome.gyp (right):
> 
>
https://codereview.chromium.org/1285313003/diff/100001/chrome/chrome.gyp#newc...
> chrome/chrome.gyp:568: # GN: //chrome/android:chrome_java
> Do you need to update gn too?

No need to update gn since //src/chrome/android/BUILD.gn already put it in the
right place:
 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/BUI...

This is indeed a fix to the problem that we put it in the wrong place
previously.

[sky, 2015-08-14 22:30:28]

LGTM

[jianli, 2015-08-14 22:59:53]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-08-14 22:59:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, mariakhomenko@chromium.org, kkimlabs@chromium.org
Link to the patchset: https://codereview.chromium.org/1285313003/#ps120001
(title: "Sync")

[commit-bot: I haz the power, 2015-08-14 23:00:12]

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

[commit-bot: I haz the power, 2015-08-14 23:44:51]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-08-14 23:45:30]

Patchset 6 (id:??) landed as
https://crrev.com/ad61982619147865d7feb3b92b6c28d6487a58a2
Cr-Commit-Position: refs/heads/master@{#343531}