Remove IsSynchronous API from NavigationHandle.

Remove IsSynchronous API from NavigationHandle.

The IsSynchronous API on NavigationHandle is almost identical to
IsSamePage and there is no reason for the two to exist. Also, IsSamePage
can be based on the same implementation as IsSynchronous, which allows
it to be called at any point in the lifetime of NavigationHandle.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2e9de94b320e23d920de86114648cb5e851a2fd5
Cr-Commit-Position: refs/heads/master@{#428226}

[nasko, 2016-09-16 21:19:51]

Description was changed from

==========
Remove IsSynchronous API from NavigationHandle.

The IsSynchronous API on NavigationHandle is almost identical to
IsSamePage and there is no reason for the two to exist. Also, IsSamePage
can be based on the same implementation as IsSynchronous, which allows
it to be called at any point in the lifetime of NavigationHandle.

BUG=
==========

to

==========
Remove IsSynchronous API from NavigationHandle.

The IsSynchronous API on NavigationHandle is almost identical to
IsSamePage and there is no reason for the two to exist. Also, IsSamePage
can be based on the same implementation as IsSynchronous, which allows
it to be called at any point in the lifetime of NavigationHandle.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-09-16 21:21:40]

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

[commit-bot: I haz the power, 2016-09-16 21:22:16]

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-09-16 22:03:17]

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

[commit-bot: I haz the power, 2016-09-16 22:03:18]

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

[nasko, 2016-09-16 22:12:41]

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

[nasko, 2016-09-16 22:12:41]

Hey Charlie,
Can you review this CL for me? There are some tests I need to triage, but the
gist of the work should be there.
Thanks in advance!
Nasko

[Charlie Reis, 2016-09-16 22:23:53]

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

[Charlie Reis, 2016-09-16 22:23:53]

Cool.  Adding clamy@ as well so she can sanity check it.

I think this will help a lot with WebContentsObservers who listen to
DidStartNavigation, wondering if the document is going to change.  Being able to
call IsSamePage at that time will be useful.

A few questions and comments below.

https://codereview.chromium.org/2345053006/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/2345053006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:272: if
(navigation_handle->IsSamePage() ||
Is this a desirable behavior change, assuming that it doesn't return early for
about:blank anymore?  Or did you find that IsSynchronousNavigation was false for
about:blank?

https://codereview.chromium.org/2345053006/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (left):

https://codereview.chromium.org/2345053006/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:203: DCHECK(state_ ==
DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE)
Yes, this is exactly what I hoped for.  It should be possible to call IsSamePage
at DidStartnavigation so that callers can understand if they will be getting a
different document or not.

https://codereview.chromium.org/2345053006/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:197: return is_same_page_
&& url_ != GURL(url::kAboutBlankURL);
Hmm, I don't think it makes sense for is_same_page_ to be true for about:blank,
even if IsSamePage() returns the right value.  It seems too risky for other code
in NavigationHandleImpl to do the wrong thing by checking is_same_page_
directly.

Is there another way to handle this?

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1221:
validated_params.was_within_same_page,
I don't understand this change.  When is this false?

[nasko, 2016-09-16 23:38:11]

https://codereview.chromium.org/2345053006/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/2345053006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:272: if
(navigation_handle->IsSamePage() ||
On 2016/09/16 22:23:53, Charlie Reis (slow) wrote:
> Is this a desirable behavior change, assuming that it doesn't return early for
> about:blank anymore?  Or did you find that IsSynchronousNavigation was false
for
> about:blank?

It doesn't account for about:blank anymore, so that should be 1:1 with the code
before.

https://codereview.chromium.org/2345053006/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (left):

https://codereview.chromium.org/2345053006/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:203: DCHECK(state_ ==
DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE)
On 2016/09/16 22:23:53, Charlie Reis (slow) wrote:
> Yes, this is exactly what I hoped for.  It should be possible to call
IsSamePage
> at DidStartnavigation so that callers can understand if they will be getting a
> different document or not.

Acknowledged.

https://codereview.chromium.org/2345053006/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:197: return is_same_page_
&& url_ != GURL(url::kAboutBlankURL);
On 2016/09/16 22:23:53, Charlie Reis (slow) wrote:
> Hmm, I don't think it makes sense for is_same_page_ to be true for
about:blank,
> even if IsSamePage() returns the right value.  It seems too risky for other
code
> in NavigationHandleImpl to do the wrong thing by checking is_same_page_
> directly.
> 
> Is there another way to handle this?

After poking at this a bit more, I think I don't need the about:blank
restriction, so I'm very happy to remove it.

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:447: DCHECK_EQ(same_page,
IsSamePage());
Sadly, I think I will have to lose this DCHECK due to
NavigationTypeClassification_On1InPageTo2While2Pending.
It creates page1, while there, it starts a load to page2, but the network is
blocked on it. That creates a NavigationHandle for page2 with IsSamePage set to
false. The test then does a pushState for the exact same URL for page2, which
sends a commit IPC. Since the URLs match exactly, we keep the created
NavigationHandle and commit the URL, however same_page is true because it was a
pushState navigation.

Any ideas on how to disambiguate this case? FWIW, it works correctly in
PlzNavigate mode :).

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1221:
validated_params.was_within_same_page,
On 2016/09/16 22:23:53, Charlie Reis (slow) wrote:
> I don't understand this change.  When is this false?

Unfortunately, there are some cases where this is false:
* loading of data URL with base URL - the URL in the handle is the base URL and
the committed URL is the real data URL.
* blocking a document load due to CSP - the URL in the handle is the real URL we
started loading, but when blocked, we load "data:," into the frame and that
comes as the committed URL.

These cases cause the code above, on line 1208, to reset the existing
navigation_handle_. So going into this branch, we assume it is a synchronous
load, even though it isn't. Using validated_params.was_within_same_page accounts
for those cases as it has the correct value passed by the renderer.

[nasko, 2016-09-16 23:38:19]

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

[commit-bot: I haz the power, 2016-09-16 23:38:41]

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

[Charlie Reis, 2016-09-16 23:59:16]

Thanks!  And a few more questions.  Sorry, didn't realize this request would
turn out to be so tricky.  :)

https://codereview.chromium.org/2345053006/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/2345053006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:272: if
(navigation_handle->IsSamePage() ||
On 2016/09/16 23:38:11, nasko (slow) wrote:
> On 2016/09/16 22:23:53, Charlie Reis (slow) wrote:
> > Is this a desirable behavior change, assuming that it doesn't return early
for
> > about:blank anymore?  Or did you find that IsSynchronousNavigation was false
> for
> > about:blank?
> 
> It doesn't account for about:blank anymore, so that should be 1:1 with the
code
> before.

Hmm, I don't understand.

Was it the case that IsSynchronousNavigation ever returned true for about:blank?
 (I thought so, but maybe I'm wrong.  I often get this case wrong, because I
know some about:blank navigations go through NavigationScheduler and are thus
async.  I thought there were some sync cases, though-- maybe Daniel knows?)

If so, then WebNavigationTabObserver would have early returned for about:blank. 
After this CL, it won't.  Is that ok?

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:447: DCHECK_EQ(same_page,
IsSamePage());
On 2016/09/16 23:38:11, nasko (slow) wrote:
> Sadly, I think I will have to lose this DCHECK due to
> NavigationTypeClassification_On1InPageTo2While2Pending.
> It creates page1, while there, it starts a load to page2, but the network is
> blocked on it. That creates a NavigationHandle for page2 with IsSamePage set
to
> false. The test then does a pushState for the exact same URL for page2, which
> sends a commit IPC. Since the URLs match exactly, we keep the created
> NavigationHandle and commit the URL, however same_page is true because it was
a
> pushState navigation.

Aaaa!  This is why comparing URLs is never a good way to track things.  :(

I'm a bit concerned about this case, because we're clearly using a different
navigation's handle for the same-page commit.  Is there any other state on the
handle that might be incorrect when this happens?  For example, what if the slow
one was a POST?  Would the handle's resource_request_body_ get persisted in the
NavEntry for the same-page navigation?  That seems bad.

Seems better if we can create a new NavigationHandle for the pushState here,
even if the URL matches the old handle.  I don't know how to detect that,
though.

> 
> Any ideas on how to disambiguate this case? FWIW, it works correctly in
> PlzNavigate mode :).

How does PlzNavigate get it right?

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1221:
validated_params.was_within_same_page,
On 2016/09/16 23:38:11, nasko (slow) wrote:
> On 2016/09/16 22:23:53, Charlie Reis (slow) wrote:
> > I don't understand this change.  When is this false?
> 
> Unfortunately, there are some cases where this is false:
> * loading of data URL with base URL - the URL in the handle is the base URL
and
> the committed URL is the real data URL.
> * blocking a document load due to CSP - the URL in the handle is the real URL
we
> started loading, but when blocked, we load "data:," into the frame and that
> comes as the committed URL.
> 
> These cases cause the code above, on line 1208, to reset the existing
> navigation_handle_. So going into this branch, we assume it is a synchronous
> load, even though it isn't. Using validated_params.was_within_same_page
accounts
> for those cases as it has the correct value passed by the renderer.

Thanks!  That clarifies it.  Can you update the comment on line 1211, since this
block is not just about synchronous navigations?

[commit-bot: I haz the power, 2016-09-17 00:02:38]

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

[commit-bot: I haz the power, 2016-09-17 00:02:39]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[clamy, 2016-09-19 11:09:50]

Thanks! There seems to be an issue with the case where the renderer tells us the
navigation was within the same-page, but the browser computes it actually
wasn't.

https://codereview.chromium.org/2345053006/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:447: DCHECK_EQ(same_page,
IsSamePage());
It seems tests are hitting this DCHECK. I think this is because we recompute
whether the navigation was same-page in the NavigatorImpl, and call
DidCommitNavigation with this computed value instead of the one given by the
renderer.

[Charlie Reis, 2016-09-20 22:06:25]

Quick note after we just stepped through this in person.

https://codereview.chromium.org/2345053006/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1189:
(navigation_handle_->GetURL() != validated_params.url)) {
Note: Add a check for whether navigation_handle_->IsSamePage() agrees with
validated_params.was_within_same_page.  That should fix
NavigationTypeClassification_On1InPageTo1While1Pending.

[nasko, 2016-09-20 22:58:10]

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

[commit-bot: I haz the power, 2016-09-20 22:58:49]

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

[nasko, 2016-09-20 23:11:20]

https://codereview.chromium.org/2345053006/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/2345053006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:272: if
(navigation_handle->IsSamePage() ||
On 2016/09/16 23:59:15, Charlie Reis (slow) wrote:
> On 2016/09/16 23:38:11, nasko (slow) wrote:
> > On 2016/09/16 22:23:53, Charlie Reis (slow) wrote:
> > > Is this a desirable behavior change, assuming that it doesn't return early
> for
> > > about:blank anymore?  Or did you find that IsSynchronousNavigation was
false
> > for
> > > about:blank?
> > 
> > It doesn't account for about:blank anymore, so that should be 1:1 with the
> code
> > before.
> 
> Hmm, I don't understand.
> 
> Was it the case that IsSynchronousNavigation ever returned true for
about:blank?

I don't think that was ever the case.

>  (I thought so, but maybe I'm wrong.  I often get this case wrong, because I
> know some about:blank navigations go through NavigationScheduler and are thus
> async.  I thought there were some sync cases, though-- maybe Daniel knows?)

This is why I added explicit tests for that in
NavigationHandleImplBrowserTest::VerifySamePage. It tests a brand new iframe
being added with no src attribute, which should account for the initial blank
page. Also it has a case that it navigates a frame with some session history to
about:blank explicitly, which should cover the other cases.

> If so, then WebNavigationTabObserver would have early returned for
about:blank. 
> After this CL, it won't.  Is that ok?

Since even the initial empty document emits DidStartProvisionalLoad, it should
never have been the case that about:blank would get the
IsSynchronousNavigation() return value to be true.

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:447: DCHECK_EQ(same_page,
IsSamePage());
On 2016/09/16 23:59:15, Charlie Reis (slow) wrote:
> On 2016/09/16 23:38:11, nasko (slow) wrote:
> > Sadly, I think I will have to lose this DCHECK due to
> > NavigationTypeClassification_On1InPageTo2While2Pending.
> > It creates page1, while there, it starts a load to page2, but the network is
> > blocked on it. That creates a NavigationHandle for page2 with IsSamePage set
> to
> > false. The test then does a pushState for the exact same URL for page2,
which
> > sends a commit IPC. Since the URLs match exactly, we keep the created
> > NavigationHandle and commit the URL, however same_page is true because it
was
> a
> > pushState navigation.
> 
> Aaaa!  This is why comparing URLs is never a good way to track things.  :(
> 
> I'm a bit concerned about this case, because we're clearly using a different
> navigation's handle for the same-page commit.  Is there any other state on the
> handle that might be incorrect when this happens?  For example, what if the
slow
> one was a POST?  Would the handle's resource_request_body_ get persisted in
the
> NavEntry for the same-page navigation?  That seems bad.
> 
> Seems better if we can create a new NavigationHandle for the pushState here,
> even if the URL matches the old handle.  I don't know how to detect that,
> though.

As we discussed in person, I am adding in the mix of checks whether the
NavigationHandle and the renderer process agree on the value of "is same page
navigation". If there is a disagreement, then the old NavigationHandle will be
cleared and a new one will be created.

The eventual commit of the slow URL will create a new NavigationHandle and fire
the set of Start/Finish notifications. However, that does mean that the first
handle that was initially created will get a failed navigation, which isn't
quite true, but probably the safer of the two possible outcomes.

> > Any ideas on how to disambiguate this case? FWIW, it works correctly in
> > PlzNavigate mode :).
> 
> How does PlzNavigate get it right?

PlzNavigate gets it mostly right, since the slow loading NavigationHandle is
owned by the NavigationRequest until we get the response from the network. At
the time we send the Commit IPC down to the RenderFrame, we transfer the
ownership to the RFH. The race is still possible, but the window for it is much
smaller.

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1221:
validated_params.was_within_same_page,
On 2016/09/16 23:59:16, Charlie Reis (slow) wrote:
> On 2016/09/16 23:38:11, nasko (slow) wrote:
> > On 2016/09/16 22:23:53, Charlie Reis (slow) wrote:
> > > I don't understand this change.  When is this false?
> > 
> > Unfortunately, there are some cases where this is false:
> > * loading of data URL with base URL - the URL in the handle is the base URL
> and
> > the committed URL is the real data URL.
> > * blocking a document load due to CSP - the URL in the handle is the real
URL
> we
> > started loading, but when blocked, we load "data:," into the frame and that
> > comes as the committed URL.
> > 
> > These cases cause the code above, on line 1208, to reset the existing
> > navigation_handle_. So going into this branch, we assume it is a synchronous
> > load, even though it isn't. Using validated_params.was_within_same_page
> accounts
> > for those cases as it has the correct value passed by the renderer.
> 
> Thanks!  That clarifies it.  Can you update the comment on line 1211, since
this
> block is not just about synchronous navigations?

Tried to clarify both this comment and the one above.

https://codereview.chromium.org/2345053006/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:447: DCHECK_EQ(same_page,
IsSamePage());
On 2016/09/19 11:09:50, clamy wrote:
> It seems tests are hitting this DCHECK. I think this is because we recompute
> whether the navigation was same-page in the NavigatorImpl, and call
> DidCommitNavigation with this computed value instead of the one given by the
> renderer.

Actually, the reason was different - it was not resetting the NavigationHandle
in cases where it needed to. Fixed by the new check in
RenderFrameHostImpl::OnDidCommitProvisionalLoad.

[commit-bot: I haz the power, 2016-09-20 23:21:16]

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

[commit-bot: I haz the power, 2016-09-20 23:21:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Reis, 2016-09-20 23:35:24]

Looks like we're down to just a few unit tests!  On the right track.

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:557:
ExecuteScript(root->child_at(0), "window.location.replace('#frag');"));
Wow, subtle!  I didn't realize you could location.replace to a fragment URL and
stay in page, but you can indeed.

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1179: // happen if an
ongoing slow same-process navigation is interwoven by a
nit: interwoven with

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1223:
validated_params.was_within_same_page, validated_params.is_srcdoc,
I agree with clamy@ that this can technically not match same_page in
DidCommitNavigation (after going through IsURLInPageNavigation), but that would
only happen in cases the renderer is killed anyway.  I *think* that's ok.

Actually, I wonder if that's why the PushStateWithoutPreviousEntry unit test is
failing.  (There's a line in that test output about trying to kill the
renderer.)

[nasko, 2016-09-28 18:29:47]

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

[commit-bot: I haz the power, 2016-09-28 18:30:14]

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

[nasko, 2016-09-28 18:43:56]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2016-10-14 18:34:21]

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

[commit-bot: I haz the power, 2016-10-14 18:35:20]

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-14 18:52:51]

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

[commit-bot: I haz the power, 2016-10-14 18:52:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nasko, 2016-10-14 20:00:25]

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

[commit-bot: I haz the power, 2016-10-14 20:00:50]

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-14 20:45:43]

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

[commit-bot: I haz the power, 2016-10-14 20:45:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2016-10-27 17:26:59]

Hey Charlie,
This is ready for another round of review after a long break. I've fixed all the
unit test failures by keeping track of error page commits in the
TestRenderFrameHost code, otherwise the code would incorrectly set the
"within_same_page" bit.
Can you take another look?
Thanks!
Nasko

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:557:
ExecuteScript(root->child_at(0), "window.location.replace('#frag');"));
On 2016/09/20 23:35:24, Charlie Reis wrote:
> Wow, subtle!  I didn't realize you could location.replace to a fragment URL
and
> stay in page, but you can indeed.

I was trying to have the shortest string I can have : ).

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1179: // happen if an
ongoing slow same-process navigation is interwoven by a
On 2016/09/20 23:35:24, Charlie Reis wrote:
> nit: interwoven with

Done.

https://codereview.chromium.org/2345053006/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1223:
validated_params.was_within_same_page, validated_params.is_srcdoc,
On 2016/09/20 23:35:24, Charlie Reis wrote:
> I agree with clamy@ that this can technically not match same_page in
> DidCommitNavigation (after going through IsURLInPageNavigation), but that
would
> only happen in cases the renderer is killed anyway.  I *think* that's ok.
> 
> Actually, I wonder if that's why the PushStateWithoutPreviousEntry unit test
is
> failing.  (There's a line in that test output about trying to kill the
> renderer.)

Acknowledged.

[nasko, 2016-10-27 20:19:16]

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

[commit-bot: I haz the power, 2016-10-27 20:20:13]

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

[Charlie Reis, 2016-10-27 20:51:24]

Thanks!  LGTM with nits.

https://codereview.chromium.org/2345053006/diff/240001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

https://codereview.chromium.org/2345053006/diff/240001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:78: // Load
kStartUrl. This ensures that any subsequent navigation to kHttpsUrl2
Why was this change necessary?  Maybe mention in the comment why kHttpUrl isn't
used?

https://codereview.chromium.org/2345053006/diff/240001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/2345053006/diff/240001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_security_state_model_client.cc:367: // and not for
synchronous navigations like reference fragments and
nit: s/synchronous/same-page/

https://codereview.chromium.org/2345053006/diff/240001/content/test/test_rend...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/2345053006/diff/240001/content/test/test_rend...
content/test/test_render_frame_host.cc:164:
GetLastCommittedURL().ReplaceComponents(replacements));
This replacements check seems wrong to me, but maybe it's sufficient for unit
tests.  (For example, it can't handle pushState, which is allowed to change the
URL to be anything else within the same origin.)

Maybe it's worth adding a comment as a warning to others?  (I certainly don't
want this pattern to be copy/pasted elsewhere.)  :)

[nasko, 2016-10-27 21:22:34]

https://codereview.chromium.org/2345053006/diff/240001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

https://codereview.chromium.org/2345053006/diff/240001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:78: // Load
kStartUrl. This ensures that any subsequent navigation to kHttpsUrl2
On 2016/10/27 20:51:24, Charlie Reis wrote:
> Why was this change necessary?  Maybe mention in the comment why kHttpUrl
isn't
> used?

Done.

https://codereview.chromium.org/2345053006/diff/240001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_security_state_model_client.cc (right):

https://codereview.chromium.org/2345053006/diff/240001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_security_state_model_client.cc:367: // and not for
synchronous navigations like reference fragments and
On 2016/10/27 20:51:24, Charlie Reis wrote:
> nit: s/synchronous/same-page/

Done.

https://codereview.chromium.org/2345053006/diff/240001/content/test/test_rend...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/2345053006/diff/240001/content/test/test_rend...
content/test/test_render_frame_host.cc:164:
GetLastCommittedURL().ReplaceComponents(replacements));
On 2016/10/27 20:51:24, Charlie Reis wrote:
> This replacements check seems wrong to me, but maybe it's sufficient for unit
> tests.  (For example, it can't handle pushState, which is allowed to change
the
> URL to be anything else within the same origin.)

Unit tests can definitely use some more love : ). This is also why the change of
start URL was needed in the captive portal unit tests.
My goal here was to make it as correct as I can in the context of the changes
I'm making. I don't think we can handle pushState with this method and will
likely require a separate one.

> Maybe it's worth adding a comment as a warning to others?  (I certainly don't
> want this pattern to be copy/pasted elsewhere.)  :)

Took a stab at adding a scary comment :).

[nasko, 2016-10-27 21:25:11]

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

[commit-bot: I haz the power, 2016-10-27 21:25:58]

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-27 22:11:04]

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

[commit-bot: I haz the power, 2016-10-27 22:11:05]

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

[nasko, 2016-10-27 22:43:39]

nasko@chromium.org changed reviewers:
+ sky@chromium.org

[nasko, 2016-10-27 22:43:40]

Hey Scott,
Can you do an OWNERS review of chrome/ for me?
Thanks in advance!
Nasko

[sky, 2016-10-27 23:15:32]

LGTM

[nasko, 2016-10-27 23:18:08]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-10-27 23:18:09]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2345053006/#ps260001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2016-10-27 23:19:09]

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 00:01:12]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-10-28 00:05:36]

Description was changed from

==========
Remove IsSynchronous API from NavigationHandle.

The IsSynchronous API on NavigationHandle is almost identical to
IsSamePage and there is no reason for the two to exist. Also, IsSamePage
can be based on the same implementation as IsSynchronous, which allows
it to be called at any point in the lifetime of NavigationHandle.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove IsSynchronous API from NavigationHandle.

The IsSynchronous API on NavigationHandle is almost identical to
IsSamePage and there is no reason for the two to exist. Also, IsSamePage
can be based on the same implementation as IsSynchronous, which allows
it to be called at any point in the lifetime of NavigationHandle.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2e9de94b320e23d920de86114648cb5e851a2fd5
Cr-Commit-Position: refs/heads/master@{#428226}
==========

[commit-bot: I haz the power, 2016-10-28 00:05:38]

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