Removing ContentView dependencies for few functions which acts as WebContents wrapper.

Removing ContentView dependencies for few functions which acts as WebContents wrapper.

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

[AKVT, 2014-07-09 11:34:32]

PTAL this change

[AKVT, 2014-07-09 11:44:27]

PTAL

[AKVT, 2014-07-09 16:48:39]

@Yaron and jdduke - PTAL

[jdduke (slow), 2014-07-09 17:13:14]

https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853:
if (mWebContents != null) return mWebContents.getBackgroundColor();
I wonder if checking mWebContenst for null in each of these places is really
just hiding bugs... Should we be asserting instead? Are there legitimate cases
where ContentViewCore can/should be accessed *after* ContentViewCore.destroy()
has been called?

[Yaron, 2014-07-09 17:34:02]

https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853:
if (mWebContents != null) return mWebContents.getBackgroundColor();
On 2014/07/09 17:13:14, jdduke wrote:
> I wonder if checking mWebContenst for null in each of these places is really
> just hiding bugs... Should we be asserting instead? Are there legitimate cases
> where ContentViewCore can/should be accessed *after* ContentViewCore.destroy()
> has been called?

That's a fair point. I thought Ted and I discussed this when he started
introducing WebContents.java but I don't see it in review comments.

CVC should be 1<->1 with WC. The tricky case is prerendering but I believe that
should swap both of these classes. So I think you're right that we could try
removing the null checks and investigate NPEs that come up for real bugs that
this was bandaiding. I imagine this could happen because Tab has to deal with
the fact that CVC/WebContents might not be loaded but our null-checks may have
masked it

[AKVT, 2014-07-11 12:59:42]

@jdduke and Yaron PTAL

https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853:
if (mWebContents != null) return mWebContents.getBackgroundColor();
On 2014/07/09 17:13:14, jdduke wrote:
> I wonder if checking mWebContenst for null in each of these places is really
> just hiding bugs... Should we be asserting instead? Are there legitimate cases
> where ContentViewCore can/should be accessed *after* ContentViewCore.destroy()
> has been called?

Done.

https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853:
if (mWebContents != null) return mWebContents.getBackgroundColor();
On 2014/07/09 17:13:14, jdduke wrote:
> I wonder if checking mWebContenst for null in each of these places is really
> just hiding bugs... Should we be asserting instead? Are there legitimate cases
> where ContentViewCore can/should be accessed *after* ContentViewCore.destroy()
> has been called?

I have kept assert to catch any hidden errors due to null checks.

[AKVT, 2014-07-11 16:42:27]

On 2014/07/11 12:59:42, AKV wrote:
> @jdduke and Yaron PTAL
> 
>
https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853:
> if (mWebContents != null) return mWebContents.getBackgroundColor();
> On 2014/07/09 17:13:14, jdduke wrote:
> > I wonder if checking mWebContenst for null in each of these places is really
> > just hiding bugs... Should we be asserting instead? Are there legitimate
cases
> > where ContentViewCore can/should be accessed *after*
ContentViewCore.destroy()
> > has been called?
> 
> Done.
> 
>
https://codereview.chromium.org/381593002/diff/40001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853:
> if (mWebContents != null) return mWebContents.getBackgroundColor();
> On 2014/07/09 17:13:14, jdduke wrote:
> > I wonder if checking mWebContenst for null in each of these places is really
> > just hiding bugs... Should we be asserting instead? Are there legitimate
cases
> > where ContentViewCore can/should be accessed *after*
ContentViewCore.destroy()
> > has been called?
> 
> I have kept assert to catch any hidden errors due to null checks.

@jdduke and Yaron PTAL

[jdduke (slow), 2014-07-11 16:51:25]

https://codereview.chromium.org/381593002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/381593002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:856:
assert mWebContents == null;
Have you done at least a smoke debug (Android and Chrome) build test and trial
to make sure this hasn't uncovered any problems? You'll probably want to change
== to != in the meantime, and remove the additional line break after the
asserts. You'll need a user debug build for Android, or use "adb shell setprop
debug.assert 1".

[AKVT, 2014-07-12 08:46:12]

@jdduke & Yaron PTAL

https://codereview.chromium.org/381593002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/381593002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:856:
assert mWebContents == null;
On 2014/07/11 16:51:25, jdduke wrote:
> Have you done at least a smoke debug (Android and Chrome) build test and trial
> to make sure this hasn't uncovered any problems? You'll probably want to
change
> == to != in the meantime, and remove the additional line break after the
> asserts. You'll need a user debug build for Android, or use "adb shell setprop
> debug.assert 1".
@jdduke Thanks for the review comments. I have changed the assert conditions and
ran adb shell setprop debug.assert 1 command to ensure asserts added has been
tested thoroughly. I couldn't see any asserts in various scenarios.

[AKVT, 2014-07-14 18:52:27]

On 2014/07/12 08:46:12, AKV wrote:
> @jdduke & Yaron PTAL
> 
>
https://codereview.chromium.org/381593002/diff/60001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/381593002/diff/60001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:856:
> assert mWebContents == null;
> On 2014/07/11 16:51:25, jdduke wrote:
> > Have you done at least a smoke debug (Android and Chrome) build test and
trial
> > to make sure this hasn't uncovered any problems? You'll probably want to
> change
> > == to != in the meantime, and remove the additional line break after the
> > asserts. You'll need a user debug build for Android, or use "adb shell
setprop
> > debug.assert 1".
> @jdduke Thanks for the review comments. I have changed the assert conditions
and
> ran adb shell setprop debug.assert 1 command to ensure asserts added has been
> tested thoroughly. I couldn't see any asserts in various scenarios.

@jdduke and Yaron PTAL

[AKVT, 2014-07-15 05:05:07]

On 2014/07/14 18:52:27, AKV wrote:
> On 2014/07/12 08:46:12, AKV wrote:
> > @jdduke & Yaron PTAL
> > 
> >
>
https://codereview.chromium.org/381593002/diff/60001/content/public/android/j...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/381593002/diff/60001/content/public/android/j...
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:856:
> > assert mWebContents == null;
> > On 2014/07/11 16:51:25, jdduke wrote:
> > > Have you done at least a smoke debug (Android and Chrome) build test and
> trial
> > > to make sure this hasn't uncovered any problems? You'll probably want to
> > change
> > > == to != in the meantime, and remove the additional line break after the
> > > asserts. You'll need a user debug build for Android, or use "adb shell
> setprop
> > > debug.assert 1".
> > @jdduke Thanks for the review comments. I have changed the assert conditions
> and
> > ran adb shell setprop debug.assert 1 command to ensure asserts added has
been
> > tested thoroughly. I couldn't see any asserts in various scenarios.
> 
> @jdduke and Yaron PTAL

android_fyi_dbg buildbot failure is not related to this change. It shows assert
is unreliable in other files, not in changed files. PTAL

[jdduke (slow), 2014-07-15 15:32:45]

On 2014/07/15 05:05:07, AKV wrote:
> On 2014/07/14 18:52:27, AKV wrote:
> > @jdduke and Yaron PTAL
> 
> android_fyi_dbg buildbot failure is not related to this change. It shows
assert
> is unreliable in other files, not in changed files. PTAL

I'll defer to yfriedman@ for further review.

[Yaron, 2014-07-15 21:23:49]

https://codereview.chromium.org/381593002/diff/80001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (left):

https://codereview.chromium.org/381593002/diff/80001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1368: long
ContentViewCoreImpl::GetNativeImeAdapter(JNIEnv* env, jobject obj) {
I don't think it makes sense to move this out. You've left the whole IME adapter
management in CVC but moved this accessor (which has the side-effect of making
it public). 

As we've discussed briefly elsewhere, as a separate change I think we'll need a
RenderWidgetHostViewAndroid wrapper as well to properly manage IME

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:221: NavigationEntry* entry
= web_contents_->GetController().GetVisibleEntry();
Seems like this would be better in NavigationController

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_android.h (right):

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.h:18: using
base::android::ScopedJavaLocalRef;
Don't add this to header

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.h:45: float dpi_scale() const
{ return dpi_scale_; }
Why did this move and why is it still duplicated in content_view_core? It
doesn't help us to partially move something and we should keep ownership more
clear. Please move things in logical blocks

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.h:67:
ScopedJavaLocalRef<jstring> GetOriginalUrlForActiveNavigationEntry(
base::android::ScopedJavaLocalRef

https://codereview.chromium.org/381593002/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/381593002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1611:
// The native side keeps track of whether the zoom and scroll actually occurred.
It is
This comment doesn't make sense here. Move to WebContentsImpl

https://codereview.chromium.org/381593002/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/381593002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:46:
public int getBackgroundColor();
Please add javadoc for new functions here.

[Yaron, 2014-07-15 21:28:07]

as an aside: I thought of another way of looking at this general issue. We can
look at ContentViewCore::FromWebContents and see who's trying to go from a
WebContents to a ContentViewCore and see whether that makes sense or if the
responsibility should be of WebContents

[AKVT, 2014-07-16 09:53:47]

PTAL

https://codereview.chromium.org/381593002/diff/80001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (left):

https://codereview.chromium.org/381593002/diff/80001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1368: long
ContentViewCoreImpl::GetNativeImeAdapter(JNIEnv* env, jobject obj) {
On 2014/07/15 21:23:48, Yaron wrote:
> I don't think it makes sense to move this out. You've left the whole IME
adapter
> management in CVC but moved this accessor (which has the side-effect of making
> it public). 
> 
> As we've discussed briefly elsewhere, as a separate change I think we'll need
a
> RenderWidgetHostViewAndroid wrapper as well to properly manage IME
Thanks, IME changes I am keeping untouched time being to handle separately.

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:221: NavigationEntry* entry
= web_contents_->GetController().GetVisibleEntry();
On 2014/07/15 21:23:48, Yaron wrote:
> Seems like this would be better in NavigationController

Will take care in next patch when I handle NavigationController separately.

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_android.h (right):

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.h:18: using
base::android::ScopedJavaLocalRef;
On 2014/07/15 21:23:48, Yaron wrote:
> Don't add this to header

Done.

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.h:45: float dpi_scale() const
{ return dpi_scale_; }
On 2014/07/15 21:23:48, Yaron wrote:
> Why did this move and why is it still duplicated in content_view_core? It
> doesn't help us to partially move something and we should keep ownership more
> clear. Please move things in logical blocks

I will take care this one during next patch.

https://codereview.chromium.org/381593002/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.h:67:
ScopedJavaLocalRef<jstring> GetOriginalUrlForActiveNavigationEntry(
On 2014/07/15 21:23:48, Yaron wrote:
> base::android::ScopedJavaLocalRef

Done.

https://codereview.chromium.org/381593002/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/381593002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1611:
// The native side keeps track of whether the zoom and scroll actually occurred.
It is
On 2014/07/15 21:23:48, Yaron wrote:
> This comment doesn't make sense here. Move to WebContentsImpl

Done.

https://codereview.chromium.org/381593002/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/381593002/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:46:
public int getBackgroundColor();
On 2014/07/15 21:23:48, Yaron wrote:
> Please add javadoc for new functions here.

Done.

[Yaron, 2014-07-16 17:14:54]

https://codereview.chromium.org/381593002/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/381593002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:91: if (web_contents_) {
remove if-check. This is constructed with a web contents and can't be unset.

https://codereview.chromium.org/381593002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:129: if (!web_contents_)
remove

[AKVT, 2014-07-17 03:08:32]

PTAL

https://codereview.chromium.org/381593002/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/381593002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:91: if (web_contents_) {
On 2014/07/16 17:14:54, Yaron wrote:
> remove if-check. This is constructed with a web contents and can't be unset.

Done.

https://codereview.chromium.org/381593002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:129: if (!web_contents_)
On 2014/07/16 17:14:54, Yaron wrote:
> remove

Done.

[Yaron, 2014-07-18 00:39:40]

lgtm

+tedchoc for web_contents_android

[AKVT, 2014-07-18 15:25:16]

@Ted C and sievers PTAL.

[no sievers, 2014-07-18 19:42:03]

lgtm, but you might want to TBR Ted at least (he's OOTO).

https://codereview.chromium.org/381593002/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/381593002/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:188: RenderViewHost* host =
web_contents_->GetRenderViewHost();
Is there a reason why some call sites do a null-check on |host| and others
don't?

https://codereview.chromium.org/381593002/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.h (right):

https://codereview.chromium.org/381593002/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_android.h:43:
RenderWidgetHostViewAndroid* GetRenderWidgetHostViewAndroid();
GetRenderWidgetHostViewAndroid() doesn't need to be public.

[AKVT, 2014-07-19 10:39:33]

PTAL

https://codereview.chromium.org/381593002/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/381593002/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:188: RenderViewHost* host =
web_contents_->GetRenderViewHost();
On 2014/07/18 19:42:03, sievers wrote:
> Is there a reason why some call sites do a null-check on |host| and others
> don't?
Thanks. null-check has been made as uniform in all the areas where host is
getting accessed.

https://codereview.chromium.org/381593002/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.h (right):

https://codereview.chromium.org/381593002/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_android.h:43:
RenderWidgetHostViewAndroid* GetRenderWidgetHostViewAndroid();
On 2014/07/18 19:42:03, sievers wrote:
> GetRenderWidgetHostViewAndroid() doesn't need to be public.

Done.

[AKVT, 2014-07-19 10:39:53]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-07-19 10:40:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/381593002/140001

[commit-bot: I haz the power, 2014-07-19 12:55:08]

Change committed as 284334