[Android] Refactor NativeView to be able to use it for AutofillDialog.

[Android] Refactor NativeView to be able to use it for AutofillDialog.

Create a new class WindowAndroid that serves the purpose of representing a native (platform specific) view where Chromium code expects to have a cross platform handle to the system view type. As Views are Java objects on Android, ViewAndroid.java and view_android.* will provide the expected abstractions on the C++ side and allow it to be flexibly glued to an actual Android Java View at runtime. It should only be used where access to Android Views is needed from the C++ code as there are easier/better ways to access Views from Java.

This new abstraction will allow to reuse AutofillPopup for regular use in a webpage (using ContentView) as well as for AutofillDialog.

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

[aruslan, 2013-04-12 16:53:35]

https://chromiumcodereview.appspot.com/14018004/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/14018004/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:641:
}
Here -- destroy/notify the native counterpart.

https://chromiumcodereview.appspot.com/14018004/diff/1/ui/android/view_android.h
File ui/android/view_android.h (right):

https://chromiumcodereview.appspot.com/14018004/diff/1/ui/android/view_androi...
ui/android/view_android.h:21: void Destroy(JNIEnv* env, jobject obj);
This should be called strictly from the Java side by the owner of the object.
(*env)->IsSameObject(env, obj1, obj2).

[aurimas (slooooooooow), 2013-04-12 18:00:04]

Hey Jonathan,

Could you take a look at this CL? It might have implications on WebView, if so
please let me know so I can sort it out.

Aurimas

https://codereview.chromium.org/14018004/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14018004/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:641:
}
On 2013/04/12 16:53:35, aruslan wrote:
> Here -- destroy/notify the native counterpart.

Done.

https://codereview.chromium.org/14018004/diff/1/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/14018004/diff/1/ui/android/view_android.h#new...
ui/android/view_android.h:21: void Destroy(JNIEnv* env, jobject obj);
On 2013/04/12 16:53:35, aruslan wrote:
> This should be called strictly from the Java side by the owner of the object.
> (*env)->IsSameObject(env, obj1, obj2).

Done.

[aurimas (slooooooooow), 2013-04-12 18:46:52]

Hey Nilesh,

Please take a look at this CL.

Aurimas

[joth, 2013-04-12 19:00:48]

+sgurun FYI -- he's currently wiring up autofill in aw/

I only read content/public on. This seems a good separation of concerns in
general, just some complications about object lifetimes across JNI (nothing new
there)

https://codereview.chromium.org/14018004/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14018004/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:487:
mNativeView = new NativeView(nativeWindow, getContainerViewDelegate());
not sure we can doing anything about it, but the word 'native' here had my head
spinning a bit. Ultimately this reflects the cross-plat chromium "NativeView"
type def, so native means 'system's built in widget system' but in our java code
native normally means "c++"

hmmmm. In c++ NativeView is type'defed to ViewAndroid. How about we name both
the c++ and java class ViewAndroid? And WindowAndroid, and ViewAndroidDelegate,
etc...
This way it's obvious here that this is a 'chromium' concept, as real android
view's (and classes in general) don't have the word "Android" in their names :)

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ContainerViewDelegate.java (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ContainerViewDelegate.java:12: public
interface ContainerViewDelegate {
This may make more sense as NativeViewDelegate now? (could be an inner interface
on  NativeView; don't mind)

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/NativeView.java (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2012 is over (multiple places)

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:13: * used to attach
decorations like Autofill popup.
maybe useful to also document this guys purpose from the chromium architecture
point of view: chrome expects to have a cross platform 'handle' to a system view
type; as views are java objects on android, this class exists to provide the
expected abstractions on the C++ side and allow it to be flexibly glued to a
java view at runtime. (Handwave/)

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:34: public void destroy() {
in webview we need to allow ContentViewCore to cleanup nicely when GC'd. We use
CleanupReference to achieve that ... I think we'll need one on this guy too.

oh.. and you'll need to move CleanupReference to base. (I was actually hoping to
move it up to android_webview, as right now our one and only finalizer is on
AwContents and we rely on WebContents::UserData to delete all the
similar-lifetimed native objects when the owning AwContents (and hence
ContentViewCore) java objects have died.
... unless we can arrange for this not to be a native-object-owning java class,
and in the same pattern, lean on some existing appropriate life-timed native
object to do the cleanup? I'm not fully clear on the use cases to know if that's
reasonable

adding cleanup ref makes life a bit more complicated as you need to deal with
the java object going away but the native object may not yet know (the finalizer
can get delayed indefinitely). Also, need to be confident that when the native
object is finally deleted (via explicit destroy() or by finalizer) we don't have
any dangling pointers to that NativeView still lurking in other native objects.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:21: {
on prev line

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:25: // This object can only be destroyed by it's
native counterpart.
this smells like a programming error and should be a DCHECK?

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:26: if
(env->IsSameObject(weak_java_view_.get(env).obj(), obj))
in the window between GC and finalization, this means passing null for |obj|
would satisfy this...

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:16: // This class deals with the Android native window
ref count.
i"m not sure what ref count that is?

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:17: class ViewAndroid {
Can we get the java and native class names consistent, given they are identical
lifetimes?

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:19: ViewAndroid(JNIEnv* env, jobject obj,
WindowAndroid* window);
what's the lifetime relationship of |window| ?  it's safe to just hold a weak
pointer to it?

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:25: base::android::ScopedJavaLocalRef<jobject>
GetJavaObject();
note - will return NULL if the java side was GCed (but the native side not yet
finalized)

[nilesh, 2013-04-12 19:07:09]

Few comments.

https://codereview.chromium.org/14018004/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14018004/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:487:
mNativeView = new NativeView(nativeWindow, getContainerViewDelegate());
+1

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ContainerViewDelegate.java (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ContainerViewDelegate.java:18: public void
addViewToContainerView(View view);
Drop public from interface methods.

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/NativeView.java (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:20: public final
ContainerViewDelegate mContainerViewDelegate;
make it private final and add a getter if needed.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:5: #include "view_android.h"
ui/android/vew_android.h
Does this compile?

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:21: {
{} on line 20

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:25: // This object can only be destroyed by it's
native counterpart.
which object?

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:31: JNIEnv* env = AttachCurrentThread();
no need of variable env

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:47: // Native JNI methods
remove this comment block. I dont think it is helpful.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
2013

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:5: #ifndef UI_VIEW_ANDROID_H_
UI_ANDROID_VIEW_ANDROID_H

[aurimas (slooooooooow), 2013-04-12 22:00:40]

joth: please take a look at my non-done responses below.

https://codereview.chromium.org/14018004/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14018004/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:487:
mNativeView = new NativeView(nativeWindow, getContainerViewDelegate());
On 2013/04/12 19:00:48, joth wrote:
> not sure we can doing anything about it, but the word 'native' here had my
head
> spinning a bit. Ultimately this reflects the cross-plat chromium "NativeView"
> type def, so native means 'system's built in widget system' but in our java
code
> native normally means "c++"
> 
> hmmmm. In c++ NativeView is type'defed to ViewAndroid. How about we name both
> the c++ and java class ViewAndroid? And WindowAndroid, and
ViewAndroidDelegate,
> etc...
> This way it's obvious here that this is a 'chromium' concept, as real android
> view's (and classes in general) don't have the word "Android" in their names
:)

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/ContainerViewDelegate.java (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ContainerViewDelegate.java:12: public
interface ContainerViewDelegate {
On 2013/04/12 19:00:48, joth wrote:
> This may make more sense as NativeViewDelegate now? (could be an inner
interface
> on  NativeView; don't mind)

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/ContainerViewDelegate.java:18: public void
addViewToContainerView(View view);
On 2013/04/12 19:07:10, nilesh wrote:
> Drop public from interface methods.

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/NativeView.java (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/04/12 19:07:10, nilesh wrote:
> 2013

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:13: * used to attach
decorations like Autofill popup.
On 2013/04/12 19:00:48, joth wrote:
> maybe useful to also document this guys purpose from the chromium architecture
> point of view: chrome expects to have a cross platform 'handle' to a system
view
> type; as views are java objects on android, this class exists to provide the
> expected abstractions on the C++ side and allow it to be flexibly glued to a
> java view at runtime. (Handwave/)

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:20: public final
ContainerViewDelegate mContainerViewDelegate;
On 2013/04/12 19:07:10, nilesh wrote:
> make it private final and add a getter if needed.

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:34: public void destroy() {
On 2013/04/12 19:00:48, joth wrote:
> in webview we need to allow ContentViewCore to cleanup nicely when GC'd. We
use
> CleanupReference to achieve that ... I think we'll need one on this guy too.
> 
> oh.. and you'll need to move CleanupReference to base. (I was actually hoping
to
> move it up to android_webview, as right now our one and only finalizer is on
> AwContents and we rely on WebContents::UserData to delete all the
> similar-lifetimed native objects when the owning AwContents (and hence
> ContentViewCore) java objects have died.
> ... unless we can arrange for this not to be a native-object-owning java
class,
> and in the same pattern, lean on some existing appropriate life-timed native
> object to do the cleanup? I'm not fully clear on the use cases to know if
that's
> reasonable
> 
> adding cleanup ref makes life a bit more complicated as you need to deal with
> the java object going away but the native object may not yet know (the
finalizer
> can get delayed indefinitely). Also, need to be confident that when the native
> object is finally deleted (via explicit destroy() or by finalizer) we don't
have
> any dangling pointers to that NativeView still lurking in other native
objects.
> 
> 

So I was hoping that ContentViewCore.java is the owner ViewAndroid.java.

ViewAndroid.java is the owner view_android.cc object.

I though when ContentViewCore.java goes away, it would destroy ViewAndroid.java
and that in turn would kill view_android.cc. I was hoping that the
view_android.cc would not be destroyed in any other way.

So I guess that is an illusion?

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:5: #include "view_android.h"
On 2013/04/12 19:07:10, nilesh wrote:
> ui/android/vew_android.h
> Does this compile?

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:21: {
On 2013/04/12 19:07:10, nilesh wrote:
> {} on line 20

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:25: // This object can only be destroyed by it's
native counterpart.
On 2013/04/12 19:07:10, nilesh wrote:
> which object?

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:26: if
(env->IsSameObject(weak_java_view_.get(env).obj(), obj))
On 2013/04/12 19:00:48, joth wrote:
> in the window between GC and finalization, this means passing null for |obj|
> would satisfy this...

Should I check if obj is !null too?

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:31: JNIEnv* env = AttachCurrentThread();
On 2013/04/12 19:07:10, nilesh wrote:
> no need of variable env

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:47: // Native JNI methods
On 2013/04/12 19:07:10, nilesh wrote:
> remove this comment block. I dont think it is helpful.

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2013/04/12 19:07:10, nilesh wrote:
> 2013

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:5: #ifndef UI_VIEW_ANDROID_H_
On 2013/04/12 19:07:10, nilesh wrote:
> UI_ANDROID_VIEW_ANDROID_H

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:16: // This class deals with the Android native window
ref count.
On 2013/04/12 19:00:48, joth wrote:
> i"m not sure what ref count that is?

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:17: class ViewAndroid {
On 2013/04/12 19:00:48, joth wrote:
> Can we get the java and native class names consistent, given they are
identical
> lifetimes?

Done.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:19: ViewAndroid(JNIEnv* env, jobject obj,
WindowAndroid* window);
On 2013/04/12 19:00:48, joth wrote:
> what's the lifetime relationship of |window| ?  it's safe to just hold a weak
> pointer to it?

WindowAndroid has a life time of the application. It gets created by Main.java
and get removed once application is closed.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:25: base::android::ScopedJavaLocalRef<jobject>
GetJavaObject();
On 2013/04/12 19:00:48, joth wrote:
> note - will return NULL if the java side was GCed (but the native side not yet
> finalized)

How can I fix this? Add null checks?

[joth, 2013-04-16 04:02:42]

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
File ui/android/java/src/org/chromium/ui/NativeView.java (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/java/src/org/ch...
ui/android/java/src/org/chromium/ui/NativeView.java:34: public void destroy() {
On 2013/04/12 22:00:41, aurimas wrote:
> On 2013/04/12 19:00:48, joth wrote:
> > in webview we need to allow ContentViewCore to cleanup nicely when GC'd. We
> use
> > CleanupReference to achieve that ... I think we'll need one on this guy too.
> > 
> > oh.. and you'll need to move CleanupReference to base. (I was actually
hoping
> to
> > move it up to android_webview, as right now our one and only finalizer is on
> > AwContents and we rely on WebContents::UserData to delete all the
> > similar-lifetimed native objects when the owning AwContents (and hence
> > ContentViewCore) java objects have died.
> > ... unless we can arrange for this not to be a native-object-owning java
> class,
> > and in the same pattern, lean on some existing appropriate life-timed native
> > object to do the cleanup? I'm not fully clear on the use cases to know if
> that's
> > reasonable
> > 
> > adding cleanup ref makes life a bit more complicated as you need to deal
with
> > the java object going away but the native object may not yet know (the
> finalizer
> > can get delayed indefinitely). Also, need to be confident that when the
native
> > object is finally deleted (via explicit destroy() or by finalizer) we don't
> have
> > any dangling pointers to that NativeView still lurking in other native
> objects.
> > 
> > 
> 
> So I was hoping that ContentViewCore.java is the owner ViewAndroid.java.
> 
> ViewAndroid.java is the owner view_android.cc object.
> 
> I though when ContentViewCore.java goes away, it would destroy
ViewAndroid.java
> and that in turn would kill view_android.cc. I was hoping that the
> view_android.cc would not be destroyed in any other way.
> 
> So I guess that is an illusion?

It's all an illusion.
ContentViewCore.destroy() will be called by a conscientious user that does
pro-active resource cleanup. For everyone else, we have to rely on finalizers.

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.cc...
ui/android/view_android.cc:26: if
(env->IsSameObject(weak_java_view_.get(env).obj(), obj))
On 2013/04/12 22:00:41, aurimas wrote:
> On 2013/04/12 19:00:48, joth wrote:
> > in the window between GC and finalization, this means passing null for |obj|
> > would satisfy this...
> 
> Should I check if obj is !null too?

you got it ( PS#5.)

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:19: ViewAndroid(JNIEnv* env, jobject obj,
WindowAndroid* window);
On 2013/04/12 22:00:41, aurimas wrote:
> On 2013/04/12 19:00:48, joth wrote:
> > what's the lifetime relationship of |window| ?  it's safe to just hold a
weak
> > pointer to it?
> 
> WindowAndroid has a life time of the application. It gets created by Main.java
> and get removed once application is closed.

I see.

nit: I think you mean activity, not application. (Main is an activity, and
window_android.h seems to talk about activities).

Maybe comment here that WindowAndroid must outlive the view [and typically, is
bound to the lifetime of the android activity?]

However, now I get this I find a couple more problems:
1/ NativeWindow.java should be renamed to WindowAndroid.java for consistency
2/ NativeWindow.java has the same "assumes destroy() will be called" flaw as
with NativeView in this patch. (possibly can solve both together, although
NativeWindow is far less severe as it's only one leak per activity, rather than
per view), except...
3/ AwContents doesn't respect this contract, and it creating one NativeWindow
per WebView, not per activity. (iirc NativeWindow is to all intents a
placeholder in webview and so it's arbitrary when it's made. quick fix would be
to just turn it into a process-lifetimed singleton).

https://codereview.chromium.org/14018004/diff/5001/ui/android/view_android.h#...
ui/android/view_android.h:25: base::android::ScopedJavaLocalRef<jobject>
GetJavaObject();
On 2013/04/12 22:00:41, aurimas wrote:
> On 2013/04/12 19:00:48, joth wrote:
> > note - will return NULL if the java side was GCed (but the native side not
yet
> > finalized)
> 
> How can I fix this? Add null checks?

yeah.. we basically get this pattern that any time there's a native object with
long-lived relationship to java peer it must be prepared for the java side to
just disappear without any notice.

[aurimas (slooooooooow), 2013-04-17 23:02:52]

joth: please take a look at my change to ContentViewCore#initialize() call, that
now passes a boolean whether ViewAndroid should be initialized.

[joth, 2013-04-18 01:41:17]

I like the way this is going, making on the bold assumption we'll not be needing
any this chromium native view/window plumbing for webview.

It would be nice to do the NativeWindow rename (and move?) in a separate patch,
but not a big deal if you can wrap this up in one more CL...

https://codereview.chromium.org/14018004/diff/25043/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/14018004/diff/25043/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:288:
nativeWebContents, new AwWindowAndroid(mContainerView.getContext()), false,
could you add a bug link here about the fact the AwWindowAndroid may have a
native-side object that is never cleaned up?

https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:107:
AttributeSet attrs, int defStyle, int personality) {
OT for this patch, but the 3 overloads that take |personality| should all be
deleted... we never use ContentView in PERSONALITY_VIEW (aka webview) mode.

https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:144:
boolean isPersonalityView() {
(ditto)

https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:474:
int nativeWebContents, WindowAndroid windowAndroid, boolean
shouldCreateViewAndroid,
Seems odd to pass in the window here, along with bool on whether to pass the
view.
Couldn't we just pass ViewAndroid, and ask that for its window if needed? the
getViewAndroidDelegate() method could be moved to ContentView just fine (it's
only a wrapper around ContentView, after all) and so ContentView will pass in
the ViewAndroidDelegate() instance.

We need to solve the fact that the windowAndroid will then be null for WebView
too, but that's actually nicer and brings it in line with viewAndroid. We'll
pass NULL across to native side for both, and just deal with the null-pointers
at point of use.

the do-nothing AwWindowAndroid can then be deleted :-)

https://codereview.chromium.org/14018004/diff/25043/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):

https://codereview.chromium.org/14018004/diff/25043/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ViewAndroid.java:9: import
org.chromium.ui.gfx.WindowAndroid;
why Window in in ui.gfx but ViewAndroid just in .ui ?

https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.c...
ui/android/view_android.cc:28: return
weak_java_view_.get(AttachCurrentThread());
We should DCHECK that the local-ref is not null, with a comment that it is
mandatory to explicitly call destroy() before releasing the java side object.
(And maybe mention that if we would need to relax this is in future if adding 
CleanupReference based destruct path)

https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.h...
ui/android/view_android.h:16: // This class is the native counterpart for
ViewAndroid.
mention ownership

[aurimas (slooooooooow), 2013-04-18 02:49:57]

Please see my question about AwWindowAndroid usage below.

I will work on creating a separate patch for renaming and moving WindowAndroid
now to land before this one.

https://codereview.chromium.org/14018004/diff/25043/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/14018004/diff/25043/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:288:
nativeWebContents, new AwWindowAndroid(mContainerView.getContext()), false,
On 2013/04/18 01:41:17, joth wrote:
> could you add a bug link here about the fact the AwWindowAndroid may have a
> native-side object that is never cleaned up?

Done.

https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:474:
int nativeWebContents, WindowAndroid windowAndroid, boolean
shouldCreateViewAndroid,
On 2013/04/18 01:41:17, joth wrote:
> Seems odd to pass in the window here, along with bool on whether to pass the
> view.
> Couldn't we just pass ViewAndroid, and ask that for its window if needed? the
> getViewAndroidDelegate() method could be moved to ContentView just fine (it's
> only a wrapper around ContentView, after all) and so ContentView will pass in
> the ViewAndroidDelegate() instance.
> 
> We need to solve the fact that the windowAndroid will then be null for WebView
> too, but that's actually nicer and brings it in line with viewAndroid. We'll
> pass NULL across to native side for both, and just deal with the null-pointers
> at point of use.
> 
> the do-nothing AwWindowAndroid can then be deleted :-)

Does WebView support JavascriptAppModalDialog or SelectFileDialog? It not I can
go ahead an start passing NULL for windowAndroid and delete AwWindow.

https://codereview.chromium.org/14018004/diff/25043/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):

https://codereview.chromium.org/14018004/diff/25043/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ViewAndroid.java:9: import
org.chromium.ui.gfx.WindowAndroid;
On 2013/04/18 01:41:17, joth wrote:
> why Window in in ui.gfx but ViewAndroid just in .ui ?

Em. No good reason :/

https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.c...
ui/android/view_android.cc:28: return
weak_java_view_.get(AttachCurrentThread());
On 2013/04/18 01:41:17, joth wrote:
> We should DCHECK that the local-ref is not null, with a comment that it is
> mandatory to explicitly call destroy() before releasing the java side object.
> (And maybe mention that if we would need to relax this is in future if adding 
> CleanupReference based destruct path)

Done.

https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.h...
ui/android/view_android.h:16: // This class is the native counterpart for
ViewAndroid.
On 2013/04/18 01:41:17, joth wrote:
> mention ownership

Done.

[joth, 2013-04-18 13:52:20]

On Wednesday, 17 April 2013,  <aurimas@chromium.org> wrote:
> Please see my question about AwWindowAndroid usage below.
>

It's all just empty methods. IIRC the usage of WindowAndroid all happens in
chrome/ code and we don't even build that.

> I will work on creating a separate patch for renaming and moving
WindowAndroid
> now to land before this one.
>
>
>
https://codereview.chromium.org/14018004/diff/25043/android_webview/java/src/...
> File
> android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
>
>
https://codereview.chromium.org/14018004/diff/25043/android_webview/java/src/...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:288:
> nativeWebContents, new AwWindowAndroid(mContainerView.getContext()),
> false,
> On 2013/04/18 01:41:17, joth wrote:
>>
>> could you add a bug link here about the fact the AwWindowAndroid may
>
> have a
>>
>> native-side object that is never cleaned up?
>
> Done.
>
>
https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
>
>
https://codereview.chromium.org/14018004/diff/25043/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:474:
> int nativeWebContents, WindowAndroid windowAndroid, boolean
> shouldCreateViewAndroid,
> On 2013/04/18 01:41:17, joth wrote:
>>
>> Seems odd to pass in the window here, along with bool on whether to
>
> pass the
>>
>> view.
>> Couldn't we just pass ViewAndroid, and ask that for its window if
>
> needed? the
>>
>> getViewAndroidDelegate() method could be moved to ContentView just
>
> fine (it's
>>
>> only a wrapper around ContentView, after all) and so ContentView will
>
> pass in
>>
>> the ViewAndroidDelegate() instance.
>
>> We need to solve the fact that the windowAndroid will then be null for
>
> WebView
>>
>> too, but that's actually nicer and brings it in line with viewAndroid.
>
> We'll
>>
>> pass NULL across to native side for both, and just deal with the
>
> null-pointers
>>
>> at point of use.
>
>> the do-nothing AwWindowAndroid can then be deleted :-)
>
> Does WebView support JavascriptAppModalDialog or SelectFileDialog? It
> not I can go ahead an start passing NULL for windowAndroid and delete
> AwWindow.
>
>
https://codereview.chromium.org/14018004/diff/25043/ui/android/java/src/org/c...
> File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):
>
>
https://codereview.chromium.org/14018004/diff/25043/ui/android/java/src/org/c...
> ui/android/java/src/org/chromium/ui/ViewAndroid.java:9: import
> org.chromium.ui.gfx.WindowAndroid;
> On 2013/04/18 01:41:17, joth wrote:
>>
>> why Window in in ui.gfx but ViewAndroid just in .ui ?
>
> Em. No good reason :/
>
>
https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.cc
> File ui/android/view_android.cc (right):
>
>
https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.c...
> ui/android/view_android.cc:28: return
> weak_java_view_.get(AttachCurrentThread());
> On 2013/04/18 01:41:17, joth wrote:
>>
>> We should DCHECK that the local-ref is not null, with a comment that
>
> it is
>>
>> mandatory to explicitly call destroy() before releasing the java side
>
> object.
>>
>> (And maybe mention that if we would need to relax this is in future if
>
> adding
>>
>> CleanupReference based destruct path)
>
> Done.
>
>
https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.h
> File ui/android/view_android.h (right):
>
>
https://codereview.chromium.org/14018004/diff/25043/ui/android/view_android.h...
> ui/android/view_android.h:16: // This class is the native counterpart
> for ViewAndroid.
> On 2013/04/18 01:41:17, joth wrote:
>>
>> mention ownership
>
> Done.
>
> https://codereview.chromium.org/14018004/
>

[aurimas (slooooooooow), 2013-04-19 18:10:56]

nilesh: please take a look at this CL after my rebase.

[nilesh, 2013-04-19 20:52:29]

lgtm. Please wait for Joth's approval

https://codereview.chromium.org/14018004/diff/41001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):

https://codereview.chromium.org/14018004/diff/41001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ViewAndroid.java:24: private final
WindowAndroid mNativeWindow;
mWindowAndroid

[joth, 2013-04-19 20:59:44]

lgtm (I just skimmed the content/ changes this time)

https://codereview.chromium.org/14018004/diff/41001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14018004/diff/41001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:662:
if (mViewAndroid != null) mViewAndroid.destroy();
I forget - shouldn't there be a similar mWindowAndroid.destory() too?

[aurimas (slooooooooow), 2013-04-19 21:32:04]

Hey guys!

Please take a look.

+thakis for ui/* and chrome/browser/platform_util_android.cc

+piman for content/browser/* and content/shell/*

Aurimas

https://codereview.chromium.org/14018004/diff/41001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14018004/diff/41001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:662:
if (mViewAndroid != null) mViewAndroid.destroy();
On 2013/04/19 20:59:45, joth wrote:
> I forget - shouldn't there be a similar mWindowAndroid.destory() too?

It is in Main.java#Destroy().

https://codereview.chromium.org/14018004/diff/41001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):

https://codereview.chromium.org/14018004/diff/41001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ViewAndroid.java:24: private final
WindowAndroid mNativeWindow;
On 2013/04/19 20:52:30, nilesh wrote:
> mWindowAndroid

Done.

[aurimas (slooooooooow), 2013-04-20 05:12:28]

Hey guys!

Please take a look at this CL.

+thakis for ui/* and chrome/browser/platform_util_android.cc

+piman for content/browser/* and content/shell/*

Aurimas

[Nico, 2013-04-20 05:14:15]

Can you change the CL description so that it actually describes the change and
doesn't just say "refactoring"? Describe which changes you are making.

[aurimas (slooooooooow), 2013-04-21 20:57:02]

On 2013/04/20 05:14:15, Nico wrote:
> Can you change the CL description so that it actually describes the change and
> doesn't just say "refactoring"? Describe which changes you are making.

Nico, I just added the description. Let me know if it needs more detail.

Thanks!
Aurimas

[Nico, 2013-04-22 18:34:34]

lgtm, thanks

(nit: cl descriptions are imho easier to read if you don't use gerund forms,
e.g. say "Refactor" and "Create" instead of "Refactoring" and "Creating". Also,
s/Java object/Java objects/)

[aurimas (slooooooooow), 2013-04-22 18:49:47]

Hey Antoine,

I am missing review for:
content/browser/web_contents/web_contents_view_android.cc
content/shell/shell_web_contents_view_delegate_android.cc

Could you please take a look?

Thanks,
Aurimas

[piman, 2013-04-22 19:44:21]

LGTM for content/

[commit-bot: I haz the power, 2013-04-22 20:04:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/14018004/59001

[commit-bot: I haz the power, 2013-04-23 00:11:09]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java
  Hunk #1 FAILED at 22.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java.rej

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java
b/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java
index
7423e3facc5e5b919135252308851cb0910f7504..5ed27577440eab530c590d44c09f47d5338f56cb
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogGlue.java
@@ -22,21 +22,21 @@ public class AutofillDialogGlue implements
AutofillDialogDelegate,
     private final AutofillDialog mAutofillDialog;
     private final AutofillDialogAccountHelper mAccountHelper;
 
-    public AutofillDialogGlue(int nativeAutofillDialogViewAndroid,
WindowAndroid nativeWindow,
+    public AutofillDialogGlue(int nativeAutofillDialogViewAndroid,
WindowAndroid windowAndroid,
             String useBillingForShippingText, String saveLocallyText) {
         mNativeDialogPopup = nativeAutofillDialogViewAndroid;
-        mAccountHelper = new AutofillDialogAccountHelper(this,
nativeWindow.getContext());
+        mAccountHelper = new AutofillDialogAccountHelper(this,
windowAndroid.getContext());
 
         mAutofillDialog = new AutofillDialog(
-                nativeWindow.getContext(), this, useBillingForShippingText,
saveLocallyText);
+                windowAndroid.getContext(), this, useBillingForShippingText,
saveLocallyText);
         mAutofillDialog.show();
     }
 
     @CalledByNative
     private static AutofillDialogGlue create(int
nativeAutofillDialogViewAndroid,
-            WindowAndroid nativeWindow,
+            WindowAndroid windowAndroid,
             String useBillingForShippingText, String saveLocallyText) {
-        return new AutofillDialogGlue(nativeAutofillDialogViewAndroid,
nativeWindow,
+        return new AutofillDialogGlue(nativeAutofillDialogViewAndroid,
windowAndroid,
                 useBillingForShippingText, saveLocallyText);
     }

[commit-bot: I haz the power, 2013-04-23 00:46:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/14018004/70001

[commit-bot: I haz the power, 2013-04-23 02:31:58]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-23 03:54:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/14018004/70001

[commit-bot: I haz the power, 2013-04-23 07:36:47]

Change committed as 195750