Implement WebContents.downloadImage() on Java side

Implement WebContents.downloadImage() on Java side

This CL bridges the native WebContents::DownloadImage to Java. We are planning to use the method for fetching artwork images (WIP) in MediaMetadata and show them in Android media notification.

Related CLs:
https://codereview.chromium.org/2013813002/
https://codereview.chromium.org/2015433003/
https://codereview.chromium.org/2009243002/ (most relevant one)

BUG=616411
Committed: https://crrev.com/7aeb422ef7c9f6a387d9f2bcfc0d8cc5c8331b84
Cr-Commit-Position: refs/heads/master@{#397807}

[Zhiqiang Zhang (Slow), 2016-05-31 14:49:41]

zqzhang@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-05-31 16:18:37]

sgtm apart from the JNI calls.

Also, could you point to the other CLs from the same "set". Reviewers will need
to know where and how this is used.

https://codereview.chromium.org/2014553002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2014553002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_android.cc:736:
Java_WebContentsImpl_onDownloadImageFinished(
Would there be a way to avoid doing that many JNI calls? JNI calls are fairly
expensive and I don't think we usually do more than one. I don't know on top of
my head how we could do what you are trying to do without these JNI calls
though. Maybe base/android/jni_array.h can help?

https://codereview.chromium.org/2014553002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content_public/browser/DownloadImageCallback.java
(right):

https://codereview.chromium.org/2014553002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/DownloadImageCallback.java:15:
public interface DownloadImageCallback {
The native callback is called "ImageDownloadCallback". Maybe you should keep
that name for consistency.

[Zhiqiang Zhang (Slow), 2016-06-01 10:54:46]

Description was changed from

==========
Implement WebContents.downloadImage() on Java side

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

to

==========
Implement WebContents.downloadImage() on Java side

This CL bridges the native WebContents::DownloadImage to Java. We are planning
to use the method for fetching artwork images (WIP) in MediaMetadata and show
them in Android media notification.

Related CLs:
https://codereview.chromium.org/2013813002/
https://codereview.chromium.org/2015433003/
https://codereview.chromium.org/2009243002/ (most relevant one)

BUG=616411
==========

[Zhiqiang Zhang (Slow), 2016-06-01 11:05:55]

zqzhang@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Zhiqiang Zhang (Slow), 2016-06-01 11:05:55]

PTAL, addressed Mounir's comments.
+tedchoc to look at all the changes.

https://codereview.chromium.org/2014553002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2014553002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_android.cc:736:
Java_WebContentsImpl_onDownloadImageFinished(
On 2016/05/31 16:18:37, Mounir Lamouri (slow) wrote:
> Would there be a way to avoid doing that many JNI calls? JNI calls are fairly
> expensive and I don't think we usually do more than one. I don't know on top
of
> my head how we could do what you are trying to do without these JNI calls
> though. Maybe base/android/jni_array.h can help?

"jni_array.h" doesn't seem to have what we need.

I merged createSize() and addToSizeList() into createSizeAndAddToList() (found
this pattern elsewhere).

For Bitmaps I think we should keep it, since the logic in ConvertToJavaBitmap is
non-trivial and we should reuse it.

https://codereview.chromium.org/2014553002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content_public/browser/DownloadImageCallback.java
(right):

https://codereview.chromium.org/2014553002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/DownloadImageCallback.java:15:
public interface DownloadImageCallback {
On 2016/05/31 16:18:37, Mounir Lamouri (slow) wrote:
> The native callback is called "ImageDownloadCallback". Maybe you should keep
> that name for consistency.

Done.

[mlamouri (slow - plz ping), 2016-06-02 15:55:00]

I'm still concerned that we are making too many JNI calls (two loops + 3 calls)
for onFinishDownloadImage. Though, I don't have a strong knowledge about JNI so
I'm only assuming it's bad and don't have real alternative to propose. I will
let tedchoc@ have a look.

[Ted C, 2016-06-03 16:44:10]

lgtm

basically nits about style and requests for additional comments...the jni is
unavoidable so it seems reasonable to me

https://codereview.chromium.org/2014553002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2014553002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:728:
ScopedJavaLocalRef<jobject> jbitmap = gfx::ConvertToJavaBitmap(&bitmap);
forgive my ignorance (and for my education), but how could multiple bitmaps be
returned?

https://codereview.chromium.org/2014553002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:729:
Java_WebContentsImpl_addToBitmapList(env, jbitmaps.obj(), jbitmap.obj());
This is the only really sad line.  Converting to java bitmaps results in
duplicate memory allocations, so we're more likely to hit OOMs here if people
use it incorrectly or for the wrong purposes.

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:476:
public void downloadImage(String url, boolean isFavicon, int maxBitmapSize,
let's return the id here to match the native signature and to make the callback
a bit more explainable.

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:477:
boolean bypassCache, ImageDownloadCallback callback) {
indent 8 from the start of the previous line

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/ImageDownloadCallback.java
(right):

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/ImageDownloadCallback.java:19:
void onFinishDownloadImage(int id, int httpStatusCode, String imageUrl,
List<Bitmap> bitmaps,
Might be good to add javadoc about these.

In particular, sizes is somewhat unexpected as it reflect the original size
prior to resizing.

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/ImageDownloadCallback.java:20:
List<Rect> sizes);
same indenting comment (here and the following files)

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:365:
public void downloadImage(String url, boolean isFavicon, int maxBitmapSize,
add javadoc

[Zhiqiang Zhang (Slow), 2016-06-03 18:45:24]

Patchset #4 (id:60001) has been deleted

[Zhiqiang Zhang (Slow), 2016-06-03 18:53:13]

Patchset #4 (id:80001) has been deleted

[Zhiqiang Zhang (Slow), 2016-06-03 18:53:52]

https://codereview.chromium.org/2014553002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2014553002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:728:
ScopedJavaLocalRef<jobject> jbitmap = gfx::ConvertToJavaBitmap(&bitmap);
On 2016/06/03 16:44:09, Ted C wrote:
> forgive my ignorance (and for my education), but how could multiple bitmaps be
> returned?

Yes, .ico files can contain multiple images of different sizes.

https://codereview.chromium.org/2014553002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:729:
Java_WebContentsImpl_addToBitmapList(env, jbitmaps.obj(), jbitmap.obj());
On 2016/06/03 16:44:09, Ted C wrote:
> This is the only really sad line.  Converting to java bitmaps results in
> duplicate memory allocations, so we're more likely to hit OOMs here if people
> use it incorrectly or for the wrong purposes.

Added comments about this.

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:476:
public void downloadImage(String url, boolean isFavicon, int maxBitmapSize,
On 2016/06/03 16:44:09, Ted C wrote:
> let's return the id here to match the native signature and to make the
callback
> a bit more explainable.

Done.

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:477:
boolean bypassCache, ImageDownloadCallback callback) {
On 2016/06/03 16:44:09, Ted C wrote:
> indent 8 from the start of the previous line

Done.

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/ImageDownloadCallback.java
(right):

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/ImageDownloadCallback.java:19:
void onFinishDownloadImage(int id, int httpStatusCode, String imageUrl,
List<Bitmap> bitmaps,
On 2016/06/03 16:44:09, Ted C wrote:
> Might be good to add javadoc about these.
> 
> In particular, sizes is somewhat unexpected as it reflect the original size
> prior to resizing.

Done.

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/ImageDownloadCallback.java:20:
List<Rect> sizes);
On 2016/06/03 16:44:09, Ted C wrote:
> same indenting comment (here and the following files)

Done.

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2014553002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:365:
public void downloadImage(String url, boolean isFavicon, int maxBitmapSize,
On 2016/06/03 16:44:09, Ted C wrote:
> add javadoc

Done.

[Zhiqiang Zhang (Slow), 2016-06-03 18:53:56]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-06-03 18:53:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2014553002/#ps100001
(title: "addressed Ted's comments")

[commit-bot: I haz the power, 2016-06-03 18:54:21]

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

[commit-bot: I haz the power, 2016-06-03 21:27:53]

Description was changed from

==========
Implement WebContents.downloadImage() on Java side

This CL bridges the native WebContents::DownloadImage to Java. We are planning
to use the method for fetching artwork images (WIP) in MediaMetadata and show
them in Android media notification.

Related CLs:
https://codereview.chromium.org/2013813002/
https://codereview.chromium.org/2015433003/
https://codereview.chromium.org/2009243002/ (most relevant one)

BUG=616411
==========

to

==========
Implement WebContents.downloadImage() on Java side

This CL bridges the native WebContents::DownloadImage to Java. We are planning
to use the method for fetching artwork images (WIP) in MediaMetadata and show
them in Android media notification.

Related CLs:
https://codereview.chromium.org/2013813002/
https://codereview.chromium.org/2015433003/
https://codereview.chromium.org/2009243002/ (most relevant one)

BUG=616411
==========

[commit-bot: I haz the power, 2016-06-03 21:27:54]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-06-03 21:30:14]

Description was changed from

==========
Implement WebContents.downloadImage() on Java side

This CL bridges the native WebContents::DownloadImage to Java. We are planning
to use the method for fetching artwork images (WIP) in MediaMetadata and show
them in Android media notification.

Related CLs:
https://codereview.chromium.org/2013813002/
https://codereview.chromium.org/2015433003/
https://codereview.chromium.org/2009243002/ (most relevant one)

BUG=616411
==========

to

==========
Implement WebContents.downloadImage() on Java side

This CL bridges the native WebContents::DownloadImage to Java. We are planning
to use the method for fetching artwork images (WIP) in MediaMetadata and show
them in Android media notification.

Related CLs:
https://codereview.chromium.org/2013813002/
https://codereview.chromium.org/2015433003/
https://codereview.chromium.org/2009243002/ (most relevant one)

BUG=616411

Committed: https://crrev.com/7aeb422ef7c9f6a387d9f2bcfc0d8cc5c8331b84
Cr-Commit-Position: refs/heads/master@{#397807}
==========

[commit-bot: I haz the power, 2016-06-03 21:30:15]

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