Add methods to NavigationHandle to allow refactoring webNavigation to use it.

Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

The refactoring of webNavigation to use the methods added in this CL is in https://codereview.chromium.org/1670673003.
BUG=532666, 584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/918f1d4c00f6bb2f757e80af118f1785bd333ecf
Cr-Commit-Position: refs/heads/master@{#374478}

[nasko, 2016-02-04 18:03:16]

Description was changed from

==========
Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

BUG=532666
==========

to

==========
Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

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

[nasko, 2016-02-04 18:22:32]

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

[nasko, 2016-02-04 18:22:33]

Hey Charlie and Camille,
This CL contains the changes I need to make to NavigationHandle in order to be
able to use it for refactoring the webNavigation API implementation. Once I get
the refactoring CL up, I'll update this CL's description to point to it.
Can you start reviewing this one for me?
Thanks in advance!
Nasko

[Charlie Reis, 2016-02-04 20:46:04]

Glad to see this API expanding.  A few quick thoughts as you update the compile
failures.

Is there a NavigationHandle bug we can list in the CL description?  I also think
it would be useful to have a more specific bug for updating the Web Navigation
API than 532666.

Also, let's list the dependent CL in the CL description, so that it's easy to
see where these methods will be used.

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:231: // proper values are
specified for is_synchronous and is_srcdoc.
Will the new tests fail for PlzNavigate in the meantime?  I'm wondering if they
need to be disabled or how hard it would be to fix them here.

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1017: true,   //
is_synchronous
Is it safe to conclude this?  Above, we clear the NavHandle if the URL doesn't
match.  What if we're in that case for an async navigation?

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1018: false,  // is_srcdoc
Is it possible for srcdoc to come through this path, or is it necessarily not
synchronous?

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:44: // Whether the navigation is
taking place in a frame, which is a direct child
nit: in a frame that is a

(The current phrasing makes the reader stop at "in a frame" without realizing
that the second phrase is a qualifier.)

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:52: // * navigating to about:blank
Double check with dcheng on the about:blank case.  I can never keep track.

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:55: // Whether the navigation is for
iframe with srcdoc attribute.
nit: for an

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:58: // Whether the navigation has
encountered a server redirect or not.
Hmm, I'm not sure this should be in "available at navigation start time" group. 
Seems like we wouldn't know until later, and most of the methods in this section
don't change over time.

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:59: virtual bool WasRedirected() = 0;
Hmm, I always like to be explicit about server vs client redirect if I can,
since those are very different cases.  Maybe WasServerRedirect (if it
intentionally excludes the client redirect case)?

[nasko, 2016-02-04 23:35:12]

Description was changed from

==========
Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

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

to

==========
Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

The refactoring of webNavigation to use the methods added in this CL is in
https://codereview.chromium.org/1670673003.
BUG=532666, 584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-02-04 23:37:00]

I don't have a separate bug for NavigationHandle itself. I filed one for the
webNavigaion API refactor and added this one to the list. Also, as I mentioned
in my initial email, I've added the link to the other CL, in which webNavigation
is actually refactored to use it.

Comments addressed.

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:231: // proper values are
specified for is_synchronous and is_srcdoc.
On 2016/02/04 20:46:04, Charlie Reis wrote:
> Will the new tests fail for PlzNavigate in the meantime?  I'm wondering if
they
> need to be disabled or how hard it would be to fix them here.

The one failing is srcdoc, which I'm not sure is working properly in PlzNavigate
currently. The sync one should work, as the NavigationHandle isn't created
through this method, but rather at the DidCommitProvisionalLoad call, as there
is no resource to be loaded.

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1017: true,   //
is_synchronous
On 2016/02/04 20:46:04, Charlie Reis wrote:
> Is it safe to conclude this?  Above, we clear the NavHandle if the URL doesn't
> match.  What if we're in that case for an async navigation?

Based on the comments of the two blocks, it seems that we can only get here for
synchronous navigations. The block resetting the navigation_handle_ is explicit
that it is for cases of sync navigation interrupting an ongoing one. It must be
the case, otherwise we would've gotten DidStartProvisionalLoad, which will
replace the NavigationHandle with a new one and the URLs would match.

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1018: false,  // is_srcdoc
On 2016/02/04 20:46:04, Charlie Reis wrote:
> Is it possible for srcdoc to come through this path, or is it necessarily not
> synchronous?

I don't think srcdoc is synchronous. We do get DidStartProvisionalLoad for
those.

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:44: // Whether the navigation is
taking place in a frame, which is a direct child
On 2016/02/04 20:46:05, Charlie Reis wrote:
> nit: in a frame that is a
> 
> (The current phrasing makes the reader stop at "in a frame" without realizing
> that the second phrase is a qualifier.)

Done.

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:52: // * navigating to about:blank
On 2016/02/04 20:46:05, Charlie Reis wrote:
> Double check with dcheng on the about:blank case.  I can never keep track.

Removing for now, to ensure I'm not misleading.

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:55: // Whether the navigation is for
iframe with srcdoc attribute.
On 2016/02/04 20:46:05, Charlie Reis wrote:
> nit: for an

Done.

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:58: // Whether the navigation has
encountered a server redirect or not.
On 2016/02/04 20:46:05, Charlie Reis wrote:
> Hmm, I'm not sure this should be in "available at navigation start time"
group. 
> Seems like we wouldn't know until later, and most of the methods in this
section
> don't change over time.

Good point. It was clustered with the rest of the APIs I was adding and missed
the headings. Moved.

https://codereview.chromium.org/1667163002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:59: virtual bool WasRedirected() = 0;
On 2016/02/04 20:46:05, Charlie Reis wrote:
> Hmm, I always like to be explicit about server vs client redirect if I can,
> since those are very different cases.  Maybe WasServerRedirect (if it
> intentionally excludes the client redirect case)?

Done.

[Charlie Reis, 2016-02-05 18:09:41]

Thanks.  Looks good apart from the failing test!

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:231: // proper values are
specified for is_synchronous and is_srcdoc.
On 2016/02/04 23:36:59, nasko wrote:
> On 2016/02/04 20:46:04, Charlie Reis wrote:
> > Will the new tests fail for PlzNavigate in the meantime?  I'm wondering if
> they
> > need to be disabled or how hard it would be to fix them here.
> 
> The one failing is srcdoc, which I'm not sure is working properly in
PlzNavigate
> currently. The sync one should work, as the NavigationHandle isn't created
> through this method, but rather at the DidCommitProvisionalLoad call, as there
> is no resource to be loaded.

Great.  Do we need to add the srcdoc one to their test filter so we don't turn
them red?  (Just trying to be proactive, since we've been in those shoes.)

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1017: true,   //
is_synchronous
On 2016/02/04 23:36:59, nasko wrote:
> On 2016/02/04 20:46:04, Charlie Reis wrote:
> > Is it safe to conclude this?  Above, we clear the NavHandle if the URL
doesn't
> > match.  What if we're in that case for an async navigation?
> 
> Based on the comments of the two blocks, it seems that we can only get here
for
> synchronous navigations. The block resetting the navigation_handle_ is
explicit
> that it is for cases of sync navigation interrupting an ongoing one. It must
be
> the case, otherwise we would've gotten DidStartProvisionalLoad, which will
> replace the NavigationHandle with a new one and the URLs would match.

Acknowledged.

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1018: false,  // is_srcdoc
On 2016/02/04 23:36:59, nasko wrote:
> On 2016/02/04 20:46:04, Charlie Reis wrote:
> > Is it possible for srcdoc to come through this path, or is it necessarily
not
> > synchronous?
> 
> I don't think srcdoc is synchronous. We do get DidStartProvisionalLoad for
> those.

Ok, thanks.

[nasko, 2016-02-05 21:40:21]

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1667163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:231: // proper values are
specified for is_synchronous and is_srcdoc.
On 2016/02/05 18:09:40, Charlie Reis wrote:
> On 2016/02/04 23:36:59, nasko wrote:
> > On 2016/02/04 20:46:04, Charlie Reis wrote:
> > > Will the new tests fail for PlzNavigate in the meantime?  I'm wondering if
> > they
> > > need to be disabled or how hard it would be to fix them here.
> > 
> > The one failing is srcdoc, which I'm not sure is working properly in
> PlzNavigate
> > currently. The sync one should work, as the NavigationHandle isn't created
> > through this method, but rather at the DidCommitProvisionalLoad call, as
there
> > is no resource to be loaded.
> 
> Great.  Do we need to add the srcdoc one to their test filter so we don't turn
> them red?  (Just trying to be proactive, since we've been in those shoes.)

Done.

[Charlie Reis, 2016-02-06 00:21:39]

Thanks, LGTM.

[clamy, 2016-02-08 12:54:58]

Thanks! A few questions below.

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

https://codereview.chromium.org/1667163002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:236: bool is_synchronous_;
Can is_synchronous_ and is_srcdoc_ be made const? I don't think they are
supposed to change over the course of the navigation.

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

https://codereview.chromium.org/1667163002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:234: false,  // is_synchronous
Can a browser-initiated nav count as synchronous? If so, can you just get
whether it's synchronous or not using ShouldMakeNetworkRequestForURL? This
filter out about:blank, javascript urls, empty urls and data urls right now.

For the srcdoc thing, doesn't what is done in DidStartProvisionalLoad work here
(ie url.spec() == kAboutSrcDocURL)?

https://codereview.chromium.org/1667163002/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1667163002/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:63: // Retruns the FrameTreeNode ID
for the parent frame. If this navigation is
nit: s/retruns/returns

[nasko, 2016-02-08 16:58:55]

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

https://codereview.chromium.org/1667163002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:236: bool is_synchronous_;
On 2016/02/08 12:54:58, clamy wrote:
> Can is_synchronous_ and is_srcdoc_ be made const? I don't think they are
> supposed to change over the course of the navigation.

Done.

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

https://codereview.chromium.org/1667163002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:234: false,  // is_synchronous
On 2016/02/08 12:54:58, clamy wrote:
> Can a browser-initiated nav count as synchronous? If so, can you just get
> whether it's synchronous or not using ShouldMakeNetworkRequestForURL? This
> filter out about:blank, javascript urls, empty urls and data urls right now.

I don't think we can equivalate no network request with synchronous navigations.
I think data: URLs are not synchronous, but have to double check. Also, I recall
Charlie and Daniel talking recently that "about:blank" might not be synchronous
if it is not the first commit. To be on the safe side, I decided not to touch
this area in this CL and that's why it has a TODO.

> For the srcdoc thing, doesn't what is done in DidStartProvisionalLoad work
here
> (ie url.spec() == kAboutSrcDocURL)?

Browser initiated navigation cannot be to srcdoc, as it is an iframe attribute,
right?

https://codereview.chromium.org/1667163002/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1667163002/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:63: // Retruns the FrameTreeNode ID
for the parent frame. If this navigation is
On 2016/02/08 12:54:58, clamy wrote:
> nit: s/retruns/returns

Done.

[clamy, 2016-02-09 10:45:25]

Thanks! Lgtm.

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

https://codereview.chromium.org/1667163002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:234: false,  // is_synchronous
On 2016/02/08 16:58:55, nasko wrote:
> On 2016/02/08 12:54:58, clamy wrote:
> > Can a browser-initiated nav count as synchronous? If so, can you just get
> > whether it's synchronous or not using ShouldMakeNetworkRequestForURL? This
> > filter out about:blank, javascript urls, empty urls and data urls right now.
> 
> I don't think we can equivalate no network request with synchronous
navigations.
> I think data: URLs are not synchronous, but have to double check. Also, I
recall
> Charlie and Daniel talking recently that "about:blank" might not be
synchronous
> if it is not the first commit. To be on the safe side, I decided not to touch
> this area in this CL and that's why it has a TODO.
> 
> > For the srcdoc thing, doesn't what is done in DidStartProvisionalLoad work
> here
> > (ie url.spec() == kAboutSrcDocURL)?
> 
> Browser initiated navigation cannot be to srcdoc, as it is an iframe
attribute,
> right?

Acknowledged for the first concern.

For the second one, note that this method is also called in renderer-initiated
navigations. However we can leave the detection of srcdoc navigations for a
later patch.

[nasko, 2016-02-09 14:52:40]

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

[commit-bot: I haz the power, 2016-02-09 14:53:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1667163002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1667163002/60001

[commit-bot: I haz the power, 2016-02-09 16:23:07]

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

[commit-bot: I haz the power, 2016-02-09 16:23:07]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-02-09 20:03:52]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-02-09 20:03:52]

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/1667163002/#ps60001
(title: "Fixes based on Camille's review.")

[commit-bot: I haz the power, 2016-02-09 20:09:24]

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

[commit-bot: I haz the power, 2016-02-09 21:49:17]

Description was changed from

==========
Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

The refactoring of webNavigation to use the methods added in this CL is in
https://codereview.chromium.org/1670673003.
BUG=532666, 584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

The refactoring of webNavigation to use the methods added in this CL is in
https://codereview.chromium.org/1670673003.
BUG=532666, 584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-02-09 21:49:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-09 21:51:24]

Description was changed from

==========
Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

The refactoring of webNavigation to use the methods added in this CL is in
https://codereview.chromium.org/1670673003.
BUG=532666, 584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add methods to NavigationHandle to allow refactoring webNavigation to use it.

The webNavigation API is currently using WebContentsObserver APIs based
on RenderFrameHost. Both Site Isolation and PlzNavigate are changing how
navigation behaves and the latter requires the observers to move to use
the NavigationHandle API. However, the webNavigation code requires
functionality which isn't currently present and this CL adds that.

The refactoring of webNavigation to use the methods added in this CL is in
https://codereview.chromium.org/1670673003.
BUG=532666, 584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/918f1d4c00f6bb2f757e80af118f1785bd333ecf
Cr-Commit-Position: refs/heads/master@{#374478}
==========

[commit-bot: I haz the power, 2016-02-09 21:51:26]

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