[Android] Add proxy for Java-based WebContentsObservers

[Android] Add proxy for Java-based WebContentsObservers

A good portion of the current WebContentsObserver instances for any
given tab are Java-based. Each of these require separate JNI hops, as
well as repeated string conversions, for each observer callback. Avoid
such costs by having just one Java-based WebContentsObserverProxy that
bridges from native and allows multiple Java-based WebContentsObserver
subscribers.

BUG=440134
Committed: https://crrev.com/c747fed0edbe60e29c6810314021210687577cd6
Cr-Commit-Position: refs/heads/master@{#319100}

[jdduke (slow), 2015-02-18 19:47:29]

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

[jdduke (slow), 2015-02-18 19:47:30]

tedchoc@: Any thoughts on this change? I've been sitting on it for a while,
debating if it's worth it... it may very well not be.

It does end up saving us a good number of JNI calls. By my count, we typically
have around 7-8 Java-based WebContentsObserver instances per tab. So that's (at
least) 6-7 saved JNI calls per listener method, which can come quite frequently
during page load (and it avoids multiple, redundant string conversions for a few
of the methods). My primary hesitation is the added complexity, though maybe
having a single JNI-entry point makes up for it...

https://codereview.chromium.org/786933005/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/BaseObserverList.java (right):

https://codereview.chromium.org/786933005/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/BaseObserverList.java:46: abstract
Wrapper wrap(E obs);
This structuring might use a little work... or renaming even.

[Ted C, 2015-02-18 22:54:37]

I'm all for introducing the proxy, but I think the weak observer thing is
overkill and probably too much.

Are they really that coupled or could we have one without the other?

https://codereview.chromium.org/786933005/diff/140001/content/browser/android...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/786933005/diff/140001/content/browser/android...
content/browser/android/web_contents_observer_android.cc:55: void
WebContentsObserverAndroid::WebContentsDestroyed() {
I would update the name of this to make it easier to sanely jump between java
and C++

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:21:
private final WeakObserverList<WebContentsObserver> mObservers;
I don't really like the weak ref-ness of this.  It makes it too easy to write
bad code.  I'd rather force everyone to actually clean up refs correctly.

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:31:
void addObserver(WebContentsObserver observer) {
javadocs :-)

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:209:
public void detachFromWebContents() {
I would just call this destroy.

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java
(right):

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:187:
public void detachFromWebContents() {
I'd love to see method removed and require everyone to call
webContents.removeObserver themselves...a person can dream.

[jdduke (slow), 2015-02-24 20:55:49]

https://codereview.chromium.org/786933005/diff/140001/content/browser/android...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/786933005/diff/140001/content/browser/android...
content/browser/android/web_contents_observer_android.cc:55: void
WebContentsObserverAndroid::WebContentsDestroyed() {
On 2015/02/18 22:54:36, Ted C wrote:
> I would update the name of this to make it easier to sanely jump between java
> and C++

Done.

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:21:
private final WeakObserverList<WebContentsObserver> mObservers;
On 2015/02/18 22:54:36, Ted C wrote:
> I don't really like the weak ref-ness of this.  It makes it too easy to write
> bad code.  I'd rather force everyone to actually clean up refs correctly.

Done.

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:31:
void addObserver(WebContentsObserver observer) {
On 2015/02/18 22:54:36, Ted C wrote:
> javadocs :-)

Done.

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:209:
public void detachFromWebContents() {
On 2015/02/18 22:54:37, Ted C wrote:
> I would just call this destroy.

Done.

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java
(right):

https://codereview.chromium.org/786933005/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:187:
public void detachFromWebContents() {
On 2015/02/18 22:54:37, Ted C wrote:
> I'd love to see method removed and require everyone to call
> webContents.removeObserver themselves...a person can dream.

Agreed. OK if I tackle that in a follow-up?

[Ted C, 2015-02-24 21:34:45]

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

[Ted C, 2015-02-24 21:34:46]

+yfriedman -- like to hear his thoughts on this

but this lgtm...any idea how many observers per tab we are collapsing with this
change?

https://codereview.chromium.org/786933005/diff/200001/content/browser/android...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/786933005/diff/200001/content/browser/android...
content/browser/android/web_contents_observer_android.cc:5: #include
"content/browser/android/web_contents_observer_android.h"
In my last round of comments, I mentioned changing the file name (meaning to
change it to web_contents_observer_proxy), but that didn't seem to happen or my
comment wasn't clear enough.

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:351:
;
spurious ;

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java
(left):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:210:
public void detachFromWebContents() {
Should we keep this around and call destroy to allow this to roll cleanly?

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:16:
private WeakReference<WebContents> mWebContents;
Is this a weak ref because of WebView?

[jdduke (slow), 2015-02-24 21:36:37]

On 2015/02/24 21:34:46, Ted C wrote:
> +yfriedman -- like to hear his thoughts on this
> 
> but this lgtm...any idea how many observers per tab we are collapsing with
this
> change?

I believe there are 9 observers currently (with Chrome...
ChromeShell/ContentShell have less, and WebView now only has 2).

[Yaron, 2015-02-24 21:43:34]

I haven't looked through the whole change (can do, if you'd like) but I like it
as a concept.

Relatedly, I was just implementing yet another TabObserver today, noticed that
ChromeTab actually has a TabObserver and I was wondering "how many of these do
we have for a given tab?". Anyway, I agree that it's wasteful to do the JNI
marshalling repeatedly here and it does seem worthwhile

[Yaron, 2015-02-25 16:39:33]

https://codereview.chromium.org/786933005/diff/200001/content/content_jni.gypi
File content/content_jni.gypi (right):

https://codereview.chromium.org/786933005/diff/200001/content/content_jni.gyp...
content/content_jni.gypi:42:
'public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java',
I assume you have a downstream patch ready for the move..

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:285:
// Using a weak reference avoids cycles that might prevent GC of WebView's
WebContents.
Why wasn't this necessary prevoiusly

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:228:
}
super.destroy?

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java
(left):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:210:
public void detachFromWebContents() {
On 2015/02/24 21:34:46, Ted C wrote:
> Should we keep this around and call destroy to allow this to roll cleanly?

It won't already cause of the package move.

[jdduke (slow), 2015-02-26 21:22:10]

https://codereview.chromium.org/786933005/diff/200001/content/browser/android...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/786933005/diff/200001/content/browser/android...
content/browser/android/web_contents_observer_android.cc:5: #include
"content/browser/android/web_contents_observer_android.h"
On 2015/02/24 21:34:46, Ted C wrote:
> In my last round of comments, I mentioned changing the file name (meaning to
> change it to web_contents_observer_proxy), but that didn't seem to happen or
my
> comment wasn't clear enough.

Oops, I thought you were referring to the *method* name (of destroy), not the
filename. Sounds reasonable.

https://codereview.chromium.org/786933005/diff/200001/content/content_jni.gypi
File content/content_jni.gypi (right):

https://codereview.chromium.org/786933005/diff/200001/content/content_jni.gyp...
content/content_jni.gypi:42:
'public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java',
On 2015/02/25 16:39:33, Yaron wrote:
> I assume you have a downstream patch ready for the move..

https://chrome-internal-review.googlesource.com/#/c/187677/

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:285:
// Using a weak reference avoids cycles that might prevent GC of WebView's
WebContents.
On 2015/02/25 16:39:33, Yaron wrote:
> Why wasn't this necessary prevoiusly

Previously, creating a WebContentsObserver instance didn't add an additional
strong ref anywhere. Only the creator held a strong ref, as the native
WebContentsObserverAndroid only holds a weak ref to its Java counterpart.

With this change, WebContentsObserverProxy holds a strong ref to all proxied
Java WebContentsObserver instances. A previous version of this patch did some
fanciness with ObserverList to allow it to support weak refs to observers, but
we scrapped that as it seemed like unnecessary complexity.

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:351:
;
On 2015/02/24 21:34:46, Ted C wrote:
> spurious ;

Done.

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/02/24 21:34:46, Ted C wrote:
> 2015

Done.

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:228:
}
On 2015/02/25 16:39:33, Yaron wrote:
> super.destroy?

This is one case where we explicitly don't want super destruction behavior. I'll
add a note.

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:16:
private WeakReference<WebContents> mWebContents;
On 2015/02/24 21:34:46, Ted C wrote:
> Is this a weak ref because of WebView?

Yes, I'll make a note.

[Yaron, 2015-02-27 19:34:57]

lgtm

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:285:
// Using a weak reference avoids cycles that might prevent GC of WebView's
WebContents.
On 2015/02/26 21:22:09, jdduke wrote:
> On 2015/02/25 16:39:33, Yaron wrote:
> > Why wasn't this necessary prevoiusly
> 
> Previously, creating a WebContentsObserver instance didn't add an additional
> strong ref anywhere. Only the creator held a strong ref, as the native
> WebContentsObserverAndroid only holds a weak ref to its Java counterpart.
> 
> With this change, WebContentsObserverProxy holds a strong ref to all proxied
> Java WebContentsObserver instances. A previous version of this patch did some
> fanciness with ObserverList to allow it to support weak refs to observers, but
> we scrapped that as it seemed like unnecessary complexity.

Sounds reasonable. I guess I'm a bit worried that this pattern could be repeated
and violated elsewhere but perhaps this is the only one that needs a reference
back to the CVC. Most get away with just the WebContents

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/786933005/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:228:
}
On 2015/02/26 21:22:10, jdduke wrote:
> On 2015/02/25 16:39:33, Yaron wrote:
> > super.destroy?
> 
> This is one case where we explicitly don't want super destruction behavior.
I'll
> add a note.

Ah right, thanks for clarifying. BTW I think the rename was a nice improvement
for code comprehension

https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:25: // TODO(dcheng): File
a bug. This class incorrectly passes just a frame ID,
Aside: "TODO: file a bug" is amusing

https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
content/browser/android/web_contents_observer_proxy.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
I think you're missing a gyp file change for the rename

[jdduke (slow), 2015-03-03 17:05:15]

jdduke@chromium.org changed reviewers:
+ boliu@chromium.org, gunsch@chromium.org

[jdduke (slow), 2015-03-03 17:05:17]

+boliu for android_webview/
+gunsch for chromecast/

https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:25: // TODO(dcheng): File
a bug. This class incorrectly passes just a frame ID,
On 2015/02/27 19:34:57, Yaron wrote:
> Aside: "TODO: file a bug" is amusing

Hey! Some bugs are tricky to file!

https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
content/browser/android/web_contents_observer_proxy.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/02/27 19:34:57, Yaron wrote:
> I think you're missing a gyp file change for the rename

Done.

[gunsch, 2015-03-03 17:08:56]

On 2015/03/03 17:05:17, jdduke wrote:
> +boliu for android_webview/
> +gunsch for chromecast/
> 
>
https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
> File content/browser/android/web_contents_observer_proxy.cc (right):
> 
>
https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
> content/browser/android/web_contents_observer_proxy.cc:25: // TODO(dcheng):
File
> a bug. This class incorrectly passes just a frame ID,
> On 2015/02/27 19:34:57, Yaron wrote:
> > Aside: "TODO: file a bug" is amusing
> 
> Hey! Some bugs are tricky to file!
> 
>
https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
> File content/browser/android/web_contents_observer_proxy.h (right):
> 
>
https://codereview.chromium.org/786933005/diff/220001/content/browser/android...
> content/browser/android/web_contents_observer_proxy.h:1: // Copyright 2015 The
> Chromium Authors. All rights reserved.
> On 2015/02/27 19:34:57, Yaron wrote:
> > I think you're missing a gyp file change for the rename
> 
> Done.

chromecast lgtm

[boliu, 2015-03-03 18:43:52]

android_webview rs lgtm

[jdduke (slow), 2015-03-04 17:16:24]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-03-04 17:16:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, yfriedman@chromium.org, boliu@chromium.org,
gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/786933005/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-04 17:17:14]

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

[commit-bot: I haz the power, 2015-03-04 17:27:01]

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

[commit-bot: I haz the power, 2015-03-04 17:27:02]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[jdduke (slow), 2015-03-04 17:27:51]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-03-04 17:28:56]

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

[jdduke (slow), 2015-03-04 19:11:46]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2015-03-04 19:12:08]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-03-04 19:13:04]

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

[commit-bot: I haz the power, 2015-03-04 19:39:03]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-03-04 19:39:40]

Patchset 14 (id:??) landed as
https://crrev.com/c747fed0edbe60e29c6810314021210687577cd6
Cr-Commit-Position: refs/heads/master@{#319100}