[Enhanced Bookmark]Upstream image fetching code in android

[Enhanced Bookmark]Upstream image fetching code in android

This CL added the image fetching code that previously lived in clank private repository.

Also I moved android enhanced_bookmark_bridge from
chrome/browser/android/enhanced_bookmarks/ to chrome/browser/enhanced_bookmarks/android,
since the latter is a more unified place to put all files in.(I deleted the
android/enhanced_bookmarks folder).

Last but not least, I made some small changes to the internal implementation of
bookmark_image_service, making it more readable.

The previously private code can be found in code search:
https://cs.corp.google.com/#clankium/src/clank/native/framework/chrome/enhanced_bookmarks/

Downstream CL: https://chrome-internal-review.googlesource.com/#/c/195438/
BUG=454623
Committed: https://crrev.com/566c432e789653356da210e066b1f0ed62f1aa3d
Cr-Commit-Position: refs/heads/master@{#314872}

[Ian Wen, 2015-02-03 18:21:36]

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

[Ian Wen, 2015-02-03 18:21:37]

Dear reviewers, I have this large CL for you to review. This is an important
step for enhanced bookmarks.

lpromeo@, could you ptal the modification I made to
components/bookmark_image_service?

sky@, could you ptal at tab_helpers.cc?

Thank you all! :-)

[sky, 2015-02-03 21:29:31]

tab_helpers.cc LGTM

[noyau (Ping after 24h), 2015-02-04 12:45:27]

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

[noyau (Ping after 24h), 2015-02-04 12:45:27]

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.cc:185: return;
The refactoring leads to the duplication of this code, something that I was
trying to avoid originally. Personally I prefer the original version and don't
see an improvement on readability here.

[lpromero, 2015-02-04 13:54:51]

Mostly rubberstamp lgtm.
Please consider noyau's comments.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.cc:181: // If the request
is done while the image is currently being retrieved, just
Add DCHECK called on valid thread.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_image_service.h (right):

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.h:83: // Stores the image
to local. If update_bookmarks is true, relates the
s/local/local storage.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.h:112: // Retrieve the
salient image associated with page_url from local storage.
s/Retrieve/Retrieves

[Kibeom Kim (inactive), 2015-02-05 01:19:24]

lgtm
 - chrome/browser/enhanced_bookmark/*
 - *.java

with the other comments addressed. Thanks!

https://codereview.chromium.org/899653003/diff/1/chrome/browser/ui/tab_helper...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/899653003/diff/1/chrome/browser/ui/tab_helper...
chrome/browser/ui/tab_helpers.cc:167:
EnhancedBookmarkTabHelper::CreateForWebContents(web_contents);
nit: seems other parts of this function is sorted alphabetically.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.cc:193: bool
update_bookmarks,
Question to noyau@ or lpromero@: can we remove update_bookmarks? Seems it's
unnecessary now.

[noyau (Ping after 24h), 2015-02-05 08:49:59]

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.cc:193: bool
update_bookmarks,
On 2015/02/05 01:19:24, Kibeom Kim wrote:
> Question to noyau@ or lpromero@: can we remove update_bookmarks? Seems it's
> unnecessary now.

This is not unnecessary. This decides if the image url is set on the bookmark.
And is necessary because if the image url comes from the bookmark in the first
place there is no need to update it.

[Ian Wen, 2015-02-05 18:32:43]

New patchsets have been uploaded after l-g-t-m from
sky@chromium.org,lpromero@chromium.org,kkimlabs@chromium.org

[Ian Wen, 2015-02-05 18:33:17]

Thanks guys for quick lgtm. :)

https://codereview.chromium.org/899653003/diff/1/chrome/browser/ui/tab_helper...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/899653003/diff/1/chrome/browser/ui/tab_helper...
chrome/browser/ui/tab_helpers.cc:167:
EnhancedBookmarkTabHelper::CreateForWebContents(web_contents);
On 2015/02/05 01:19:24, Kibeom Kim wrote:
> nit: seems other parts of this function is sorted alphabetically.

Done.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.cc:181: // If the request
is done while the image is currently being retrieved, just
On 2015/02/04 13:54:51, lpromero wrote:
> Add DCHECK called on valid thread.

Reverted the signature change.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.cc:185: return;
On 2015/02/04 12:45:27, noyau wrote:
> The refactoring leads to the duplication of this code, something that I was
> trying to avoid originally. Personally I prefer the original version and don't
> see an improvement on readability here.

Reverted.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.cc:193: bool
update_bookmarks,
On 2015/02/05 01:19:24, Kibeom Kim wrote:
> Question to noyau@ or lpromero@: can we remove update_bookmarks? Seems it's
> unnecessary now.

update_bookmarks is useful to prefetch the image when enhanced bookmark is not
turned on. We want the users to see some images when he turns on this feature.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_image_service.h (right):

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.h:83: // Stores the image
to local. If update_bookmarks is true, relates the
On 2015/02/04 13:54:51, lpromero wrote:
> s/local/local storage.

Done.

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.h:112: // Retrieve the
salient image associated with page_url from local storage.
On 2015/02/04 13:54:51, lpromero wrote:
> s/Retrieve/Retrieves

Done.

[Ian Wen, 2015-02-05 18:33:32]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2015-02-05 18:34:31]

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

[commit-bot: I haz the power, 2015-02-05 18:45:36]

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

[commit-bot: I haz the power, 2015-02-05 18:45:37]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Kibeom Kim (inactive), 2015-02-05 18:51:34]

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/899653003/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_image_service.cc:193: bool
update_bookmarks,
On 2015/02/05 08:49:59, noyau wrote:
> 
> This is not unnecessary. This decides if the image url is set on the bookmark.
> And is necessary because if the image url comes from the bookmark in the first
> place there is no need to update it.

I see I missed that part.


On 2015/02/05 18:33:17, Ian Wen wrote:
> 
> update_bookmarks is useful to prefetch the image when enhanced bookmark is not
> turned on. We want the users to see some images when he turns on this feature.

If the feature is turned off, this function shouldn't be called at the first
place so it doesn't seem that's the usecase.

e.g., line #34 is always true.
https://codereview.chromium.org/899653003/diff/20001/chrome/browser/enhanced_...

[Ian Wen, 2015-02-05 19:00:34]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2015-02-05 19:01:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/899653003/40001

[Kibeom Kim (inactive), 2015-02-05 19:09:01]

+twellington fyi

[commit-bot: I haz the power, 2015-02-05 20:56:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-05 20:57:51]

Patchset 3 (id:??) landed as
https://crrev.com/566c432e789653356da210e066b1f0ed62f1aa3d
Cr-Commit-Position: refs/heads/master@{#314872}