New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on: View detail
issue 273710
issue 853705



Sign in to add a comment
link

Issue 803859: NavigationController refactoring

Reported by clamy@chromium.org, Jan 19 2018 Project Member

Issue description

I have seen an issue come up in several reviews: someone working on a feature needs to pass navigation parameters from OpenURLParams. Currently, the only way do that is to stock them in the NavigationEntry, which makes little sense if the parameters are intended for one navigation only.

Instead, let's create the NavigationRequest at the same time we create the NavigationEntry. We'll keep navigation parameters that are persistent in the NavigationEntry, and remove the rest since we'll know be able to create our NavigationParams at the right time as we create the NavigationRequest. This way, NavigationEntry gets back to representing state that persists accross navigations (which had become murkier), while NavigationRequest holds the per-navigation state (which is destroyed at commit time).

We can also take advantage of this to keep a stronger link between the NavigationEntry and the NavigationRequest that manages the navigation to this particular entry. In particular, if all NavigationRequests handling a navigation to the pending NavigationEntry have been destroyed, then we should reset the pending NavigationEntry. If we make this link explicit (eg by having a map or having NavigationEntry keep a weak_ptr to its associated NavigationRequests), I'm hoping that we can make the pending NavigationEntry deletion clearer. See  issue 760342  for the latest issues with pending NavigationEntry deletion. The current state is that we can go from NavigationRequest to NavigationEntry, but not the reverse. There appear to be some edge cases where we'd end up with two NavigationRequests for the same NavigationEntry (in reloads for example).

nasko, creis: wdyt of this idea? Note that this require the extension of the NavigationRequest lifetime until commit, but this should land soon in https://chromium-review.googlesource.com/c/chromium/src/+/808974.
 

Comment 1 by nasko@chromium.org, Jan 20 2018

Cc: alex...@chromium.org
Why not be even more ambitious and just try to kill the whole notion of pending NavigationEntry? NavigationRequest is what we really need in practice - something that describes an ongoing navigation, which hasn't committed. Once the NavigationRequest is committed, we just make a new NavigaionEntry based on the NavigationRequest and add it to session history. I'd love if we can get to this simple model. If we cannot for some reason, what you described above sounds good to me, but I defer to creis@ on that area of the code.

Comment 2 by creis@chromium.org, Jan 20 2018

Cc: mkwst@chromium.org
Components: UI>Browser>Navigation
Labels: M-66 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: clamy@chromium.org
Status: Assigned (was: Untriaged)
I love both ideas.  Let's keep them separate, though.  :)  Camille's idea to create the NavigationRequest alongside the NavigationEntry sounds like it's needed (or at least highly useful) for issue 803365, and it should be easy to reason about as a step on its own.  I really like that it will eliminate a ton of things from NavigationEntry which otherwise get cleared in ResetForCommit.

Once that's done, we can investigate removing the concept of pending NavigationEntry.  I've long hoped that would be possible after PlzNavigate, but it's a bigger change that would slow down part 1.  (There's lot of things to be careful about regarding the visible URL in the omnibox, etc.)  Let's file a separate bug for that, blocked on this one.

clamy@, would you be able to make the change?  I'm happy to review if that's helpful.

Comment 3 by clamy@chromium.org, Jan 22 2018

Yes, me or someone else form the Paris team should be able to have a look at it.

Comment 4 by bugdroid1@chromium.org, Feb 1 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1df7b96528838e2f6a317503648ff9600a40dac

commit b1df7b96528838e2f6a317503648ff9600a40dac
Author: clamy <clamy@chromium.org>
Date: Thu Feb 01 17:38:17 2018

Introduce a mojo method to send renderer debug URLS to the renderer

This CL introduces a mojo method to send renderer debug URLS to the renderer
for handling, instead of using CommitNavigation. This allows not to create a
NavigationRequest nor a NavigationHandle for these URLs, which do not represent
actual navigations.

Bug: 784904,803859
Change-Id: Ica9b17a071d2f9faa7f43eb428b0d73424a180e9
Reviewed-on: https://chromium-review.googlesource.com/817560
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533719}
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/common/frame.mojom
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/renderer/render_frame_impl.h
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/test/content_browser_test_test.cc
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/content/test/test_render_frame_host.cc
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.h
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/third_party/WebKit/public/web/WebLocalFrame.h
[modify] https://crrev.com/b1df7b96528838e2f6a317503648ff9600a40dac/tools/perf/core/stacktrace_unittest.py

Comment 5 by mkwst@chromium.org, Feb 7 2018

Blocking: 803365

Comment 6 by bugdroid1@chromium.org, May 3 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/987a37584e3282bf96815070c42fed79618e83d5

commit 987a37584e3282bf96815070c42fed79618e83d5
Author: clamy <clamy@chromium.org>
Date: Thu May 03 17:36:26 2018

Move NavigationRequest creation to NavigationController: 1/3

This Cl is the first in a 3 CL series to move the NavigationRequest
creation to the NavigationController. The end goal is to have Navigator
take a NavigationRequest as an argument to Navigate instead of the
pending NavigationEntry.

Summary of the changes in this CL:
1) Navigator
 - NavigatorImpl::RequestTransferURL becomes NavigateFromFrameProxy. It calls
   NavigationControllerImpl::NavigateFromFrameProxy instead of creating a
   NavigationEntry and navigating to it directly.
 - NavigatorImpl::NavigateNewChildFrame becomes
   StartHistoryNavigationInNewChild. It calls
   NavigationControllerImpl::StartHistoryNavigationInNewChild instead of creating
   a NavigationEntry and navigating to it directly.

2) NavigationController:
 - StartHistoryNavigationInNewChild is a new function that contains code that
   used to be in NavigatorImpl::NavigateNewChildFrame. It is called when creating
   a subframe during a history navigation. It will try to perform a history
   navigation in the child frame.
 - NavigateFromFrameProxy is a new function that contains code moved from
   NavigatorImpl::RequestTransferURL. It creates a NavigationEntry and a
   NavigationRequest for the navigation, and asks the Navigator to perform them.

Bug: 803859
Change-Id: I45891c21dc8f1e86e3f1bc1829ee3a92af1a5bfb
Reviewed-on: https://chromium-review.googlesource.com/959012
Commit-Queue: Camille Lamy <clamy@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555791}
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/navigation_controller_impl.h
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/navigation_controller_impl_unittest.cc
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/navigator.cc
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/navigator.h
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/navigator_impl.h
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/987a37584e3282bf96815070c42fed79618e83d5/content/browser/frame_host/render_frame_proxy_host.cc

Comment 7 by clamy@chromium.org, Jun 4 2018

Update: the third part of the NavigationController refactoring is in review at https://chromium-review.googlesource.com/c/chromium/src/+/904988. I plan to follow up that CL with additionnal work:
1) Adding another method for creating NavigationRequests that only uses the LoadURLParams and does not depend on the pending NavigationEntry.

2) Unifying code in StartHistoryNavigationInNewChild and NavigateFromFrameProxy with NavigateToExistingEntry and NavigateWithoutEntry.

3) Removing the pending NavigationEntry and instead create NavigationEntries from the NavigationRequests when the navigation commits.

For part 3, we will likely need to provide a content/public interface with an interface that gives the embedder the data it needs about the pending Navigation (let's tentatively call it PendingNavigationData). This interface should be implemented by NavigationRequest. Note that this is blocked on not having race conditions leading to early deletion of NavigationRequests.

For part 3, we will also need to assess what will become of the VisibleEntry concept. This will likely require an audit of what existing callers are using it for, to find the best replacement/adaptation to the change.

Comment 8 by nasko@chromium.org, Jun 4 2018

For "we will also need to assess what will become of the VisibleEntry concept.", I would try to go a step further - look at why code needs to know about GetVisibleEntry instead of using WebContents::GetVisibleURL. As far as I know, we only care about the distinction between visible vs last committed when it comes to the omnibox and what we show there. Most other code should only care about last committed.

Comment 9 by bugdroid1@chromium.org, Jun 13 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21718cc2274ec87c679d9a14643d440148966cdd

commit 21718cc2274ec87c679d9a14643d440148966cdd
Author: clamy <clamy@chromium.org>
Date: Wed Jun 13 13:34:24 2018

Move NavigationRequest creation to NavigationController 3/3

This CL creates a new function in NavigationController following the move of
the NavigationRequest creation to the NavigationController. This function,
NavigateWithoutEntry is called for new navigations, for which we don't have a
NavigationEntry. It creates a pending NavigationEntry and a single
NavigationRequest based on the LoadURLParams, then asks the Navigator to
navigate to it. This allows the creation of NavigationRequests based on
LoadURLParams in a function that has access to LoadURLParams.

This is part of a larger refactoring effort. I plan to follow up this
CL by additional work:
1) Adding another method for creating NavigationRequests that only uses the
LoadURLParams and does not depend on the pending NavigationEntry.

2) Introduce a NavigateToExistingEntry method that will be used for navigations
to existing NavigationEntries.

3) Unifying code in StartHistoryNavigationInNewChild and NavigateFromFrameProxy
with NavigateToExistingEntry and NavigateWithoutEntry.

4) Removing the pending NavigationEntry and instead create NavigationEntries
from the NavigationRequests when the navigation commits.

Bug: 803365, 803859
Change-Id: I325ddbab2309ce88c138e7e92755aa31cc6c4617
Reviewed-on: https://chromium-review.googlesource.com/904988
Commit-Queue: Camille Lamy <clamy@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566817}
[modify] https://crrev.com/21718cc2274ec87c679d9a14643d440148966cdd/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/21718cc2274ec87c679d9a14643d440148966cdd/content/browser/frame_host/navigation_controller_impl.h

Comment 10 by clamy@chromium.org, Jun 14 2018

Blockedon: 852774

Comment 11 by clamy@chromium.org, Jun 18 2018

Summary: NavigationController refactoring (was: Create NavigationRequest in NavigationController)

Comment 12 by clamy@chromium.org, Jun 18 2018

Blockedon: -852774 273710

Comment 13 by clamy@chromium.org, Jun 18 2018

Blockedon: 853705

Comment 14 by bugdroid1@chromium.org, Jul 10 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3cb9bea95cc71183d56666baf06bbedf7d2e527b

commit 3cb9bea95cc71183d56666baf06bbedf7d2e527b
Author: clamy <clamy@chromium.org>
Date: Tue Jul 10 12:42:02 2018

Introduce NavigateToExistingPendingEntry in NavigationController

Following the introduction of NavigateWithoutEntry in
https://chromium-review.googlesource.com/c/chromium/src/+/904988, this
CL introduces a new function in NavigationController,
NavigateToExistingPendingEntry. This function should be used when requesting a
navigation to an existing NavigationEntry.

Bug: 803365, 803859
Change-Id: I2b1901c0e63934aac4df9837104dcb3f8f28fdc0
Reviewed-on: https://chromium-review.googlesource.com/1089052
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573693}
[modify] https://crrev.com/3cb9bea95cc71183d56666baf06bbedf7d2e527b/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/3cb9bea95cc71183d56666baf06bbedf7d2e527b/content/browser/frame_host/navigation_controller_impl.h

Comment 15 by clamy@chromium.org, Sep 4

Blocking: -803365

Comment 16 by bugdroid1@chromium.org, Oct 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5193caa162249e66ecec99fbf1e94fd38557b672

commit 5193caa162249e66ecec99fbf1e94fd38557b672
Author: Camille Lamy <clamy@chromium.org>
Date: Fri Oct 12 11:59:42 2018

Create NavigationRequest from LoadURLParams

This CL allows to create the NavigationRequest directly from
LoadURLParams for new navigations.

Bug: 803859
Change-Id: Ic0f3f368ade86fe57bb549ae6c1d5711f1b58d0e
Reviewed-on: https://chromium-review.googlesource.com/c/1097407
Commit-Queue: Camille Lamy <clamy@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599176}
[modify] https://crrev.com/5193caa162249e66ecec99fbf1e94fd38557b672/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/5193caa162249e66ecec99fbf1e94fd38557b672/content/browser/frame_host/navigation_controller_impl.h
[modify] https://crrev.com/5193caa162249e66ecec99fbf1e94fd38557b672/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/5193caa162249e66ecec99fbf1e94fd38557b672/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/5193caa162249e66ecec99fbf1e94fd38557b672/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/5193caa162249e66ecec99fbf1e94fd38557b672/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/5193caa162249e66ecec99fbf1e94fd38557b672/content/browser/frame_host/render_frame_host_manager_unittest.cc

Comment 17 by bugdroid1@chromium.org, Oct 19

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2baa802799f9aebcc169d436d39ce10920b440d4

commit 2baa802799f9aebcc169d436d39ce10920b440d4
Author: Camille Lamy <clamy@chromium.org>
Date: Fri Oct 19 16:43:17 2018

Don't rewrite subframe navigation URLs

This CL makes sure we do not attempt to rewrite a subframe navigation URL in
should only be performed on main frame navigations.

NavigationControllerImpl: :CreateNavigationRequestFromLoadParams. Rewrites
Bug:  895065 , 803859, 896028
Change-Id: I2a2326d802b55655d59f0c6d3d73e3060c58152b
Reviewed-on: https://chromium-review.googlesource.com/c/1282992
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601183}
[modify] https://crrev.com/2baa802799f9aebcc169d436d39ce10920b440d4/content/browser/browser_url_handler_impl.cc
[modify] https://crrev.com/2baa802799f9aebcc169d436d39ce10920b440d4/content/browser/browser_url_handler_impl.h
[modify] https://crrev.com/2baa802799f9aebcc169d436d39ce10920b440d4/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/2baa802799f9aebcc169d436d39ce10920b440d4/content/browser/frame_host/navigation_controller_impl_unittest.cc

Comment 18 by bugdroid1@chromium.org, Dec 5

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10aafcd3177bef1ad096d55bd2469504a95acb82

commit 10aafcd3177bef1ad096d55bd2469504a95acb82
Author: Camille Lamy <clamy@chromium.org>
Date: Wed Dec 05 15:48:13 2018

Pass the NavigationRequest to NavigationController at commit time

This CL makes it so that NavigationRequests are passed to the
NavigationController at commit time instead of NavigationHandles. This
is a preparatory CL for tracking NavigationRequests in the
NavigationController and using them to make decisions about the deletion
of the pending NavigationEntry.

Bug: 803859
Change-Id: I71f48e2d8ee68c6d02d4ac6b7642137b8206febb
Reviewed-on: https://chromium-review.googlesource.com/c/1327201
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613977}
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/interstitial_page_navigator_impl.cc
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/interstitial_page_navigator_impl.h
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/navigation_controller_impl.h
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/navigator.h
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/navigator_impl.h
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/10aafcd3177bef1ad096d55bd2469504a95acb82/content/browser/frame_host/render_frame_host_impl.h

Sign in to add a comment