Notify the renderer if a history navigation has no subframe items.

Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088, 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65
Cr-Commit-Position: refs/heads/master@{#420486}

[Charlie Reis, 2016-09-07 04:33:10]

Description was changed from

==========
Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088
TEST=Restore chrome://settings after disabling MD settings mode.
==========

to

==========
Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-09-07 04:33:48]

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

[commit-bot: I haz the power, 2016-09-07 04:33:59]

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-07 05:00:54]

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

[commit-bot: I haz the power, 2016-09-07 05:00:54]

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-08 03:35:59]

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

[commit-bot: I haz the power, 2016-09-08 03:36:15]

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-08 03:39:01]

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

[commit-bot: I haz the power, 2016-09-08 03:39:02]

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_...)

[Charlie Reis, 2016-09-08 04:05:06]

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

[commit-bot: I haz the power, 2016-09-08 04:05:23]

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-08 05:17:32]

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

[commit-bot: I haz the power, 2016-09-08 05:17:33]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-21 21:08:55]

Description was changed from

==========
Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088, 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-09-21 21:08:55]

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

[Charlie Reis, 2016-09-21 21:09:50]

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

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

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-21 21:17:49]

Alex, would you be interested in reviewing this one?  It's a step towards fixing
https://crbug.com/639842 so that we don't need to make more trips to the browser
process than necessary for subframes during a history navigation.

As we get further, we can send all the same-process subframe PageStates, which
will let us get rid of some other stopLoading hacks we have for about:blank
frames.  This first step just tells the renderer if the PageState has no
subframes at all, in which case it can load any iframes directly without asking
the browser.  It helps if bad PageStates were written to disk, or if the page
changed (and added frames) since the last visit.

Specifically, this will help the MD settings folks, since they have a Finch
trial which switches between a version of the page with frames and a version
without.

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:1107: bool
browser_has_subtree_history_items_;
Figuring out where (and how long) to store this is the trickiest part. 
Subframes need to inherit this info (unlike NavigationState), and it's only
useful during the initial navigation.  I think putting it directly on
RenderFrame and clearing it at didStopLoading should be sufficient.

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

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

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

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-09-22 01:44:06]

Thanks!  LGTM with nit and a few small questions.

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3684:
new_controller.Restore(entries.size() - 1,
Is there a reason why the restore has to be in a new tab?  (wondering since
nothing's been loaded in the current tab yet.)

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3700: //
Verify subframe entries if they're enabled (e.g. in --site-per-process).
Since they're enabled everywhere now, how long until we can start
unconditionally checking the FrameNavigationEntries in tests? :)

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:853: return tree_node ?
!tree_node->children.empty() : false;
nit: just tree_node && !tree_node->children.empty()?

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

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:140: false,                  
// has_subtree_history_items
Just for my own understanding: why can we assume this is always false?  Would
something like history.back() be treated as a browser-initiated navigation?

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1046:
browser_has_subtree_history_items_(false),
I was thinking whether "true" might be a safer default, given that the previous
behavior was to always ask the browser.  But I guess it's doesn't really matter
given that this will always be set for history navigations before any child
frames are created.

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4847: browser_has_subtree_history_items_ =
false;
This will be reset in all frames, since main frames and subframes all will
eventually fire didStopLoading(), right?  What if we have a main frame with a
subframe, the subframe finishes loading and then creates a new (grandchild)
frame while the main frame hasn't finished loading?  The grandchild is not
considered part of the current history navigation, right?

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:1107: bool
browser_has_subtree_history_items_;
On 2016/09/21 21:17:49, Charlie Reis (slow) wrote:
> Figuring out where (and how long) to store this is the trickiest part. 
> Subframes need to inherit this info (unlike NavigationState), and it's only
> useful during the initial navigation.  I think putting it directly on
> RenderFrame and clearing it at didStopLoading should be sufficient.

Acknowledged.

[Charlie Reis, 2016-09-22 20:59:45]

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

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

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-22 21:00:36]

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

[Charlie Reis, 2016-09-22 21:00:37]

Thanks!  Fixes and answers below.

Nasko, can you review frame_messages.h for IPC owners?

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3684:
new_controller.Restore(entries.size() - 1,
On 2016/09/22 01:44:06, alexmos wrote:
> Is there a reason why the restore has to be in a new tab?  (wondering since
> nothing's been loaded in the current tab yet.)

Good call.  Done.

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3700: //
Verify subframe entries if they're enabled (e.g. in --site-per-process).
On 2016/09/22 01:44:06, alexmos wrote:
> Since they're enabled everywhere now, how long until we can start
> unconditionally checking the FrameNavigationEntries in tests? :)

We plan to rip out the old path in M56.  That makes changes easier to test and
merge if we discover any showstoppers.

(And yes, I can't wait!)  :)

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:853: return tree_node ?
!tree_node->children.empty() : false;
On 2016/09/22 01:44:06, alexmos wrote:
> nit: just tree_node && !tree_node->children.empty()?

Done.

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

https://codereview.chromium.org/2316003002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:140: false,                  
// has_subtree_history_items
On 2016/09/22 01:44:06, alexmos wrote:
> Just for my own understanding: why can we assume this is always false?  Would
> something like history.back() be treated as a browser-initiated navigation? 

Good question, and yes.  Renderer-initiated history navigations get punted to
the browser process via a GoToEntryAtOffset IPC message, which then goes through
the browser-initiated back/forward path.  (In some sense, it's not great that a
renderer can cause a "browser initiated" navigation, but it's only to an entry
the user has already been to.)

Just to be sure, I tried it in --enable-browser-side-navigation and verified
that history.back() goes through NavigationRequest::CreateBrowserInitiated, not
here.

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1046:
browser_has_subtree_history_items_(false),
On 2016/09/22 01:44:06, alexmos wrote:
> I was thinking whether "true" might be a safer default, given that the
previous
> behavior was to always ask the browser.  But I guess it's doesn't really
matter
> given that this will always be set for history navigations before any child
> frames are created.

Right.  I think this is a smaller delta from where we're heading, when the
browser will tell us exactly which frame unique names it should be consulted for
during a history navigation.

https://codereview.chromium.org/2316003002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4847: browser_has_subtree_history_items_ =
false;
On 2016/09/22 01:44:06, alexmos wrote:
> This will be reset in all frames, since main frames and subframes all will
> eventually fire didStopLoading(), right?  What if we have a main frame with a
> subframe, the subframe finishes loading and then creates a new (grandchild)
> frame while the main frame hasn't finished loading?  The grandchild is not
> considered part of the current history navigation, right?

Exactly.  The newly created child frame checks to see if its parent is still
loading before trying to find a history item for itself.  See this comment/code
in FrameLoaderClientImpl for where it happens:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/FrameLoade...

As a side note, didStopLoading happens bottom up.  A frame won't hear it until
all of its subframes are done, so it's possible for the main frame to keep its
bit around much longer than children/grandchildren.

[alexmos, 2016-09-22 21:09:39]

Thanks for the answers! LGTM.

https://codereview.chromium.org/2316003002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2316003002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3680:
RestoreType::LAST_SESSION_EXITED_CLEANLY, &entries);
nit: indent

[Charlie Reis, 2016-09-22 21:14:39]

Oops, thanks!

https://codereview.chromium.org/2316003002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2316003002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3680:
RestoreType::LAST_SESSION_EXITED_CLEANLY, &entries);
On 2016/09/22 21:09:39, alexmos wrote:
> nit: indent

Done.

[Charlie Reis, 2016-09-22 21:14:43]

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

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

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-22 21:40:34]

IPC LGTM

[Charlie Reis, 2016-09-22 21:44:13]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-09-22 21:44:21]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-09-22 21:44:21]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2316003002/#ps140001
(title: "Fix indent.")

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

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-22 22:47:33]

Description was changed from

==========
Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088, 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088, 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-22 22:47:35]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-22 22:50:07]

Description was changed from

==========
Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088, 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG=638088, 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65
Cr-Commit-Position: refs/heads/master@{#420486}
==========

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

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