[Android WebView] Synthesize a fake page loading event on page source modification

[Android WebView] Synthesize a fake page loading event on page source modification

When a script modifies page source of a non-committed page, we need to
notify clients, so they can update the URL bar to avoid confusion.

BUG=458569
Committed: https://crrev.com/e38bb6cdeb1170cb2cd7faae8507dc6c34c58c7d
Cr-Commit-Position: refs/heads/master@{#318222}

[mnaganov (inactive), 2015-02-16 17:07:07]

mnaganov@chromium.org changed reviewers:
+ torne@chromium.org

[mnaganov (inactive), 2015-02-16 17:07:08]

The fix is quite simple and should not introduce any unnecessary calls of
'onPageFinished' and related functions.

I'm a bit concerned that we have to add another case of accessing non-public
interface of WebContents inside WebContentsAndroid, but other options like
exposing the 'hasAccessedInitialDocument' on the public interface, or
duplicating WebViewImpl's logic for handling this property appear even worse to
me.

[mnaganov (inactive), 2015-02-19 21:03:48]

On 2015/02/16 17:07:08, mnaganov (cr) wrote:
> The fix is quite simple and should not introduce any unnecessary calls of
> 'onPageFinished' and related functions.
> 
> I'm a bit concerned that we have to add another case of accessing non-public
> interface of WebContents inside WebContentsAndroid, but other options like
> exposing the 'hasAccessedInitialDocument' on the public interface, or
> duplicating WebViewImpl's logic for handling this property appear even worse
to
> me.

torne@: Ping!

[mnaganov (inactive), 2015-02-19 21:03:48]

On 2015/02/16 17:07:08, mnaganov (cr) wrote:
> The fix is quite simple and should not introduce any unnecessary calls of
> 'onPageFinished' and related functions.
> 
> I'm a bit concerned that we have to add another case of accessing non-public
> interface of WebContents inside WebContentsAndroid, but other options like
> exposing the 'hasAccessedInitialDocument' on the public interface, or
> duplicating WebViewImpl's logic for handling this property appear even worse
to
> me.

torne@: Ping!

[Torne, 2015-02-20 17:15:46]

Sorry, I was distracted with too many things. This all looks pretty reasonable
and the test seems fine, so LGTM.

[mnaganov (inactive), 2015-02-23 10:48:50]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2015-02-23 10:49:40]

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

[commit-bot: I haz the power, 2015-02-23 10:52:40]

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

[commit-bot: I haz the power, 2015-02-23 10:52:41]

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

[mnaganov (inactive), 2015-02-23 11:40:47]

New patchsets have been uploaded after l-g-t-m from torne@chromium.org

[mnaganov (inactive), 2015-02-23 11:49:39]

mnaganov@chromium.org changed reviewers:
+ davidben@chromium.org, tedchoc@chromium.org

[mnaganov (inactive), 2015-02-23 11:49:40]

Need an OWNERs review:

tedchoc@: Please look at android bits
davidben@: Please look at general bits in content/ and net/

[davidben, 2015-02-23 20:06:44]

davidben@chromium.org changed reviewers:
+ creis@chromium.org

[davidben, 2015-02-23 20:06:44]

+creis who is probably a much better reviewer for things that involve
HasAccessedInitialDocument and pending URLs.

[Ted C, 2015-02-23 21:04:08]

lgtm

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

https://codereview.chromium.org/924833003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:483: jboolean
WebContentsAndroid::HasPageScriptAccessedInitialDocument(
Exposing this is a layering violation of sorts.  WebContents.java "should" only
expose methods from web_contents.h, but this is publicly exposing a
web_contents_impl.h method (actually a navigation_controller_delegate.h method).
 But we already do that so much in that file, so I guess you're not being
inconsistent :-/

Why does this name differ from WebContentsImpl's method?  We try to keep them in
sync.

[Charlie Reis, 2015-02-23 21:28:05]

Hmm, I'm worried about having two functionally different defenses for the same
bug, depending on what platform we're on.  I haven't yet figured out if this
defense is correct in all cases, but a few questions first.

https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:223:
url = TextUtils.isEmpty(url) ? aboutBlank : url;
I'm concerned about this code being different from desktop Chrome.  It seems
like we should just be resetting the address bar to GetVisibleEntry.  Making
this specific to about:blank seems different and potentially risky.

What's the reason we can't do the same thing as in desktop Chrome?  If we need
to diverge, we should carefully reason about this.

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

https://codereview.chromium.org/924833003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:483: jboolean
WebContentsAndroid::HasPageScriptAccessedInitialDocument(
On 2015/02/23 21:04:08, Ted C wrote:
> Exposing this is a layering violation of sorts.  WebContents.java "should"
only
> expose methods from web_contents.h, but this is publicly exposing a
> web_contents_impl.h method (actually a navigation_controller_delegate.h
method).
>  But we already do that so much in that file, so I guess you're not being
> inconsistent :-/
> 
> Why does this name differ from WebContentsImpl's method?  We try to keep them
in
> sync.

Actually, why do we need to expose this?  In desktop Chrome,
WebContentsImpl::DidAccessInitialDocument calls
NotifyNavigationStateChanged(INVALIDATE_TYPE_URL), which chrome/ code uses to
reset the address bar to the new value of GetVisibleEntry.

HasAccessedInitialDocument is only used internally for implementing
GetVisibleEntry.

[mnaganov (inactive), 2015-02-23 22:17:55]

tedchoc@: Thanks!

creis@: Hi Charlie! I believe, there are not many differences in fact. Please
see my replies.

https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:223:
url = TextUtils.isEmpty(url) ? aboutBlank : url;
On 2015/02/23 21:28:04, Charlie Reis wrote:
> I'm concerned about this code being different from desktop Chrome.  It seems
> like we should just be resetting the address bar to GetVisibleEntry.  Making
> this specific to about:blank seems different and potentially risky.
>

I'll double-check, if we are allowed to pass null urls to clients. I'm afraid,
we can't, that's why I have added this conversion.

In fact, GetVisibleEntry is what we retrieve under the hood. This is the
sequence of calls: AwContents.getUrl() -> WebContents.getUrl() [Java] ->
WebContentsAndroid::GetURL() [C++] -> WebContentsImpl::GetURL() ->
GetVisibleEntry()->GetVirtualURL() 
 
> What's the reason we can't do the same thing as in desktop Chrome?  If we need
> to diverge, we should carefully reason about this.

So I guess, we are not diverging beyond what is required to conform to WebView
API.

As for the reason, why we send events only for blank URLs, is to reduce
possibility of synthesizing superfluous events that will confuse clients.

Do you think there are any other URLs besides empty / about:blank that should be
considered here?

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

https://codereview.chromium.org/924833003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:483: jboolean
WebContentsAndroid::HasPageScriptAccessedInitialDocument(
On 2015/02/23 21:28:04, Charlie Reis wrote:
> On 2015/02/23 21:04:08, Ted C wrote:
> > Exposing this is a layering violation of sorts.  WebContents.java "should"
> only
> > expose methods from web_contents.h, but this is publicly exposing a
> > web_contents_impl.h method (actually a navigation_controller_delegate.h
> method).
> >  But we already do that so much in that file, so I guess you're not being
> > inconsistent :-/
> > 
> > Why does this name differ from WebContentsImpl's method?  We try to keep
them
> in
> > sync.
> 
> Actually, why do we need to expose this?  In desktop Chrome,
> WebContentsImpl::DidAccessInitialDocument calls
> NotifyNavigationStateChanged(INVALIDATE_TYPE_URL), which chrome/ code uses to
> reset the address bar to the new value of GetVisibleEntry.
> 
> HasAccessedInitialDocument is only used internally for implementing
> GetVisibleEntry.

This is to avoid generating premature / superfluous OnPageStarted and
OnPageFinished events in WebView. I observed that NotifyNavigationStateChanged
for the URL is fired in the middle of page loading. If we will be
unconditionally synthesizing the events immediately, it will make clients to
believe that the page has done loading. And afterwards they will be receiving
another OnPageFinished via the regular means.

That means, we want to synthesize the events only when we are sure that this is
happening due to the document being accessed.

The difference between Chrome and WebView here is that WebView doesn't have an
API for controlling the URL bar. The only way we can make clients to update the
URL bar is to emulate page loading, which must not interfere with real page
loading process.

https://codereview.chromium.org/924833003/diff/80001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/924833003/diff/80001/content/content.gyp#newc...
content/content.gyp:502: 'source_file': 'public/browser/invalidate_type.h',
Note to self: we can now get rid of the manual mirror of these enums in
WebContentsDelegateAndroid.java

[mnaganov (inactive), 2015-02-24 11:28:42]

https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:223:
url = TextUtils.isEmpty(url) ? aboutBlank : url;
On 2015/02/23 22:17:55, mnaganov (cr) wrote:
> On 2015/02/23 21:28:04, Charlie Reis wrote:
> > I'm concerned about this code being different from desktop Chrome.  It seems
> > like we should just be resetting the address bar to GetVisibleEntry.  Making
> > this specific to about:blank seems different and potentially risky.
> >
> 
> I'll double-check, if we are allowed to pass null urls to clients. I'm afraid,
> we can't, that's why I have added this conversion.
>

I have checked that when executing WebView.loadUrl(""), onPageFinished receives
"about:blank", and this is what we should emulate here, so the conversion is
correct.
 
> In fact, GetVisibleEntry is what we retrieve under the hood. This is the
> sequence of calls: AwContents.getUrl() -> WebContents.getUrl() [Java] ->
> WebContentsAndroid::GetURL() [C++] -> WebContentsImpl::GetURL() ->
> GetVisibleEntry()->GetVirtualURL() 
>  
> > What's the reason we can't do the same thing as in desktop Chrome?  If we
need
> > to diverge, we should carefully reason about this.
> 
> So I guess, we are not diverging beyond what is required to conform to WebView
> API.
> 
> As for the reason, why we send events only for blank URLs, is to reduce
> possibility of synthesizing superfluous events that will confuse clients.
> 
> Do you think there are any other URLs besides empty / about:blank that should
be
> considered here?

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

https://codereview.chromium.org/924833003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_android.cc:483: jboolean
WebContentsAndroid::HasPageScriptAccessedInitialDocument(
On 2015/02/23 21:04:08, Ted C wrote:
> Exposing this is a layering violation of sorts.  WebContents.java "should"
only
> expose methods from web_contents.h, but this is publicly exposing a
> web_contents_impl.h method (actually a navigation_controller_delegate.h
method).
>  But we already do that so much in that file, so I guess you're not being
> inconsistent :-/
> 

I agree, this isn't good, but as I have noted in my initial message,
other options like exposing the 'hasAccessedInitialDocument' on the public
interface, or duplicating WebViewImpl's logic for handling this property appear
even worse to me.

> Why does this name differ from WebContentsImpl's method?  We try to keep them
in
> sync.

I though that a longer name is a bit more descriptive, but on the other hand,
having matching names simplifies searching, and it is easier to find the
original method, so changed the name to match the native one.

https://codereview.chromium.org/924833003/diff/80001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/924833003/diff/80001/content/content.gyp#newc...
content/content.gyp:502: 'source_file': 'public/browser/invalidate_type.h',
On 2015/02/23 22:17:55, mnaganov (cr) wrote:
> Note to self: we can now get rid of the manual mirror of these enums in
> WebContentsDelegateAndroid.java

Done.

[Charlie Reis, 2015-02-24 19:37:55]

Thanks for checking.  I tried to clarify my concern below.  We may be able to
proceed with this as is if necessary, but I think it might be worth making it
closer in spirit to the original defense.

https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:223:
url = TextUtils.isEmpty(url) ? aboutBlank : url;
On 2015/02/24 11:28:42, mnaganov (cr) wrote:
> On 2015/02/23 22:17:55, mnaganov (cr) wrote:
> > On 2015/02/23 21:28:04, Charlie Reis wrote:
> > > I'm concerned about this code being different from desktop Chrome.  It
seems
> > > like we should just be resetting the address bar to GetVisibleEntry. 
Making
> > > this specific to about:blank seems different and potentially risky.
> > >
> > 
> > I'll double-check, if we are allowed to pass null urls to clients. I'm
afraid,
> > we can't, that's why I have added this conversion.
> >
> 
> I have checked that when executing WebView.loadUrl(""), onPageFinished
receives
> "about:blank", and this is what we should emulate here, so the conversion is
> correct.

Sure, I'm not concerned about rewriting "" to about:blank.

>  
> > In fact, GetVisibleEntry is what we retrieve under the hood. This is the
> > sequence of calls: AwContents.getUrl() -> WebContents.getUrl() [Java] ->
> > WebContentsAndroid::GetURL() [C++] -> WebContentsImpl::GetURL() ->
> > GetVisibleEntry()->GetVirtualURL() 
> >  
> > > What's the reason we can't do the same thing as in desktop Chrome?  If we
> need
> > > to diverge, we should carefully reason about this.
> > 
> > So I guess, we are not diverging beyond what is required to conform to
WebView
> > API.
> > 
> > As for the reason, why we send events only for blank URLs, is to reduce
> > possibility of synthesizing superfluous events that will confuse clients.
> > 
> > Do you think there are any other URLs besides empty / about:blank that
should
> be
> > considered here? 
> 

I'm concerned about making the notification specific to about:blank, since
that's not really what GetVisibleEntry is doing.  I think it happens to be the
case that this will work, but it feels fragile to me.

GetVisibleEntry is returning the last committed entry if it's not safe to show
the pending entry.  In the HasAccessedInitialDocument case, I *think* the last
committed entry will always be about:blank.  If those ever differ, we'll be
reporting the wrong URL here.

Is it possible to change this code to use WebContents::GetLastCommittedURL()
instead?  We might be able to assert that it's equal to about:blank if that
matters, but it's closer to the intent of the spoof defense.

[mnaganov (inactive), 2015-02-25 11:33:19]

On 2015/02/24 19:37:55, Charlie Reis wrote:
> Thanks for checking.  I tried to clarify my concern below.  We may be able to
> proceed with this as is if necessary, but I think it might be worth making it
> closer in spirit to the original defense.
> 
>
https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
> File
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> (right):
> 
>
https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:223:
> url = TextUtils.isEmpty(url) ? aboutBlank : url;
> On 2015/02/24 11:28:42, mnaganov (cr) wrote:
> > On 2015/02/23 22:17:55, mnaganov (cr) wrote:
> > > On 2015/02/23 21:28:04, Charlie Reis wrote:
> > > > I'm concerned about this code being different from desktop Chrome.  It
> seems
> > > > like we should just be resetting the address bar to GetVisibleEntry. 
> Making
> > > > this specific to about:blank seems different and potentially risky.
> > > >
> > > 
> > > I'll double-check, if we are allowed to pass null urls to clients. I'm
> afraid,
> > > we can't, that's why I have added this conversion.
> > >
> > 
> > I have checked that when executing WebView.loadUrl(""), onPageFinished
> receives
> > "about:blank", and this is what we should emulate here, so the conversion is
> > correct.
> 
> Sure, I'm not concerned about rewriting "" to about:blank.
> 
> >  
> > > In fact, GetVisibleEntry is what we retrieve under the hood. This is the
> > > sequence of calls: AwContents.getUrl() -> WebContents.getUrl() [Java] ->
> > > WebContentsAndroid::GetURL() [C++] -> WebContentsImpl::GetURL() ->
> > > GetVisibleEntry()->GetVirtualURL() 
> > >  
> > > > What's the reason we can't do the same thing as in desktop Chrome?  If
we
> > need
> > > > to diverge, we should carefully reason about this.
> > > 
> > > So I guess, we are not diverging beyond what is required to conform to
> WebView
> > > API.
> > > 
> > > As for the reason, why we send events only for blank URLs, is to reduce
> > > possibility of synthesizing superfluous events that will confuse clients.
> > > 
> > > Do you think there are any other URLs besides empty / about:blank that
> should
> > be
> > > considered here? 
> > 
> 
> I'm concerned about making the notification specific to about:blank, since
> that's not really what GetVisibleEntry is doing.  I think it happens to be the
> case that this will work, but it feels fragile to me.
> 
> GetVisibleEntry is returning the last committed entry if it's not safe to show
> the pending entry.  In the HasAccessedInitialDocument case, I *think* the last
> committed entry will always be about:blank.  If those ever differ, we'll be
> reporting the wrong URL here.
> 
> Is it possible to change this code to use WebContents::GetLastCommittedURL()
> instead?  We might be able to assert that it's equal to about:blank if that
> matters, but it's closer to the intent of the spoof defense.

Thanks for the clarification, now I think I understand your concerns.

It looks like checking 'HasAccessedInitialDocument' flag is enough, as it is
always false after the first commit, which means GetLastCommittedURL must be
'empty' (I checked that it's really the case in the scenario we are testing). So
I removed the condition on "about:blank" which was bothering you, and switched
to reporting last committed url to the app in the synthesized callbacks, instead
of the visible url. Does that sound good?

[Charlie Reis, 2015-02-25 18:27:18]

On 2015/02/25 11:33:19, mnaganov (cr) wrote:
> On 2015/02/24 19:37:55, Charlie Reis wrote:
> > Thanks for checking.  I tried to clarify my concern below.  We may be able
to
> > proceed with this as is if necessary, but I think it might be worth making
it
> > closer in spirit to the original defense.
> > 
> >
>
https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
> > File
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:223:
> > url = TextUtils.isEmpty(url) ? aboutBlank : url;
> > On 2015/02/24 11:28:42, mnaganov (cr) wrote:
> > > On 2015/02/23 22:17:55, mnaganov (cr) wrote:
> > > > On 2015/02/23 21:28:04, Charlie Reis wrote:
> > > > > I'm concerned about this code being different from desktop Chrome.  It
> > seems
> > > > > like we should just be resetting the address bar to GetVisibleEntry. 
> > Making
> > > > > this specific to about:blank seems different and potentially risky.
> > > > >
> > > > 
> > > > I'll double-check, if we are allowed to pass null urls to clients. I'm
> > afraid,
> > > > we can't, that's why I have added this conversion.
> > > >
> > > 
> > > I have checked that when executing WebView.loadUrl(""), onPageFinished
> > receives
> > > "about:blank", and this is what we should emulate here, so the conversion
is
> > > correct.
> > 
> > Sure, I'm not concerned about rewriting "" to about:blank.
> > 
> > >  
> > > > In fact, GetVisibleEntry is what we retrieve under the hood. This is the
> > > > sequence of calls: AwContents.getUrl() -> WebContents.getUrl() [Java] ->
> > > > WebContentsAndroid::GetURL() [C++] -> WebContentsImpl::GetURL() ->
> > > > GetVisibleEntry()->GetVirtualURL() 
> > > >  
> > > > > What's the reason we can't do the same thing as in desktop Chrome?  If
> we
> > > need
> > > > > to diverge, we should carefully reason about this.
> > > > 
> > > > So I guess, we are not diverging beyond what is required to conform to
> > WebView
> > > > API.
> > > > 
> > > > As for the reason, why we send events only for blank URLs, is to reduce
> > > > possibility of synthesizing superfluous events that will confuse
clients.
> > > > 
> > > > Do you think there are any other URLs besides empty / about:blank that
> > should
> > > be
> > > > considered here? 
> > > 
> > 
> > I'm concerned about making the notification specific to about:blank, since
> > that's not really what GetVisibleEntry is doing.  I think it happens to be
the
> > case that this will work, but it feels fragile to me.
> > 
> > GetVisibleEntry is returning the last committed entry if it's not safe to
show
> > the pending entry.  In the HasAccessedInitialDocument case, I *think* the
last
> > committed entry will always be about:blank.  If those ever differ, we'll be
> > reporting the wrong URL here.
> > 
> > Is it possible to change this code to use WebContents::GetLastCommittedURL()
> > instead?  We might be able to assert that it's equal to about:blank if that
> > matters, but it's closer to the intent of the spoof defense.
> 
> Thanks for the clarification, now I think I understand your concerns.
> 
> It looks like checking 'HasAccessedInitialDocument' flag is enough, as it is
> always false after the first commit, which means GetLastCommittedURL must be
> 'empty' (I checked that it's really the case in the scenario we are testing).
So
> I removed the condition on "about:blank" which was bothering you, and switched
> to reporting last committed url to the app in the synthesized callbacks,
instead
> of the visible url. Does that sound good?

Yes!  That's just what I was looking for.  Thanks, LGTM.

https://codereview.chromium.org/924833003/diff/120001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/924833003/diff/120001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1250:
String url =  mWebContents.getLastCommittedUrl();
nit: Remove extra space after =.

[mnaganov (inactive), 2015-02-25 20:53:56]

On 2015/02/25 18:27:18, Charlie Reis wrote:
> On 2015/02/25 11:33:19, mnaganov (cr) wrote:
> > On 2015/02/24 19:37:55, Charlie Reis wrote:
> > > Thanks for checking.  I tried to clarify my concern below.  We may be able
> to
> > > proceed with this as is if necessary, but I think it might be worth making
> it
> > > closer in spirit to the original defense.
> > > 
> > >
> >
>
https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
> > > File
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/924833003/diff/80001/android_webview/java/src...
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:223:
> > > url = TextUtils.isEmpty(url) ? aboutBlank : url;
> > > On 2015/02/24 11:28:42, mnaganov (cr) wrote:
> > > > On 2015/02/23 22:17:55, mnaganov (cr) wrote:
> > > > > On 2015/02/23 21:28:04, Charlie Reis wrote:
> > > > > > I'm concerned about this code being different from desktop Chrome. 
It
> > > seems
> > > > > > like we should just be resetting the address bar to GetVisibleEntry.

> > > Making
> > > > > > this specific to about:blank seems different and potentially risky.
> > > > > >
> > > > > 
> > > > > I'll double-check, if we are allowed to pass null urls to clients. I'm
> > > afraid,
> > > > > we can't, that's why I have added this conversion.
> > > > >
> > > > 
> > > > I have checked that when executing WebView.loadUrl(""), onPageFinished
> > > receives
> > > > "about:blank", and this is what we should emulate here, so the
conversion
> is
> > > > correct.
> > > 
> > > Sure, I'm not concerned about rewriting "" to about:blank.
> > > 
> > > >  
> > > > > In fact, GetVisibleEntry is what we retrieve under the hood. This is
the
> > > > > sequence of calls: AwContents.getUrl() -> WebContents.getUrl() [Java]
->
> > > > > WebContentsAndroid::GetURL() [C++] -> WebContentsImpl::GetURL() ->
> > > > > GetVisibleEntry()->GetVirtualURL() 
> > > > >  
> > > > > > What's the reason we can't do the same thing as in desktop Chrome? 
If
> > we
> > > > need
> > > > > > to diverge, we should carefully reason about this.
> > > > > 
> > > > > So I guess, we are not diverging beyond what is required to conform to
> > > WebView
> > > > > API.
> > > > > 
> > > > > As for the reason, why we send events only for blank URLs, is to
reduce
> > > > > possibility of synthesizing superfluous events that will confuse
> clients.
> > > > > 
> > > > > Do you think there are any other URLs besides empty / about:blank that
> > > should
> > > > be
> > > > > considered here? 
> > > > 
> > > 
> > > I'm concerned about making the notification specific to about:blank, since
> > > that's not really what GetVisibleEntry is doing.  I think it happens to be
> the
> > > case that this will work, but it feels fragile to me.
> > > 
> > > GetVisibleEntry is returning the last committed entry if it's not safe to
> show
> > > the pending entry.  In the HasAccessedInitialDocument case, I *think* the
> last
> > > committed entry will always be about:blank.  If those ever differ, we'll
be
> > > reporting the wrong URL here.
> > > 
> > > Is it possible to change this code to use
WebContents::GetLastCommittedURL()
> > > instead?  We might be able to assert that it's equal to about:blank if
that
> > > matters, but it's closer to the intent of the spoof defense.
> > 
> > Thanks for the clarification, now I think I understand your concerns.
> > 
> > It looks like checking 'HasAccessedInitialDocument' flag is enough, as it is
> > always false after the first commit, which means GetLastCommittedURL must be
> > 'empty' (I checked that it's really the case in the scenario we are
testing).
> So
> > I removed the condition on "about:blank" which was bothering you, and
switched
> > to reporting last committed url to the app in the synthesized callbacks,
> instead
> > of the visible url. Does that sound good?
> 
> Yes!  That's just what I was looking for.  Thanks, LGTM.
> 
>
https://codereview.chromium.org/924833003/diff/120001/android_webview/java/sr...
> File android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
> 
>
https://codereview.chromium.org/924833003/diff/120001/android_webview/java/sr...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:1250:
> String url =  mWebContents.getLastCommittedUrl();
> nit: Remove extra space after =.

Thanks, Charlie, I will fix that before submitting!

davidben@: David, can you please approve now?

[davidben, 2015-02-25 20:56:32]

On 2015/02/25 20:53:56, mnaganov (cr) wrote:
> davidben@: David, can you please approve now?

Right, I suppose I could have done that earlier. //net lgtm. (Charlie already
gives you //content coverage, so I'll defer to him.)

[mnaganov (inactive), 2015-02-26 09:53:25]

https://codereview.chromium.org/924833003/diff/120001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/924833003/diff/120001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1250:
String url =  mWebContents.getLastCommittedUrl();
On 2015/02/25 18:27:18, Charlie Reis wrote:
> nit: Remove extra space after =.

Done.

[mnaganov (inactive), 2015-02-26 09:57:18]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-02-26 09:57:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, torne@chromium.org, creis@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/924833003/#ps140001
(title: "Removed extra space")

[commit-bot: I haz the power, 2015-02-26 09:57:29]

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

[commit-bot: I haz the power, 2015-02-26 12:23:16]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-26 12:24:07]

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

[grt (UTC plus 2), 2015-02-26 14:25:39]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/958933002/ by grt@chromium.org.

The reason for reverting is: Reverting to see if this caused
AwContentsClientFullScreenTest#testPowerSaveBlockerIsTransferredToFullscreen
failure here: http://crbug.com/462213..

[mnaganov (inactive), 2015-02-26 14:36:21]

On 2015/02/26 14:25:39, grt wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
> https://codereview.chromium.org/958933002/ by mailto:grt@chromium.org.
> 
> The reason for reverting is: Reverting to see if this caused
> AwContentsClientFullScreenTest#testPowerSaveBlockerIsTransferredToFullscreen
> failure here: http://crbug.com/462213..

Yeah, I was investigating this in the meantime -- it looks like the test that
has become flaky actually needs to be fixed, so I will reland an updated patch
later.