[Android] Open up API in jni bridge to fetch images from tab

[Android] Open up API in jni bridge to fetch images from tab

Enhanced bookmark feature requires the user to see the image immediately
when he bookmarks a page. By openning this API, the java side can now
call it when the user clicks star button.

BUG=454623
Committed: https://crrev.com/6a6dc80aab193de63e92ead8b97cf33a6e925fee
Cr-Commit-Position: refs/heads/master@{#315454}

[Ian Wen, 2015-02-06 02:11:37]

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

[Ian Wen, 2015-02-06 02:11:37]

Downstream CL: https://chrome-internal-review.googlesource.com/#/c/196255/

[Ted C, 2015-02-06 20:13:29]

lgtm

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

https://codereview.chromium.org/896093007/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:252:
public void fetchImageForTab(Tab tab) {
I would pass WebContents here since I don't think you really need Tab at all.

[Kibeom Kim (inactive), 2015-02-06 20:54:02]

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

https://codereview.chromium.org/896093007/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:252:
public void fetchImageForTab(Tab tab) {
This will also update bookmark's metadata so the item should be bookmarked at
the time it is fetched, or it's just leaking to the image storage. So I think
it'd good to name the function so that it's clear at call-site.


Random note: To me, BookmarkImageService class seemed unsafe. I'm not sure it
considers various leaking scenarios. I'm not 100% sure, but for example, while
fetching the image, if user deleted the bookmark,
|BookmarkImageService::ProcessNewImage| will be called when the fetching is
done. It immediately stores to the database by |PostTaskToStoreImage|, and try
to update bookmark metadata later. If bookmark is not found, it just returns.
Then isn't it that there is no bookmark but image saved to the database.

[Ian Wen, 2015-02-06 22:48:44]

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

https://codereview.chromium.org/896093007/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:252:
public void fetchImageForTab(Tab tab) {
On 2015/02/06 20:54:02, Kibeom Kim wrote:
> This will also update bookmark's metadata so the item should be bookmarked at
> the time it is fetched, or it's just leaking to the image storage. So I think
> it'd good to name the function so that it's clear at call-site.
> 
> 
> Random note: To me, BookmarkImageService class seemed unsafe. I'm not sure it
> considers various leaking scenarios. I'm not 100% sure, but for example, while
> fetching the image, if user deleted the bookmark,
> |BookmarkImageService::ProcessNewImage| will be called when the fetching is
> done. It immediately stores to the database by |PostTaskToStoreImage|, and try
> to update bookmark metadata later. If bookmark is not found, it just returns.
> Then isn't it that there is no bookmark but image saved to the database.

Hmmm.. The scenario you mentioned is not specific to this function, right? If
you call SalientImageForUrl(), and immediately after it you delete a bookmark,
is it still going to be stored by later callbacks? I would file another bug  for
it and modify ProcessForImage() to add an additional check.

[Kibeom Kim (inactive), 2015-02-06 23:02:33]

On 2015/02/06 22:48:44, Ian Wen wrote:
> Hmmm.. The scenario you mentioned is not specific to this function, right? If
> you call SalientImageForUrl(), and immediately after it you delete a bookmark,
> is it still going to be stored by later callbacks? I would file another bug 
for
> it and modify ProcessForImage() to add an additional check.

Thanks for filing a bug. This is just one quick example though. There can be a
lot of cases, for example, it should also consider sudden process kill at any
given code state since it's dealing with local storage.

[Kibeom Kim (inactive), 2015-02-09 21:30:35]

https://codereview.chromium.org/896093007/diff/20001/chrome/browser/enhanced_...
File chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc
(right):

https://codereview.chromium.org/896093007/diff/20001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:118: }
In addition to using WebContent, since this is parallel to
BookmarkImageServiceAndroid::FinishSuccessfulPageLoadForTab . I think it's good
to put there.

[Ian Wen, 2015-02-09 21:53:17]

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

https://codereview.chromium.org/896093007/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:252:
public void fetchImageForTab(Tab tab) {
On 2015/02/06 20:13:28, Ted C wrote:
> I would pass WebContents here since I don't think you really need Tab at all.

Done.

[Kibeom Kim (inactive), 2015-02-09 21:57:24]

https://codereview.chromium.org/896093007/diff/20001/chrome/browser/enhanced_...
File chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc
(right):

https://codereview.chromium.org/896093007/diff/20001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:118: }
On 2015/02/09 21:30:34, Kibeom Kim wrote:
> In addition to using WebContent, since this is parallel to
> BookmarkImageServiceAndroid::FinishSuccessfulPageLoadForTab . I think it's
good
> to put there.

discussed offline. It's just two lines, so let's just leave it.

[Ian Wen, 2015-02-09 22:09:15]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2015-02-09 22:10:10]

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

[Ian Wen, 2015-02-09 22:11:08]

The CQ bit was unchecked by ianwen@chromium.org

[Ian Wen, 2015-02-09 22:12:14]

lgtm? enhanced_bookmark_bridge lives in chrome/browser/enhanced_bookmarks/
folder.

On Mon Feb 09 2015 at 2:10:11 PM <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>   https://chromium-cq-status.appspot.com/patch-status/896093007/20001
>
>
> https://codereview.chromium.org/896093007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kibeom Kim (inactive), 2015-02-09 22:14:44]

did you miss uploading a new patchset?

[Ian Wen, 2015-02-09 22:16:45]

New patchsets have been uploaded after l-g-t-m from tedchoc@chromium.org

[Kibeom Kim (inactive), 2015-02-09 22:29:44]

lgtm with nits

https://codereview.chromium.org/896093007/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java
(right):

https://codereview.chromium.org/896093007/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:250:
* Parses the web content of the given tab, and stores salient images to local
database.
nit: update comment.

https://codereview.chromium.org/896093007/diff/40001/chrome/browser/enhanced_...
File chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h
(right):

https://codereview.chromium.org/896093007/diff/40001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h:92:
BookmarkImageServiceAndroid* bookmark_image_service_;    // weak
nit: align or two spaces each? #90-#92

[Ian Wen, 2015-02-10 00:12:55]

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

[Ian Wen, 2015-02-10 00:13:39]

https://codereview.chromium.org/896093007/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java
(right):

https://codereview.chromium.org/896093007/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:250:
* Parses the web content of the given tab, and stores salient images to local
database.
On 2015/02/09 22:29:44, Kibeom Kim wrote:
> nit: update comment.

Done.

https://codereview.chromium.org/896093007/diff/40001/chrome/browser/enhanced_...
File chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h
(right):

https://codereview.chromium.org/896093007/diff/40001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h:92:
BookmarkImageServiceAndroid* bookmark_image_service_;    // weak
On 2015/02/09 22:29:44, Kibeom Kim wrote:
> nit: align or two spaces each? #90-#92

Done.

[Ian Wen, 2015-02-10 00:13:43]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2015-02-10 00:14:45]

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

[commit-bot: I haz the power, 2015-02-10 00:42:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-10 00:42:59]

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