Add ContentViewDelegate

[Android] Add IO thread delegate for request intercepting.
    
This is to make it possible for a ResourceThrottle (which runs on
the IO thread) to call a WebViewClient method. This requires
caching the RenderViewHost<->WebContents relationship in
    a threadsafe way.

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

[mkosiba (inactive), 2012-07-03 17:12:16]

Hey!
   Very early work in progress on adding a way to access stuff that currently
lives in ContentViewClient from the chrome/ layer.
   The example usage would be that Tab.java creates the ContentViewDelegate
passing in the nativeWebContents. The anonymous class in Tab.java that currently
extends ContentViewClient would also implement the new Delegate interface.
   
   Please take a look. As this is a very early change no nit: comments (unless
you really really have to) :P
   I'm a bit concerned about clashing with other upstreamers with regard to the
chrome/ build rules - any of you know what's the scoop on that?

Cheers!
    Martin

[Yaron, 2012-07-10 06:34:55]

+Shashi who's adding the ant xml for chrome/ downstream.

I just took a quick-pass. I don't see this downstream, am I missing something?
Unfortunately, we can't link chrome upstream yet (still sorting out some link
errors) so I'm hesitant to land chrome/ code upstream since it can very easily
rot.

https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/OWNERS
File chrome/android/OWNERS (right):

https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/OWNE...
chrome/android/OWNERS:1: satish@chromium.org
Seems like you can just keep chrome/android/java/OWNERS and drop this file

https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/java...
File chrome/android/java/chrome.xml (right):

https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/java...
chrome/android/java/chrome.xml:1: <!-- Copyright (c) 2012 The Chromium Authors.
All rights reserved.
Does anything depend on this yet? You're just using this to create the
chromium_chrome.jar, right?

https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/java...
chrome/android/java/chrome.xml:43: <pathelement
path="${jar.dir}/chromium_base.jar" />
This'll likely need chromium_net and chromium_media

https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/browser/andr...
File chrome/browser/android/content_view_delegate.h (right):

https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/browser/andr...
chrome/browser/android/content_view_delegate.h:44: class ContentViewDelegate {
Is this pulling out functionality from ContentViewClient? I dont' see it
downstrema

[Yaron, 2012-07-10 06:37:17]

On 2012/07/10 06:34:55, Yaron wrote:
> +Shashi who's adding the ant xml for chrome/ downstream.
> 
> I just took a quick-pass. I don't see this downstream, am I missing something?
> Unfortunately, we can't link chrome upstream yet (still sorting out some link
> errors) so I'm hesitant to land chrome/ code upstream since it can very easily
> rot.
> 
>
https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/OWNERS
> File chrome/android/OWNERS (right):
> 
>
https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/OWNE...
> chrome/android/OWNERS:1: mailto:satish@chromium.org
> Seems like you can just keep chrome/android/java/OWNERS and drop this file
> 
>
https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/java...
> File chrome/android/java/chrome.xml (right):
> 
>
https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/java...
> chrome/android/java/chrome.xml:1: <!-- Copyright (c) 2012 The Chromium
Authors.
> All rights reserved.
> Does anything depend on this yet? You're just using this to create the
> chromium_chrome.jar, right?
> 
>
https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/android/java...
> chrome/android/java/chrome.xml:43: <pathelement
> path="${jar.dir}/chromium_base.jar" />
> This'll likely need chromium_net and chromium_media
> 
>
https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/browser/andr...
> File chrome/browser/android/content_view_delegate.h (right):
> 
>
https://chromiumcodereview.appspot.com/10702083/diff/1023/chrome/browser/andr...
> chrome/browser/android/content_view_delegate.h:44: class ContentViewDelegate {
> Is this pulling out functionality from ContentViewClient? I dont' see it
> downstrema

Ahh, I missed your first comment. Sounds like we should make sure this works
downstream first (setting up the sub-class and everything for tab's
inner-class).

[shashi, 2012-07-12 22:21:07]

Martin, FYI we are re-factoring some ant build files downstream, chrome.xml will
become much simpler.

[mkosiba (inactive), 2012-08-31 17:05:13]

tests and rest of code is coming soon, sending this out to get initial feedback
on the approach taken

[joth, 2012-09-01 01:24:13]

sorry, almost 2 months since you sent this and I never commented :-(

Still, now it's factored into android_webview, I understand it much better :-)

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.cc (right):

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:44: } // namespace
BASE_HASH_NAMESPACE
IIRC we specifically don't use hash_tables on android for some reason.

anyway, should be fine to just use map<> to avoid need for this spew.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:49: typedef
linked_ptr<JavaObjectWeakGlobalRef> LinkedJavaWeakRef;
we could give JavaObjectWeakGlobalRef a copy c'tor if needed.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:49: typedef
linked_ptr<JavaObjectWeakGlobalRef> LinkedJavaWeakRef;
we could give JavaObjectWeakGlobalRef a copy c'tor and avoid need for a link_ptr
to it

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:57:
WebContentsToWeakClientMapType g_web_contents_to_weak_delegate_map;
LazyInstance...

and will need comments about why we can't use SupportsUserData.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:93:
g_rvh_to_web_contents_map[rvh] = web_contents();
if you hold the weak ref as a member of this WebContentsObserver, then you can
just bosh it straight into a map of RVH -> scoped-jweak here, skip the double
lookup? 

assuming the caller's use-case can always go from RVH that is...

question: could we make it a map of pair<render_process_id, render_view_id> ->
jweak ?

we just use ResourceRequestInfo::GetRenderViewForRequest(url_request) to fetch
those numbers for a given url_request, and then we're there.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.h (right):

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:38: // you're holding on
to it. This could happen if the WebContents is destroyed.
if the WC is destroyed, the  AwWebContentsIoThreadDelegate instance will still
be bound to the java side class and will still call through to it?

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:47:
ForRenderViewHost(content::RenderViewHost* rvh);
returning scoped_ptr (returning ownership) makes it sound like this should be a
CreateFoo() type method.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:53:
ForWebContents(content::WebContents* contents);
Can't quite follow the usage model for these without seeing the code that uses
them. Questions in my mind are:

- Do we actually need both versions? I'd have thought we always lookup via just
RVH or web contents
- is it really new-ing up a new native object instance on every request? i guess
there's no problem in that, but seems unusual.
- what thread(s) will each these be called on?

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:63: class
WebContentsObserver;
nit: call this something about its job, JavaDelegateMapMaintainer or something.
(it could be an anon class in the .cc I think)

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:65:
DISALLOW_COPY_AND_ASSIGN(AwWebContentsIoThreadDelegate);
should be last.

the WCO forward declaration isn't needed?

d'tor could also be private.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/int...
File android_webview/native/intercepted_request_data.h (right):

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/int...
android_webview/native/intercepted_request_data.h:13: class
InterceptedRequestData {
class comment

[boliu, 2012-09-06 00:46:23]

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.cc (right):

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:56:
RenderViewHostToWebContentsMapType g_rvh_to_web_contents_map;
WebContents::FromRenderViewHost can't be used here instead because it is not
thread-safe, right?

[mkosiba (inactive), 2012-09-11 01:22:24]

new patch out

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.cc (right):

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:44: } // namespace
BASE_HASH_NAMESPACE
On 2012/09/01 01:24:14, joth wrote:
> IIRC we specifically don't use hash_tables on android for some reason.
> 
> anyway, should be fine to just use map<> to avoid need for this spew.
> 

that's sad.. a map<> is usually some form of a balanced tree and turns out
rather inefficient compared to hash (especially for small sizes of the map). not
that we expect this code to be hit too often..

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:49: typedef
linked_ptr<JavaObjectWeakGlobalRef> LinkedJavaWeakRef;
On 2012/09/01 01:24:14, joth wrote:
> we could give JavaObjectWeakGlobalRef a copy c'tor and avoid need for a
link_ptr
> to it

Done.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:56:
RenderViewHostToWebContentsMapType g_rvh_to_web_contents_map;
On 2012/09/06 00:46:23, boliu wrote:
> WebContents::FromRenderViewHost can't be used here instead because it is not
> thread-safe, right?

yes, the problem is that while you can get a WebContents from a RenderViewHost,
the RenderViewHost object can only be accessed from the UI thread.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:93:
g_rvh_to_web_contents_map[rvh] = web_contents();
On 2012/09/01 01:24:14, joth wrote:
> if you hold the weak ref as a member of this WebContentsObserver, then you can
> just bosh it straight into a map of RVH -> scoped-jweak here, skip the double
> lookup? 
> 
> assuming the caller's use-case can always go from RVH that is...
> 
> question: could we make it a map of pair<render_process_id, render_view_id> ->
> jweak ?

> we just use ResourceRequestInfo::GetRenderViewForRequest(url_request) to fetch
> those numbers for a given url_request, and then we're there.

I had it work like that in one of the initial iterations, can't remember why I
changed that though... Let's see if I can change it back.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.h (right):

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:38: // you're holding on
to it. This could happen if the WebContents is destroyed.
On 2012/09/01 01:24:14, joth wrote:
> if the WC is destroyed, the  AwWebContentsIoThreadDelegate instance will still
> be bound to the java side class and will still call through to it? 

the Java side class will exist, but we only make calls from native to Java and
not the other way around.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:47:
ForRenderViewHost(content::RenderViewHost* rvh);
On 2012/09/01 01:24:14, joth wrote:
> returning scoped_ptr (returning ownership) makes it sound like this should be
a
> CreateFoo() type method.

it's a bit uncommon but the intent behind this is that we're not really creating
a new object, we're only new-ing up the native JNI binding to it.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:53:
ForWebContents(content::WebContents* contents);
On 2012/09/01 01:24:14, joth wrote:
> Can't quite follow the usage model for these without seeing the code that uses
> them. Questions in my mind are:
> 
> - Do we actually need both versions? I'd have thought we always lookup via
just
> RVH or web contents

yeath.. I'll clean this up. We should be getting the Delegate via a (rvh_id,
rview_id) pair.

> - is it really new-ing up a new native object instance on every request? i
guess

yes, this is because the native object "pins" the java-side object.

> there's no problem in that, but seems unusual.
> - what thread(s) will each these be called on?

should be possible to call this from any thread.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:63: class
WebContentsObserver;
On 2012/09/01 01:24:14, joth wrote:
> nit: call this something about its job, JavaDelegateMapMaintainer or
something.
> (it could be an anon class in the .cc I think)

Renamed, has to be inner/friend in order to be able to use the private c'tor.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:65:
DISALLOW_COPY_AND_ASSIGN(AwWebContentsIoThreadDelegate);
On 2012/09/01 01:24:14, joth wrote:
> should be last.
Done

> the WCO forward declaration isn't needed?

it's needed, otherwise the WCO won't be able to instantiate new AWCIOTD's. I
moved the thing to be a friend class to keep the method declarations in the .cc
file shorter.

> d'tor could also be private.

sadly scoped_ptr insists on calling the destructor so I'll need to keep it
public.

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/int...
File android_webview/native/intercepted_request_data.h (right):

http://codereview.chromium.org/10702083/diff/10001/android_webview/native/int...
android_webview/native/intercepted_request_data.h:13: class
InterceptedRequestData {
On 2012/09/01 01:24:14, joth wrote:
> class comment

Done.

[joth, 2012-09-11 02:06:49]

http://codereview.chromium.org/10702083/diff/16002/android_webview/java/src/o...
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadDelegate.java
(right):

http://codereview.chromium.org/10702083/diff/16002/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadDelegate.java:19:
public interface AwContentsIoThreadDelegateImpl {
Seems funky to have an "interface" that carries the Impl suffix.

Seems most of this class is really an internal implementation detail of
AwContents, and it's this interface that's the interesting public API.

It's really some sort of "AwContentsIoThreadClient"

We could make the flow for registering and removing it follow the same pattern
as AwContentsClient? i.e. setter on the AwContents, rather than force java user
to have to grab nativeWebContents out of  one place an pass to another.

http://codereview.chromium.org/10702083/diff/16002/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadDelegate.java:22:
private AtomicReference<AwContentsIoThreadDelegateImpl> mImpl =
FWIW: all Java references are atomic for assign and reading, so you can avoid
the need for the wrapper AtomicReference. Just make it volatile to ensure memory
barriers. (unlike c++)



defo. need comments around the usage then though.

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.cc (right):

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:39:
RenderViewHostToWeakDelegateMapType g_rvh_to_weak_delegate_map;
these still need to be lazy instance-ified.

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:67: jdelegate_(env,
jdelegate) {
I think we can remove jdelegate_ member, and then just use the
AwWebContentsIoThreadDelegate::FromID in the destructor to call the onDestroyed
method.

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:75:
g_rvh_to_weak_delegate_map[rvh_id] = jdelegate_;
not sure why I didn't suggest this before, but rather than lock the map, would
we be able to post a task to IO thread to do the registrations and
unregistrations?

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:106:
io_thread_delegate.OnWebContentsDestroyed();
note this is calling the "IO" thread delegate on the UI thread. Maybe append
OnUiThread just to make this clear?

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:148: new
AwWebContentsIoThreadDelegate(java_delegate));
return make_scoped_ptr(new Foo);

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.h (right):

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:41: class
AwWebContentsIoThreadDelegate {
OK getting my head around this now.

I think future readers will be confused that AwWebContentsIoThreadDelegate java
& native parts have such different lifetimes. (and that one java instance will
be referenced by thousands of native instances over its lifetime). Can we think
of a different name for the native object to reflect this? FooHandle or
FooReference (or if nothing else FooProxy or FooAccessor) might be a starting
point.

[mkosiba (inactive), 2012-09-13 01:48:25]

new patch out

http://codereview.chromium.org/10702083/diff/16002/android_webview/java/src/o...
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadDelegate.java
(right):

http://codereview.chromium.org/10702083/diff/16002/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadDelegate.java:19:
public interface AwContentsIoThreadDelegateImpl {
On 2012/09/11 02:06:50, joth wrote:
> Seems funky to have an "interface" that carries the Impl suffix.
> 
> Seems most of this class is really an internal implementation detail of
> AwContents, and it's this interface that's the interesting public API.
> 
> It's really some sort of "AwContentsIoThreadClient"
> 
> We could make the flow for registering and removing it follow the same pattern
> as AwContentsClient? i.e. setter on the AwContents, rather than force java
user
> to have to grab nativeWebContents out of  one place an pass to another.

Done. Removed inner interface, added setter to AwContents

http://codereview.chromium.org/10702083/diff/16002/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadDelegate.java:22:
private AtomicReference<AwContentsIoThreadDelegateImpl> mImpl =
On 2012/09/11 02:06:50, joth wrote:
> FWIW: all Java references are atomic for assign and reading, so you can avoid
> the need for the wrapper AtomicReference. Just make it volatile to ensure
memory
> barriers. (unlike c++)
> 
> 
> 
> defo. need comments around the usage then though.

this is removed.

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.cc (right):

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:39:
RenderViewHostToWeakDelegateMapType g_rvh_to_weak_delegate_map;
On 2012/09/11 02:06:50, joth wrote:
> these still need to be lazy instance-ified.

Done.

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:67: jdelegate_(env,
jdelegate) {
On 2012/09/11 02:06:50, joth wrote:
> I think we can remove jdelegate_ member, and then just use the
> AwWebContentsIoThreadDelegate::FromID in the destructor to call the
onDestroyed
> method.

changed the code to not do this anymore, but in this approach the jdelegate_
memeber is needed in case the RVH is destroyed before the WebContents is.

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:75:
g_rvh_to_weak_delegate_map[rvh_id] = jdelegate_;
On 2012/09/11 02:06:50, joth wrote:
> not sure why I didn't suggest this before, but rather than lock the map, would
> we be able to post a task to IO thread to do the registrations and
> unregistrations?

IMHO the lock-based approach is simpler to read.

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:106:
io_thread_delegate.OnWebContentsDestroyed();
On 2012/09/11 02:06:50, joth wrote:
> note this is calling the "IO" thread delegate on the UI thread. Maybe append
> OnUiThread just to make this clear?

method removed

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.cc:148: new
AwWebContentsIoThreadDelegate(java_delegate));
On 2012/09/11 02:06:50, joth wrote:
> return make_scoped_ptr(new Foo);

Done.

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_delegate.h (right):

http://codereview.chromium.org/10702083/diff/16002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_delegate.h:41: class
AwWebContentsIoThreadDelegate {
On 2012/09/11 02:06:50, joth wrote:
> OK getting my head around this now.
> 
> I think future readers will be confused that AwWebContentsIoThreadDelegate
java
> & native parts have such different lifetimes. (and that one java instance will
> be referenced by thousands of native instances over its lifetime). Can we
think
> of a different name for the native object to reflect this? FooHandle or
> FooReference (or if nothing else FooProxy or FooAccessor) might be a starting
> point.

added some comments

[joth, 2012-09-13 20:09:12]

lgtm, this form is much easier to read. Thanks!

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
File android_webview/native/aw_contents.cc (right):

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents.cc:108: void
AwContents::SetIoThreadClient(JNIEnv* env, jobject obj, jobject client) {
DCHECK(CurrentlyOn(UIThread))

--seems it would be easy to misread the intent of this method.

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_client.cc (right):

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.cc:44: base::Lock
g_map_lock;
you still have a global in here!

Suggest wrapping the lock and the map into a single struct {} and have that as a
lazy instance. Maybe nice to even put methods that use the lock onto that class,
then you can keep the lock itself private.

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.cc:52: //
JavaClientMapMaintainer ----------------------------------------------------
nit: FooMaintainer sounds like something I'd expect there to be one of per Foo
instance. (So here I expect one per Map, i.e. one)

How about ClientMapEntryUpdater (making it clear it's per Entry, not per Map)

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.cc:70: JNIEnv* env,
WebContents* web_contents, jobject jdelegate)
one param per line. (and in the class declaration)

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.cc:110: // JNI Methods
----------------------------------------------------------------
nit: If you're splitting the file by class name I don't think it needs ascii art
for the method types within it too.

(static methods we tend to do "// static" prefix per method)


nit: make the ascii art ---- line up on right margin throughout the file.

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_client.h (right):

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.h:28: // is made for.
nit: maybe generalize the comment to describe this as the bridge for chromium
native -> app java callbacks that need to be made on the IO thread.
Specifically, this is used to implement URLRequest..... etc etc.

(nitty nit: upstream prefer to avoid the use of "us" in comments, so if you can
remove that too then everyone should love you)

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.cc
File base/android/jni_helper.cc (right):

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.cc...
base/android/jni_helper.cc:59: base::android::ScopedJavaLocalRef<jobject>
pinnedObj = other.get(env);
pinned_obj

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.cc...
base/android/jni_helper.cc:63: obj_ = env->NewWeakGlobalRef(pinnedObj.obj());
I don't believe you need pinned_obj at all. JNI specifies that both
env->NewWeakGlobalRef(NULL) and env->NewWeakGlobalRef(dead_weak_jobject) will do
the right thing.

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.h
File base/android/jni_helper.h (right):

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.h#...
base/android/jni_helper.h:30: void assign(const JavaObjectWeakGlobalRef& rhs);
nit: Assign

[mkosiba (inactive), 2012-09-17 17:48:22]

new version (addressed feedback + rebase) is up. PTAL

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
File android_webview/native/aw_contents.cc (right):

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents.cc:108: void
AwContents::SetIoThreadClient(JNIEnv* env, jobject obj, jobject client) {
On 2012/09/13 20:09:12, joth wrote:
> DCHECK(CurrentlyOn(UIThread))
> 
> --seems it would be easy to misread the intent of this method.

Done.

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_client.cc (right):

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.cc:44: base::Lock
g_map_lock;
On 2012/09/13 20:09:12, joth wrote:
> you still have a global in here!
> 
> Suggest wrapping the lock and the map into a single struct {} and have that as
a
> lazy instance. Maybe nice to even put methods that use the lock onto that
class,
> then you can keep the lock itself private.

Done.

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.cc:52: //
JavaClientMapMaintainer ----------------------------------------------------
On 2012/09/13 20:09:12, joth wrote:
> nit: FooMaintainer sounds like something I'd expect there to be one of per Foo
> instance. (So here I expect one per Map, i.e. one)
> 
> How about ClientMapEntryUpdater (making it clear it's per Entry, not per Map)

Done.

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.cc:70: JNIEnv* env,
WebContents* web_contents, jobject jdelegate)
On 2012/09/13 20:09:12, joth wrote:
> one param per line. (and in the class declaration)

Done.

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.cc:110: // JNI Methods
----------------------------------------------------------------
On 2012/09/13 20:09:12, joth wrote:
> nit: If you're splitting the file by class name I don't think it needs ascii
art
> for the method types within it too.
> 
> (static methods we tend to do "// static" prefix per method)
> 
> 
> nit: make the ascii art ---- line up on right margin throughout the file.

Done.

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
File android_webview/native/aw_contents_io_thread_client.h (right):

http://codereview.chromium.org/10702083/diff/15002/android_webview/native/aw_...
android_webview/native/aw_contents_io_thread_client.h:28: // is made for.
On 2012/09/13 20:09:12, joth wrote:
> nit: maybe generalize the comment to describe this as the bridge for chromium
> native -> app java callbacks that need to be made on the IO thread.
> Specifically, this is used to implement URLRequest..... etc etc.
> 
> (nitty nit: upstream prefer to avoid the use of "us" in comments, so if you
can
> remove that too then everyone should love you)

everybody would love me (puppy eyes), yes!

[mkosiba (inactive), 2012-09-17 17:55:31]

ah.. missed these, now fixed.

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.cc
File base/android/jni_helper.cc (right):

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.cc...
base/android/jni_helper.cc:59: base::android::ScopedJavaLocalRef<jobject>
pinnedObj = other.get(env);
On 2012/09/13 20:09:12, joth wrote:
> pinned_obj

Done.

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.cc...
base/android/jni_helper.cc:63: obj_ = env->NewWeakGlobalRef(pinnedObj.obj());
On 2012/09/13 20:09:12, joth wrote:
> I don't believe you need pinned_obj at all. JNI specifies that both
> env->NewWeakGlobalRef(NULL) and env->NewWeakGlobalRef(dead_weak_jobject) will
do
> the right thing.

Done. Where did you find the docs?
http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html...
says nothing about the object being already dead.

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.h
File base/android/jni_helper.h (right):

http://codereview.chromium.org/10702083/diff/15002/base/android/jni_helper.h#...
base/android/jni_helper.h:30: void assign(const JavaObjectWeakGlobalRef& rhs);
On 2012/09/13 20:09:12, joth wrote:
> nit: Assign

Done.

[commit-bot: I haz the power, 2012-09-18 08:48:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10702083/27001

[commit-bot: I haz the power, 2012-09-18 08:53:46]

Retried try job too often for step(s) crypto_unittests, unit_tests,
cacheinvalidation_unittests, remoting_unittests, jingle_unittests,
nacl_integration, ipc_tests, interactive_ui_tests, browser_tests, net_unittests,
content_unittests, gpu_unittests, base_unittests, sync_integration_tests,
media_unittests, safe_browsing_tests, content_browsertests, sql_unittests,
printing_unittests, sync_unit_tests, check_deps

[commit-bot: I haz the power, 2012-09-18 11:00:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10702083/35001