NavigationController refactoring |
||||||||||
Issue descriptionI 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. ⛆ |
|
|
,
Jan 20 2018
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.
,
Jan 22 2018
Yes, me or someone else form the Paris team should be able to have a look at it.
,
Feb 1 2018
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
,
Feb 7 2018
,
May 3 2018
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
,
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.
,
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.
,
Jun 13 2018
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
,
Jun 14 2018
,
Jun 18 2018
,
Jun 18 2018
,
Jul 10
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
,
Sep 4
,
Oct 12
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
,
Oct 19
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
,
Dec 5
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 |
||||||||||
Comment 1 by nasko@chromium.org
, Jan 20 2018