Have WebContentsViewAndroid own ViewAndroid

Have WebContentsViewAndroid own ViewAndroid

This CL transfers the ownership of ViewAndroid associated with Java
content view from CVC to WebContentsViewAndroid, as proposed in
the design https://goo.gl/tQqbZS This is also a step toward removing
references to CVC in RWHVA.

Also simplified an access method to WindowAndroid using the view
hierarchy.

BUG=598880, 626765
Committed: https://crrev.com/4101e961ba3d80e91683f1c7e28e178831595bcc
Cr-Commit-Position: refs/heads/master@{#428960}

[Jinsuk Kim, 2016-10-27 07:25:52]

Patchset #1 (id:1) has been deleted

[Jinsuk Kim, 2016-10-28 06:00:58]

Patchset #1 (id:20001) has been deleted

[Jinsuk Kim, 2016-10-28 06:01:32]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 06:01:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 06:31:05]

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

[commit-bot: I haz the power, 2016-10-28 06:31:06]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2016-10-28 06:35:39]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2016-10-28 15:58:58]

https://codereview.chromium.org/2453213003/diff/40001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2453213003/diff/40001/content/browser/android...
content/browser/android/content_view_core_impl.cc:1546:
web_contents->GetView()->CreateView(
it's not ok to just reuse this call to *set* ViewAndroid. the meaning of
CreateView is for it to the call to create a *new* NativeView, and possibly used
the passed in NativeView as a context

I think easiest thing for now is have ViewAndroid get its delegate after
construction, perhaps in an Init method as well. Then you can keep creating
ViewAndroid in WebContentsViewAndroid instance (ie not with unique_ptr),
CreateView can keep being a no-op. And then you can set the delegate here

[Jinsuk Kim, 2016-10-31 01:17:00]

https://codereview.chromium.org/2453213003/diff/40001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2453213003/diff/40001/content/browser/android...
content/browser/android/content_view_core_impl.cc:1546:
web_contents->GetView()->CreateView(
On 2016/10/28 15:58:58, boliu wrote:
> it's not ok to just reuse this call to *set* ViewAndroid. the meaning of
> CreateView is for it to the call to create a *new* NativeView, and possibly
used
> the passed in NativeView as a context
> 
> I think easiest thing for now is have ViewAndroid get its delegate after
> construction, perhaps in an Init method as well. Then you can keep creating
> ViewAndroid in WebContentsViewAndroid instance (ie not with unique_ptr),
> CreateView can keep being a no-op. And then you can set the delegate here

Done.

[boliu, 2016-10-31 01:45:52]

https://codereview.chromium.org/2453213003/diff/60001/content/browser/android...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/2453213003/diff/60001/content/browser/android...
content/browser/android/content_view_core_impl.h:390: ui::ViewAndroid*
GetViewAndroid() const;
maybe not in this CL, but eventually should remove this too, no sure how hard
that is

https://codereview.chromium.org/2453213003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2453213003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:985: if
(!content_view_core_ || !(view_.GetWindowAndroid())) {
don't need the cvc check if it's not used

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

https://codereview.chromium.org/2453213003/diff/60001/ui/android/view_android...
ui/android/view_android.cc:71: ViewAndroid::ViewAndroid(const JavaRef<jobject>&
delegate)
is there anything (other than tests) that pass in a non-null object here? if
not, remove it pls

[Jinsuk Kim, 2016-10-31 02:01:17]

https://codereview.chromium.org/2453213003/diff/60001/content/browser/android...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/2453213003/diff/60001/content/browser/android...
content/browser/android/content_view_core_impl.h:390: ui::ViewAndroid*
GetViewAndroid() const;
On 2016/10/31 01:45:52, boliu wrote:
> maybe not in this CL, but eventually should remove this too, no sure how hard
> that is

Acknowledged. Do you mean just 'const' or GetViewAndroid() method?

https://codereview.chromium.org/2453213003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2453213003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:985: if
(!content_view_core_ || !(view_.GetWindowAndroid())) {
On 2016/10/31 01:45:52, boliu wrote:
> don't need the cvc check if it's not used

Done.

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

https://codereview.chromium.org/2453213003/diff/60001/ui/android/view_android...
ui/android/view_android.cc:71: ViewAndroid::ViewAndroid(const JavaRef<jobject>&
delegate)
On 2016/10/31 01:45:52, boliu wrote:
> is there anything (other than tests) that pass in a non-null object here? if
> not, remove it pls

Done.

[boliu, 2016-10-31 02:05:45]

lgtm

https://codereview.chromium.org/2453213003/diff/60001/content/browser/android...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/2453213003/diff/60001/content/browser/android...
content/browser/android/content_view_core_impl.h:390: ui::ViewAndroid*
GetViewAndroid() const;
On 2016/10/31 02:01:16, Jinsuk Kim wrote:
> On 2016/10/31 01:45:52, boliu wrote:
> > maybe not in this CL, but eventually should remove this too, no sure how
hard
> > that is
> 
> Acknowledged. Do you mean just 'const' or GetViewAndroid() method?

whole method, things should ask WebContentsView for the Viewandroid, not go
through CVC

[Jinsuk Kim, 2016-10-31 02:07:11]

jinsukkim@chromium.org changed reviewers:
+ twellington@chromium.org

[Jinsuk Kim, 2016-10-31 02:07:12]

twellington@chromium.org: Please review changes in  ui/android.

[Theresa, 2016-10-31 18:10:25]

lgtm

[Jinsuk Kim, 2016-11-01 00:09:25]

The CQ bit was checked by jinsukkim@chromium.org

[commit-bot: I haz the power, 2016-11-01 00:09:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 00:30:31]

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

[commit-bot: I haz the power, 2016-11-01 00:30:32]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jinsuk Kim, 2016-11-01 07:13:47]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-11-01 07:13:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2453213003/#ps100001
(title: "fixed builds & rebased")

[commit-bot: I haz the power, 2016-11-01 07:14:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 07:44:08]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-11-01 07:46:18]

Description was changed from

==========
Have WebContentsViewAndroid own ViewAndroid

This CL transfers the ownership of ViewAndroid associated with Java
content view from CVC to WebContentsViewAndroid, as proposed in
the design https://goo.gl/tQqbZS This is also a step toward removing
references to CVC in RWHVA.

Also simplified an access method to WindowAndroid using the view
hierarchy.

BUG=598880, 626765
==========

to

==========
Have WebContentsViewAndroid own ViewAndroid

This CL transfers the ownership of ViewAndroid associated with Java
content view from CVC to WebContentsViewAndroid, as proposed in
the design https://goo.gl/tQqbZS This is also a step toward removing
references to CVC in RWHVA.

Also simplified an access method to WindowAndroid using the view
hierarchy.

BUG=598880, 626765

Committed: https://crrev.com/4101e961ba3d80e91683f1c7e28e178831595bcc
Cr-Commit-Position: refs/heads/master@{#428960}
==========

[commit-bot: I haz the power, 2016-11-01 07:46:19]

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

[boliu, 2016-11-01 15:11:12]

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

https://codereview.chromium.org/2453213003/diff/100001/ui/android/view_androi...
ui/android/view_android.h:59: ViewAndroid(const base::android::JavaRef<jobject>&
delegate);
fix blimp rather than adding this back

[Jinsuk Kim, 2016-11-02 04:22:01]

On 2016/11/01 15:11:12, boliu wrote:
>
https://codereview.chromium.org/2453213003/diff/100001/ui/android/view_android.h
> File ui/android/view_android.h (right):
> 
>
https://codereview.chromium.org/2453213003/diff/100001/ui/android/view_androi...
> ui/android/view_android.h:59: ViewAndroid(const
base::android::JavaRef<jobject>&
> delegate);
> fix blimp rather than adding this back

Will do that in a follow-up CL.