[AiS] Java bridge for AnswersImageService

[AiS] Java bridge for AnswersImageService

BUG=380916
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278546

[groby-ooo-7-16, 2014-06-05 03:40:41]

PTAL - the bridge for the ImageService.

(Service CL is at https://codereview.chromium.org/319623003/)

[Ted C, 2014-06-06 01:49:24]

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java
(right):

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:33:
public final AnswersImageObserver observer;
the indenting of this needs to be 4 (and many places in this file are using 2
space indenting instead of 4).

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:49:
public static AnswersImageRequest requestAnswersImage(
why not just pass back the request ID here?  You passed in the observer, so it
doesn't really need to come back in the request.

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
File chrome/browser/android/omnibox/answers_image_bridge.cc (right):

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.cc:46: jobject
java_answers_image_observer_;
Instead of a raw jobject, can you store this as a
base::android::ScopedJavaGlobalRef?

I'm surprised this works actually as the local reference to the jobject should
have been lost when returning from RequestAnaswersImage.

Here's an pretty concise description of what these mean if you're morbidly
interested:
http://developer.android.com/training/articles/perf-jni.html#local_and_global...

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.cc:69: if (profile) {
I would just dcheck profile here.  People shouldn't be calling these apis with
bad profiles.  Same above.

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.cc:77: env, java_callback,
request_id).Release();
indented too much

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
File chrome/browser/android/omnibox/answers_image_bridge.h (right):

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.h:17: explicit
AnswersImageBridge(Profile*);
This header seems quite out of sync with the .cc file.  It looks like all of
this can actually be removed except for the jni registration method.

[groby-ooo-7-16, 2014-06-13 23:52:22]

PTAL

The underlying service has been renamed, so split the consequences of that out
into a separate PS. Same for the re-indentation.

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java
(right):

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:33:
public final AnswersImageObserver observer;
On 2014/06/06 01:49:23, Ted C wrote:
> the indenting of this needs to be 4 (and many places in this file are using 2
> space indenting instead of 4).

Done.

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:49:
public static AnswersImageRequest requestAnswersImage(
On 2014/06/06 01:49:23, Ted C wrote:
> why not just pass back the request ID here?  You passed in the observer, so it
> doesn't really need to come back in the request.

It allows the Java side to hold a pointer to the observer. I'd like to avoid the
C++ side holding on to Java objects, and if the observer is an anonymous class,
somebody needs to hold on to it.

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
File chrome/browser/android/omnibox/answers_image_bridge.cc (right):

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.cc:46: jobject
java_answers_image_observer_;
On 2014/06/06 01:49:23, Ted C wrote:
> Instead of a raw jobject, can you store this as a
> base::android::ScopedJavaGlobalRef?
> 
> I'm surprised this works actually as the local reference to the jobject should
> have been lost when returning from RequestAnaswersImage.
> 
> Here's an pretty concise description of what these mean if you're morbidly
> interested:
>
http://developer.android.com/training/articles/perf-jni.html#local_and_global...

I _am_ morbidly interested. Thanks for the pointer. And done :)

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.cc:69: if (profile) {
On 2014/06/06 01:49:24, Ted C wrote:
> I would just dcheck profile here.  People shouldn't be calling these apis with
> bad profiles.  Same above.

Done.

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.cc:77: env, java_callback,
request_id).Release();
On 2014/06/06 01:49:23, Ted C wrote:
> indented too much

That's what 'git cl format' believes to be correct. If it deeply bothers you,
I'll reformat - but I prefer not fighting the automated formatter on each
check-in.

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
File chrome/browser/android/omnibox/answers_image_bridge.h (right):

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.h:17: explicit
AnswersImageBridge(Profile*);
On 2014/06/06 01:49:24, Ted C wrote:
> This header seems quite out of sync with the .cc file.  It looks like all of
> this can actually be removed except for the jni registration method.

Done.

[groby-ooo-7-16, 2014-06-17 18:09:04]

Ping?

[Ted C, 2014-06-17 20:38:15]

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java
(right):

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:49:
public static AnswersImageRequest requestAnswersImage(
On 2014/06/13 23:52:21, groby wrote:
> On 2014/06/06 01:49:23, Ted C wrote:
> > why not just pass back the request ID here?  You passed in the observer, so
it
> > doesn't really need to come back in the request.
> 
> It allows the Java side to hold a pointer to the observer. I'd like to avoid
the
> C++ side holding on to Java objects, and if the observer is an anonymous
class,
> somebody needs to hold on to it.

I don't agree with this.  I think it is entirely reasonable (and
consistent with how we do other c++ -> java callbacks) for C++ to
own a strong reference to this callback while it is pending a
response.

Personally, I think this API is actually more confusing and error
prone as it allows you to create an anonymous callback, but if you
didn't infer that you needed to hold onto the request return value
then it would be GC'd underneath.  And if you didn't care to ever
cancel your pending callback, then there is no need to actually
keep a reference.

I think changing native to hold a scoped strong global ref makes
this simpler on all sides of the api boundary.

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
File chrome/browser/android/omnibox/answers_image_bridge.cc (right):

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.cc:77: env, java_callback,
request_id).Release();
On 2014/06/13 23:52:21, groby wrote:
> On 2014/06/06 01:49:23, Ted C wrote:
> > indented too much
> 
> That's what 'git cl format' believes to be correct. If it deeply bothers you,
> I'll reformat - but I prefer not fighting the automated formatter on each
> check-in.

Nothing about this indenting looks sane to me.  If this is what clang-format is
doing, then this very much looks like a bug in the formatter.

I'd rather not let random indenting permeate the code, so I think you should
file this as a bug against clang format.

Bug link:
https://code.google.com/p/chromium/issues/entry?comment=clang-format%20produc...

https://codereview.chromium.org/312203005/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java
(right):

https://codereview.chromium.org/312203005/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:26:
public void onAnswersImageChanged(Bitmap bitmap);
-4 indent

https://codereview.chromium.org/312203005/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:67:
private static AnswersImageRequest createAnswersImageRequest(
This function needs to be -4 indented

https://codereview.chromium.org/312203005/diff/80001/chrome/browser/android/c...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/312203005/diff/80001/chrome/browser/android/c...
chrome/browser/android/chrome_jni_registrar.cc:103: { "AnswersImageBridge",
RegisterAnswersImageBridge },
order alphabetically

[groby-ooo-7-16, 2014-06-17 23:46:20]

Ownership re-arranged. PTAL.

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java
(right):

https://codereview.chromium.org/312203005/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:49:
public static AnswersImageRequest requestAnswersImage(
On 2014/06/17 20:38:15, Ted C wrote:
> On 2014/06/13 23:52:21, groby wrote:
> > On 2014/06/06 01:49:23, Ted C wrote:
> > > why not just pass back the request ID here?  You passed in the observer,
so
> it
> > > doesn't really need to come back in the request.
> > 
> > It allows the Java side to hold a pointer to the observer. I'd like to avoid
> the
> > C++ side holding on to Java objects, and if the observer is an anonymous
> class,
> > somebody needs to hold on to it.
> 
> I don't agree with this.  I think it is entirely reasonable (and
> consistent with how we do other c++ -> java callbacks) for C++ to
> own a strong reference to this callback while it is pending a
> response.
> 
> Personally, I think this API is actually more confusing and error
> prone as it allows you to create an anonymous callback, but if you
> didn't infer that you needed to hold onto the request return value
> then it would be GC'd underneath.  And if you didn't care to ever
> cancel your pending callback, then there is no need to actually
> keep a reference.
> 
> I think changing native to hold a scoped strong global ref makes
> this simpler on all sides of the api boundary.

So be it. We'll see what shakes out of the ownership tree ;)

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
File chrome/browser/android/omnibox/answers_image_bridge.cc (right):

https://codereview.chromium.org/312203005/diff/1/chrome/browser/android/omnib...
chrome/browser/android/omnibox/answers_image_bridge.cc:77: env, java_callback,
request_id).Release();
On 2014/06/17 20:38:15, Ted C wrote:
> On 2014/06/13 23:52:21, groby wrote:
> > On 2014/06/06 01:49:23, Ted C wrote:
> > > indented too much
> > 
> > That's what 'git cl format' believes to be correct. If it deeply bothers
you,
> > I'll reformat - but I prefer not fighting the automated formatter on each
> > check-in.
> 
> Nothing about this indenting looks sane to me.  If this is what clang-format
is
> doing, then this very much looks like a bug in the formatter.
> 
> I'd rather not let random indenting permeate the code, so I think you should
> file this as a bug against clang format.
> 
> Bug link:
>
https://code.google.com/p/chromium/issues/entry?comment=clang-format%20produc...


It _is_ somewhat sane in that the style guide doesn't specify what the +4
indentation for the arguments is relative to. I've filed crbug.com/385905, so
let's see how this plays out.

Thankfully, with just returning a requestId, this is not part of the CL any
more.

https://codereview.chromium.org/312203005/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java
(right):

https://codereview.chromium.org/312203005/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:26:
public void onAnswersImageChanged(Bitmap bitmap);
On 2014/06/17 20:38:15, Ted C wrote:
> -4 indent

Done.

https://codereview.chromium.org/312203005/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AnswersImage.java:67:
private static AnswersImageRequest createAnswersImageRequest(
On 2014/06/17 20:38:15, Ted C wrote:
> This function needs to be -4 indented

Function deleted, since the request class is unnecessary now

https://codereview.chromium.org/312203005/diff/80001/chrome/browser/android/c...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/312203005/diff/80001/chrome/browser/android/c...
chrome/browser/android/chrome_jni_registrar.cc:103: { "AnswersImageBridge",
RegisterAnswersImageBridge },
On 2014/06/17 20:38:15, Ted C wrote:
> order alphabetically

Done.

[Ted C, 2014-06-17 23:59:23]

lgtm

https://codereview.chromium.org/312203005/diff/100001/chrome/browser/android/...
File chrome/browser/android/omnibox/answers_image_bridge.cc (right):

https://codereview.chromium.org/312203005/diff/100001/chrome/browser/android/...
chrome/browser/android/omnibox/answers_image_bridge.cc:31:
java_answers_image_observer_.Reset(env, java_answers_image_observer);
shouldn't this be indented 2 instead of 4?

[groby-ooo-7-16, 2014-06-18 00:58:18]

Fixed. Waiting for upstream patch to land.

https://codereview.chromium.org/312203005/diff/100001/chrome/browser/android/...
File chrome/browser/android/omnibox/answers_image_bridge.cc (right):

https://codereview.chromium.org/312203005/diff/100001/chrome/browser/android/...
chrome/browser/android/omnibox/answers_image_bridge.cc:31:
java_answers_image_observer_.Reset(env, java_answers_image_observer);
On 2014/06/17 23:59:23, Ted C wrote:
> shouldn't this be indented 2 instead of 4?

And so it should. See what happens if you take away my formatting toys? ;)

[groby-ooo-7-16, 2014-06-19 14:30:37]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-06-19 14:32:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/groby@chromium.org/312203005/130001

[commit-bot: I haz the power, 2014-06-20 00:18:34]

Change committed as 278546