Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
379143002:
PlzNavigate: implement RequestNavigation in the no live renderer case (Closed)
|
Created:
6 years, 5 months ago by clamy Modified:
6 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, miu+watch_chromium.org, carlosk Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: implement RequestNavigation in the no live renderer case
This CL introduces the RenderFrameHostManager::RequestNavigation method, and
implements the case where there is no live renderer. In that case, navigation
requests are forwarded to the ResourceDispatcherHost on the IO thread via a
call to RenderFrameHostManager::BeginNavigation.
BUG=376006, 376082
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290570
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 10
Patch Set 3 : #
Total comments: 33
Patch Set 4 : #Patch Set 5 : Added a preliminary implementation of CommitNavigation to clarify the lifetime of rfhs #
Total comments: 28
Patch Set 6 : Addressed Nasko's comments #
Total comments: 7
Patch Set 7 : Addressed Nasko's comments + added missing file #
Total comments: 4
Patch Set 8 : Addressed Nasko's comments #
Total comments: 27
Patch Set 9 : Addressed Charlie's comments #
Total comments: 33
Patch Set 10 : Addressed Charlie's comments #
Total comments: 5
Patch Set 11 : Rebase #Patch Set 12 : Removed speculative rfh + no pending_navigation_entry dependency #
Total comments: 40
Patch Set 13 : Addressed Charlie's comments #
Total comments: 16
Patch Set 14 : Rebase + addressed comments #Patch Set 15 : Rebase on top of issue 471603002 #
Total comments: 4
Patch Set 16 : #Patch Set 17 : #Messages
Total messages: 60 (0 generated)
@ nasko, ppi: PTAL. This is patch is dependent on the previous one (https://chromiumcodereview.appspot.com/367653002/). With these two patches, navigation requests should be properly sent to the IO thread when there is no live renderer.
@ nasko, ppi: friendly ping now that the previous patch has landed :) https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... File content/browser/frame_host/navigation_entry_impl.h (right): https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... content/browser/frame_host/navigation_entry_impl.h:227: void MakeNavigateParams(const NavigationControllerImpl& controller, The code for this function has been moved from navigator_impl.cc because it is now also used in the unit test introduced by this patch. https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... content/browser/frame_host/navigator_impl.cc:265: #if defined(USE_PLZ_NAVIGATE) Do you think it is better to use a runtime flag instead of a compile time flag in that case?
Thanks, please find a few comments below. https://codereview.chromium.org/379143002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/379143002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_entry_impl.h:227: void MakeNavigateParams(const NavigationControllerImpl& controller, On 2014/07/16 12:13:49, clamy wrote: > The code for this function has been moved from navigator_impl.cc because it is > now also used in the unit test introduced by this patch. Could we keep this in Navigator and make it a static method? This way we would avoid coupling the knowledge of FrameMsg_Navigate_Params with NavigationEntry. https://codereview.chromium.org/379143002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/379143002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:265: #if defined(USE_PLZ_NAVIGATE) On 2014/07/16 12:13:49, clamy wrote: > Do you think it is better to use a runtime flag instead of a compile time flag > in that case? I'm curious to hear Nasko's opinion, personally I do. For instance, there is a typo in "navigate_params" below that would have been caught if this was a runtime flag ;). https://codereview.chromium.org/379143002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/379143002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:243: if (current_instance != new_instance) { Wouldn't this logic be deferred until after we followed the redirects in the /loader ? https://codereview.chromium.org/379143002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/379143002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:429: bool InitRFHsBeforeNavigation(RenderFrameHostImpl* dest_render_frame_host); nit: if no other method here is named similarily, I would expand the abbreviation
Thanks! https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... File content/browser/frame_host/navigation_entry_impl.h (right): https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... content/browser/frame_host/navigation_entry_impl.h:227: void MakeNavigateParams(const NavigationControllerImpl& controller, On 2014/07/16 14:28:09, ppi wrote: > On 2014/07/16 12:13:49, clamy wrote: > > The code for this function has been moved from navigator_impl.cc because it is > > now also used in the unit test introduced by this patch. > > Could we keep this in Navigator and make it a static method? This way we would > avoid coupling the knowledge of FrameMsg_Navigate_Params with NavigationEntry. Done. https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:243: if (current_instance != new_instance) { On 2014/07/16 14:28:09, ppi wrote: > Wouldn't this logic be deferred until after we followed the redirects in the > /loader ? I thought we wanted to speculatively spawn a renderer when we get the navigation request if we have no live one as it's extremely likely that we will need one. https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... content/browser/frame_host/render_frame_host_manager.h:429: bool InitRFHsBeforeNavigation(RenderFrameHostImpl* dest_render_frame_host); On 2014/07/16 14:28:09, ppi wrote: > nit: if no other method here is named similarily, I would expand the > abbreviation Done.
https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/20001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:243: if (current_instance != new_instance) { On 2014/07/16 15:44:53, clamy wrote: > On 2014/07/16 14:28:09, ppi wrote: > > Wouldn't this logic be deferred until after we followed the redirects in the > > /loader ? > > I thought we wanted to speculatively spawn a renderer when we get the navigation > request if we have no live one as it's extremely likely that we will need one. Yes, we want to spawn a renderer, but at this point we don't know yet what will be the site instance for the navigation, right?
I have one concern about the current state of the CL. It isn't clear to me whether in RFHM we can have both render_view_host_ and speculative_render_view_host_ at the same time. I'd rather not, as it makes things a lot more complicated. Also I didn't see anywhere the speculative RFH being transitioned to being the active RFH. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:75: NavigatorImpl::NavigatorImpl( Any reason for the move of the constructor around? https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:82: // Static. nit: lowercase S https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:347: // PlzNavigate navigation refactoring). nit: "navigation" is extraneous here. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:357: // navigation requests for that frame node. There is quite a bit of code that follows the return here, including notifications for various events and security checks. Why are we skipping them? https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:358: #if defined(USE_PLZ_NAVIGATE) Did we agree on define vs command line flag? https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:213: // Initialize the destination RFH and the current RFH if they are not live. If they are not alive, then they will be the same, right? The comment implies that they can be different. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:232: if (render_frame_host_->render_view_host()->IsRenderViewLive()) Let's put a TODO to put IsLive on RenderFrameHost, as it doesn't make sense to go to the RVH long term. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:237: OnBeginNavigation(BeginNavigationFromNavigate(navigate_params)); There is a comment in OnBeginNavigation to create the speculative RFH. You are doing it below. One of the two has to be corrected. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:260: return InitRenderFrameHostsBeforeNavigation(dest_render_frame_host); Do we need to init in the case where we don't create a new RFH? https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:980: DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden. is_speculative should never be true when swapped_out is true, right? If yes, then let's put a DCHECK to enforce it. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:1100: // its first page. (Bug 1145340) nit: convert this to a crbug link. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.h:506: // commits. This is part of the PlzNavigate navigation refactoring project. nit: I don't think we should sprinkle PlzNavigate in all comments. This is an example where it isn't really needed. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1919: EXPECT_TRUE(main_request); main_request is a pointer, not boolean. Use EXPECT_EQ or if using EXPECT_TRUE, use a comparison to NULL. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1922: EXPECT_TRUE(main_test_rfh()->render_view_host()->IsRenderViewLive()); Shouldn't the main RFH also be the speculative RFH?
Thanks! So the current state of the patch is that we create a speculative_render_frame_host_ if the site instance we are trying to navigate to is different from the site instance of render_frame_host_. However I think we need to keep the current render_frame_host_ along until the navigation commits. Since spawning the speculative_render_frame_host_ in advance is a performance optimization, it could be removed. In any case, the speculative_render_frame_host_ can only become the render_frame_host_ when the navigation commits and we realize that it is indeed the right RFH to use for that navigation (and I am working on that part in a different patch). https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:75: NavigatorImpl::NavigatorImpl( On 2014/07/17 12:05:08, nasko (in Munich) wrote: > Any reason for the move of the constructor around? Actually I'm moving the function MakeNavigateParams out of the anonymous namespace and below the constructor, but git registered this as the constructor moving up. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:82: // Static. On 2014/07/17 12:05:08, nasko (in Munich) wrote: > nit: lowercase S Done. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:347: // PlzNavigate navigation refactoring). On 2014/07/17 12:05:08, nasko (in Munich) wrote: > nit: "navigation" is extraneous here. Done. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:357: // navigation requests for that frame node. On 2014/07/17 12:05:08, nasko (in Munich) wrote: > There is quite a bit of code that follows the return here, including > notifications for various events and security checks. Why are we skipping them? They need a destination render frame host, which we don't possess at that time. I thought it would be better to check them when we have the final destination renderer (on commit). https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:358: #if defined(USE_PLZ_NAVIGATE) On 2014/07/17 12:05:08, nasko (in Munich) wrote: > Did we agree on define vs command line flag? No, I was asking about that in the previous patch set actually. Do you have any preference on the question? https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:213: // Initialize the destination RFH and the current RFH if they are not live. On 2014/07/17 12:05:08, nasko (in Munich) wrote: > If they are not alive, then they will be the same, right? The comment implies > that they can be different. What if the current renderer crashed and we navigate to a brand new different site, do we reuse the RFH in that case? https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:232: if (render_frame_host_->render_view_host()->IsRenderViewLive()) On 2014/07/17 12:05:08, nasko (in Munich) wrote: > Let's put a TODO to put IsLive on RenderFrameHost, as it doesn't make sense to > go to the RVH long term. Done. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:237: OnBeginNavigation(BeginNavigationFromNavigate(navigate_params)); On 2014/07/17 12:05:08, nasko (in Munich) wrote: > There is a comment in OnBeginNavigation to create the speculative RFH. You are > doing it below. One of the two has to be corrected. The comment in OnBeginNavigation had more to do with creating a RFH speculatively in case we realize we have to do a cross-site navigation and we don't have a proper renderer for that. I guess I could move the RFH creation part below into OnBeginNavigation, so that we only have speculative RFH spawning in one place. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:260: return InitRenderFrameHostsBeforeNavigation(dest_render_frame_host); On 2014/07/17 12:05:08, nasko (in Munich) wrote: > Do we need to init in the case where we don't create a new RFH? We are in the case where the current RFH is not live, so if we don't create a new one, we are likely to use the not live current_render_frame_host_ when the navigation commits. I think it should be initialized, but I may be mistaken. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:980: DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden. On 2014/07/17 12:05:08, nasko (in Munich) wrote: > is_speculative should never be true when swapped_out is true, right? If yes, > then let's put a DCHECK to enforce it. Done. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:1100: // its first page. (Bug 1145340) On 2014/07/17 12:05:08, nasko (in Munich) wrote: > nit: convert this to a crbug link. Done. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.h:506: // commits. This is part of the PlzNavigate navigation refactoring project. On 2014/07/17 12:05:08, nasko (in Munich) wrote: > nit: I don't think we should sprinkle PlzNavigate in all comments. This is an > example where it isn't really needed. Done. I guess I'm worried that people may not realize that it is part of a wip and should probably avoid using it until we have more functionality in place. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1919: EXPECT_TRUE(main_request); On 2014/07/17 12:05:08, nasko (in Munich) wrote: > main_request is a pointer, not boolean. Use EXPECT_EQ or if using EXPECT_TRUE, > use a comparison to NULL. Done. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1922: EXPECT_TRUE(main_test_rfh()->render_view_host()->IsRenderViewLive()); On 2014/07/17 12:05:08, nasko (in Munich) wrote: > Shouldn't the main RFH also be the speculative RFH? The speculative RFH cannot be the main RFH. The speculative RFH is only spawned when the site instance for the navigation is different from the one for the main RFH. If they are the same, then the current RFH is initialized, and no extra RFH is created.
I didn't see an updated version of the patch with the few things you marked as done. Did you miss to upload it? Overall, this looks fine, but I'd like to see more of the code that will deal with the speculative renderer and its transitions. Since you mentioned you have another WIP patch, can you share that too or fold it into this one? https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/navigator_impl.cc:358: #if defined(USE_PLZ_NAVIGATE) On 2014/07/17 13:07:53, clamy wrote: > On 2014/07/17 12:05:08, nasko (in Munich) wrote: > > Did we agree on define vs command line flag? > > No, I was asking about that in the previous patch set actually. Do you have any > preference on the question? Let's use a command line flag. This way we don't have to re-run gyp/recompile to run in the two modes. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:213: // Initialize the destination RFH and the current RFH if they are not live. On 2014/07/17 13:07:53, clamy wrote: > On 2014/07/17 12:05:08, nasko (in Munich) wrote: > > If they are not alive, then they will be the same, right? The comment implies > > that they can be different. > > What if the current renderer crashed and we navigate to a brand new different > site, do we reuse the RFH in that case? No, we don't. I was thinking initial creation only and didn't take into account the crashed renderer. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:260: return InitRenderFrameHostsBeforeNavigation(dest_render_frame_host); On 2014/07/17 13:07:53, clamy wrote: > On 2014/07/17 12:05:08, nasko (in Munich) wrote: > > Do we need to init in the case where we don't create a new RFH? > > We are in the case where the current RFH is not live, so if we don't create a > new one, we are likely to use the not live current_render_frame_host_ when the > navigation commits. I think it should be initialized, but I may be mistaken. I don't think we need a live current renderer to start the new navigation, but handling that case might just make the code more complex, which I'm not a fan of.
Sorry, I indeed forgot to upload the new patch set. I think I will fold the part of the other CL that deals with selecting the good RFH on commit into this one, and that will make things clearer. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:260: return InitRenderFrameHostsBeforeNavigation(dest_render_frame_host); On 2014/07/21 13:02:05, nasko (in Munich) wrote: > On 2014/07/17 13:07:53, clamy wrote: > > On 2014/07/17 12:05:08, nasko (in Munich) wrote: > > > Do we need to init in the case where we don't create a new RFH? > > > > We are in the case where the current RFH is not live, so if we don't create a > > new one, we are likely to use the not live current_render_frame_host_ when the > > navigation commits. I think it should be initialized, but I may be mistaken. > > I don't think we need a live current renderer to start the new navigation, but > handling that case might just make the code more complex, which I'm not a fan > of. So should I let the initialization in place?
@nasko: PTAL. This new version of the patch has an implementation of CommitNavigation which should clarify the lifetime of the various RFHs. This implementation of CommitNavigation is only focused on the management of RFHs, so it will have to be completed in future patches to provide the additional functionality needed.
A few more comments. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_impl.cc:857: if (CommandLine::ForCurrentProcess()->HasSwitch( Is OnBeginNavigation expected to be called in non-plznavigate code? I wonder if checking the command line switch here is redundant. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:600: // Create a new RFH for the navigation if necessary. nit: We are creating a speculative RFH here, right? Let's add it to the comment so it is clear why we are doing it. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:662: SetRenderFrameHost(speculative_render_frame_host_.Pass()); SetRenderFrameHost returns out the existing RFH in scoped_ptr and it looks like we will leak it here. In general this will have to handle possibly creating a RenderFrameProxyHost for the old RFH. Maybe put a TODO for it? https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:979: RenderFrameHostImpl* RenderFrameHostManager::CreateRenderFrameHostForCrossSite( nit: The "Cross" part of the name of this method is a bit awkward. I don't have anything that would be much better, but maybe using "New"? https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:984: // we are staying in the same BrowsingInstance. This allows the pending RFH nit: The pending RFH hasn't committed, so it cannot send any script calls over. It might be better to call it "the new" or something similar, since once it commits it is no longer pending. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:992: // Create a non-swapped-out speculative RFH with the given opener. nit: The comment implies that the RFH is the speculative, but there is a variable for it. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:376: RenderFrameHostImpl* SpeculativeRenderFrameHostForRenderFrameManager( nit: this and the method above are returning things out, so prefixing with Get* will make it clearer. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1874: // eventually. Why can't we do it now? https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1942: main_request =NavigationRequestForRenderFrameManager( nit: space after = https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1953: // host. Test that it is used if the navigation commits with the same url. It should be reused if we commit in the same SiteInstance, regardless of what the URL is. For example, going to docs.google.com, which bounces to accounts.google.com for auth should end up committing in the google.com renderer. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:2034: EXPECT_TRUE(main_request == NULL); Let's also assert that there is no longer speculative RFH associated with the RFHM. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:2078: EXPECT_TRUE(main_request == NULL); Same as above, it is not testing that the speculative RFH is indeed discarded. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/public/co... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/public/co... content/public/common/content_switches.cc:323: // Use the experimental browser-side navigation path. nit: Put the PlzNavigate keyword, so people can easily find the command line switch if they are searching for the project.
Thanks! https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_impl.cc:857: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2014/07/23 14:15:45, nasko (in Munich) wrote: > Is OnBeginNavigation expected to be called in non-plznavigate code? I wonder if > checking the command line switch here is redundant. This is due to a comment by davidben@ that a compromised renderer might end up sending the FrameHostMsg_BeginNavigation and we would then execute the PlzNavigate codepath which is not well tested right now, leading to possible security problems. Wdyt? https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:600: // Create a new RFH for the navigation if necessary. On 2014/07/23 14:15:46, nasko (in Munich) wrote: > nit: We are creating a speculative RFH here, right? Let's add it to the comment > so it is clear why we are doing it. Done. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:662: SetRenderFrameHost(speculative_render_frame_host_.Pass()); On 2014/07/23 14:15:46, nasko (in Munich) wrote: > SetRenderFrameHost returns out the existing RFH in scoped_ptr and it looks like > we will leak it here. In general this will have to handle possibly creating a > RenderFrameProxyHost for the old RFH. Maybe put a TODO for it Yes I was planning to do it later. I reformulated the TODO. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:979: RenderFrameHostImpl* RenderFrameHostManager::CreateRenderFrameHostForCrossSite( On 2014/07/23 14:15:46, nasko (in Munich) wrote: > nit: The "Cross" part of the name of this method is a bit awkward. I don't have > anything that would be much better, but maybe using "New"? Done. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:984: // we are staying in the same BrowsingInstance. This allows the pending RFH On 2014/07/23 14:15:46, nasko (in Munich) wrote: > nit: The pending RFH hasn't committed, so it cannot send any script calls over. > It might be better to call it "the new" or something similar, since once it > commits it is no longer pending. Done. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:992: // Create a non-swapped-out speculative RFH with the given opener. On 2014/07/23 14:15:46, nasko (in Munich) wrote: > nit: The comment implies that the RFH is the speculative, but there is a > variable for it. Done. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:376: RenderFrameHostImpl* SpeculativeRenderFrameHostForRenderFrameManager( On 2014/07/23 14:15:46, nasko (in Munich) wrote: > nit: this and the method above are returning things out, so prefixing with Get* > will make it clearer. Done. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1874: // eventually. On 2014/07/23 14:15:46, nasko (in Munich) wrote: > Why can't we do it now? Because we don't have a proper commit of the navigation yet. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1942: main_request =NavigationRequestForRenderFrameManager( On 2014/07/23 14:15:46, nasko (in Munich) wrote: > nit: space after = Done. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1953: // host. Test that it is used if the navigation commits with the same url. On 2014/07/23 14:15:46, nasko (in Munich) wrote: > It should be reused if we commit in the same SiteInstance, regardless of what > the URL is. For example, going to http://docs.google.com, which bounces to > http://accounts.google.com for auth should end up committing in the http://google.com > renderer. I now test that it works as long as we commit a url in the same site instance. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:2034: EXPECT_TRUE(main_request == NULL); On 2014/07/23 14:15:46, nasko (in Munich) wrote: > Let's also assert that there is no longer speculative RFH associated with the > RFHM. Done. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:2078: EXPECT_TRUE(main_request == NULL); On 2014/07/23 14:15:46, nasko (in Munich) wrote: > Same as above, it is not testing that the speculative RFH is indeed discarded. Done. https://chromiumcodereview.appspot.com/379143002/diff/80001/content/public/co... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/public/co... content/public/common/content_switches.cc:323: // Use the experimental browser-side navigation path. On 2014/07/23 14:15:46, nasko (in Munich) wrote: > nit: Put the PlzNavigate keyword, so people can easily find the command line > switch if they are searching for the project. Done.
Mostly there. I see there are compile errors for missing header. Is it coming from another CL? https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_impl.cc:857: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2014/07/23 14:56:09, clamy wrote: > On 2014/07/23 14:15:45, nasko (in Munich) wrote: > > Is OnBeginNavigation expected to be called in non-plznavigate code? I wonder > if > > checking the command line switch here is redundant. > > This is due to a comment by davidben@ that a compromised renderer might end up > sending the FrameHostMsg_BeginNavigation and we would then execute the > PlzNavigate codepath which is not well tested right now, leading to possible > security problems. Wdyt? sgtm https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1874: // eventually. On 2014/07/23 14:56:10, clamy wrote: > On 2014/07/23 14:15:46, nasko (in Munich) wrote: > > Why can't we do it now? > > Because we don't have a proper commit of the navigation yet. Ok https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:611: InitRenderFrameHostsBeforeNavigation(dest_render_frame_host); Shouldn't this be before we send the navigation request to the IO thread? At least the name of the function implies so. I think it is safe as is, since even if the IO thread gets the data back immediately (cache or whatever else), it will not execute anything on the main thread since this method is finishing up. https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:418: RenderFrameHostImpl* CreateRenderFrameHostForNewSite( sorry to continue nitpicking, but probably "NewSiteInstance". https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1984: // Commit the same url. nit: It is no longer the same url :).
The compilation error was due to me forgetting to add a new file to the cl, my bad :). https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:611: InitRenderFrameHostsBeforeNavigation(dest_render_frame_host); On 2014/07/24 09:55:47, nasko (in Munich) wrote: > Shouldn't this be before we send the navigation request to the IO thread? At > least the name of the function implies so. I think it is safe as is, since even > if the IO thread gets the data back immediately (cache or whatever else), it > will not execute anything on the main thread since this method is finishing up. It should not be a problem, but to make it clearer I moved the part where we start the navigation below. https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:418: RenderFrameHostImpl* CreateRenderFrameHostForNewSite( On 2014/07/24 09:55:47, nasko (in Munich) wrote: > sorry to continue nitpicking, but probably "NewSiteInstance". Done. https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1984: // Commit the same url. On 2014/07/24 09:55:47, nasko (in Munich) wrote: > nit: It is no longer the same url :). Done.
LGTM https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/100001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:611: InitRenderFrameHostsBeforeNavigation(dest_render_frame_host); On 2014/07/24 11:38:05, clamy wrote: > On 2014/07/24 09:55:47, nasko (in Munich) wrote: > > Shouldn't this be before we send the navigation request to the IO thread? At > > least the name of the function implies so. I think it is safe as is, since > even > > if the IO thread gets the data back immediately (cache or whatever else), it > > will not execute anything on the main thread since this method is finishing > up. > > It should not be a problem, but to make it clearer I moved the part where we > start the navigation below. Technically, we are better off sending the request to the IO thread as early as we can. It is best to start off defensively though, we can fine tune it in the future once we get it working. https://chromiumcodereview.appspot.com/379143002/diff/120001/content/browser/... File content/browser/frame_host/navigation_commit_info.h (right): https://chromiumcodereview.appspot.com/379143002/diff/120001/content/browser/... content/browser/frame_host/navigation_commit_info.h:25: GURL navigation_url; Does the new renderer care about what redirects the navigation went through? If yes, we should add a TODO. https://chromiumcodereview.appspot.com/379143002/diff/120001/content/browser/... content/browser/frame_host/navigation_commit_info.h:29: GURL blob_url; nit: Blob refers to the type of technology we use to transport the content to the renderer. It will be better to name it to reflect that this URL holds the actual content from the server side.
@nasko: Thanks for the review! @creis: PTAL at the changes in content. https://chromiumcodereview.appspot.com/379143002/diff/120001/content/browser/... File content/browser/frame_host/navigation_commit_info.h (right): https://chromiumcodereview.appspot.com/379143002/diff/120001/content/browser/... content/browser/frame_host/navigation_commit_info.h:25: GURL navigation_url; On 2014/07/24 12:03:58, nasko (in Munich) wrote: > Does the new renderer care about what redirects the navigation went through? If > yes, we should add a TODO. Done. https://chromiumcodereview.appspot.com/379143002/diff/120001/content/browser/... content/browser/frame_host/navigation_commit_info.h:29: GURL blob_url; On 2014/07/24 12:03:58, nasko (in Munich) wrote: > nit: Blob refers to the type of technology we use to transport the content to > the renderer. It will be better to name it to reflect that this URL holds the > actual content from the server side. Done.
@ creis: friendly ping :). @ davidben: FYI. This is the CL we talked about yesterday. I'm working on a followup where there will be a static CommitNavigation method in either RenderFrameHostManager or in FrameTreeNode, that the content/loader will be able to bind and post on the UI thread when the navigation commits.
On 2014/07/29 12:05:50, clamy wrote: > @ creis: friendly ping :). Ah, sorry! I missed the earlier request and didn't realize you were waiting on me. Glad you all have been making progress while I've been out! I haven't gotten much into the .cc files yet, but I'm hoping we can come up with a clearer convention for marking the PlzNavigate code. I want to be sure it stays separate from the OOPIF refactoring, and that the overall code doesn't get too unwieldy to read as it tries to support two different paths. A few high level questions and suggestions below, and then I can take a closer look at the code itself. https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager.h:506: // commits. This is part of the PlzNavigate navigation refactoring project. On 2014/07/17 13:07:53, clamy wrote: > On 2014/07/17 12:05:08, nasko (in Munich) wrote: > > nit: I don't think we should sprinkle PlzNavigate in all comments. This is an > > example where it isn't really needed. > > Done. I guess I'm worried that people may not realize that it is part of a wip > and should probably avoid using it until we have more functionality in place. I agree with clamy@ here. I'm very concerned about the PlzNavigate-specific code getting mixed in with everything else and becoming hard to distinguish. Hopefully a simple "// PlzNavigate:" prefix to the comments is clear and concise enough. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... File content/browser/frame_host/navigation_commit_info.h (right): https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/navigation_commit_info.h:17: // Holds the parameters determined during the load of a navigation request Sounds like this is mainly for the time before commit, so calling it CommitInfo is a bit confusing to me. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/navigation_commit_info.h:29: GURL content_url; I don't understand what this is for. 1) I thought we were going to push the stream to the renderer process, rather than having the renderer request it. Is |content_url| a temporary thing? 2) Why is it different from |navigation_url|? If it's just for uniquely identifying a stream, why is it a URL and not an identifier? https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:230: bool RenderFrameHostManager::RequestNavigation( // PlzNavigate https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:232: const FrameMsg_Navigate_Params& navigate_params) { I'd like to be very careful about preventing other developers from calling PlzNavigate-specific functions unintentionally. Can we start them with a check like the following? CHECK(CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableBrowserSideNavigation)); https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:234: // RenderFrameHost::IsLive. I think Ken's adding IsRenderFrameLive in https://codereview.chromium.org/404613005/. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:614: void RenderFrameHostManager::CommitNavigation( // PlzNavigate https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:193: // IPC to the renderer to ask it to navigate. If no live renderer is present, Why would we send a RequestNavigation IPC to the renderer if it were live? I thought we didn't need to send navigations through the renderer any more. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:317: // Used to start a navigation, part of browser-side navigation project. I'm a little concerned about all of this refactoring code making the existing code unreadable / harder to follow. It would be nice to have a simple and consistent way to label the parts that are specific to this project and can be ignored by others. For example, rather than mentioning "browser-side navigation project" and "PlzNavigate navigation refactoring project" in the comment sentences, what if we stuck to a simple comment prefix, like: // PlzNavigate: Used to start a navigation. // PlzNavigate: Used to signal the commit of a navigation request. We can put a class-level comment in RFHM (kind of the home base of the changes) explaining that PlzNavigate is a refactoring project behind a flag, plus a very high level description, etc. That would give us an easy way to grep for project-related changes and ignore them when we're otherwise working on OOPIF stuff. Thoughts? https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:416: // will be stored in pending_render_frame_host_. Also returns a pointer to the Let's try to divide this comment into separate paragraphs about how it normally behaves and how it behaves for PlzNavigate. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:442: // Initializes |dest_render_frame_host| and |render_frame_host_| if necessary // PlzNavigate? https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:448: SiteInstance* GetSiteInstanceForNavigation(const NavigationEntryImpl& entry); Why is this different than GetSiteInstanceForEntry? Should it be declared close to that one if it's just a parallel implementation for PlzNavigate? https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:510: scoped_ptr<RenderFrameHostImpl> pending_render_frame_host_; We should clarify that this won't exist in PlzNavigate, where we'll use the speculative_render_frame_host_ instead. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:519: // A RenderFrameHost speculatively created when receiving a navigation // PlzNavigate:
Thanks! The labeling should be clearer by now. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... File content/browser/frame_host/navigation_commit_info.h (right): https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/navigation_commit_info.h:17: // Holds the parameters determined during the load of a navigation request On 2014/07/29 17:46:56, Charlie Reis wrote: > Sounds like this is mainly for the time before commit, so calling it CommitInfo > is a bit confusing to me. I renamed it NavigationBeforeCommitInfo. Is that better? https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/navigation_commit_info.h:29: GURL content_url; On 2014/07/29 17:46:56, Charlie Reis wrote: > I don't understand what this is for. > > 1) I thought we were going to push the stream to the renderer process, rather > than having the renderer request it. Is |content_url| a temporary thing? > > 2) Why is it different from |navigation_url|? If it's just for uniquely > identifying a stream, why is it a URL and not an identifier? We are using the blob url construct to request the stream (so the parameter was initially called |blob_url|). Nasko suggested that the fact that we use blob urls was a detail of implementation, so I switched the parameter name to content_url. Maybe stream_url would be more explicit? https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:230: bool RenderFrameHostManager::RequestNavigation( On 2014/07/29 17:46:56, Charlie Reis wrote: > // PlzNavigate Done. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:232: const FrameMsg_Navigate_Params& navigate_params) { On 2014/07/29 17:46:56, Charlie Reis wrote: > I'd like to be very careful about preventing other developers from calling > PlzNavigate-specific functions unintentionally. Can we start them with a check > like the following? > > CHECK(CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableBrowserSideNavigation)); Done. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:234: // RenderFrameHost::IsLive. On 2014/07/29 17:46:56, Charlie Reis wrote: > I think Ken's adding IsRenderFrameLive in > https://codereview.chromium.org/404613005/. Ok. I will rebase if it lands before this CL. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:614: void RenderFrameHostManager::CommitNavigation( On 2014/07/29 17:46:56, Charlie Reis wrote: > // PlzNavigate Done. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:193: // IPC to the renderer to ask it to navigate. If no live renderer is present, On 2014/07/29 17:46:56, Charlie Reis wrote: > Why would we send a RequestNavigation IPC to the renderer if it were live? I > thought we didn't need to send navigations through the renderer any more. Actually it was decided that we would still sent it to the renderer for the renderer to apply certain modifications it would need. But instead of going through FrameLoader::load like we do actually, the renderer would just send back a FrameHostMsg_BeginNavigation and we would start the navigation on the browser thread. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:317: // Used to start a navigation, part of browser-side navigation project. On 2014/07/29 17:46:57, Charlie Reis wrote: > I'm a little concerned about all of this refactoring code making the existing > code unreadable / harder to follow. It would be nice to have a simple and > consistent way to label the parts that are specific to this project and can be > ignored by others. > > For example, rather than mentioning "browser-side navigation project" and > "PlzNavigate navigation refactoring project" in the comment sentences, what if > we stuck to a simple comment prefix, like: > // PlzNavigate: Used to start a navigation. > > // PlzNavigate: Used to signal the commit of a navigation request. > > We can put a class-level comment in RFHM (kind of the home base of the changes) > explaining that PlzNavigate is a refactoring project behind a flag, plus a very > high level description, etc. > > That would give us an easy way to grep for project-related changes and ignore > them when we're otherwise working on OOPIF stuff. Thoughts? Done. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:416: // will be stored in pending_render_frame_host_. Also returns a pointer to the On 2014/07/29 17:46:56, Charlie Reis wrote: > Let's try to divide this comment into separate paragraphs about how it normally > behaves and how it behaves for PlzNavigate. Done. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:442: // Initializes |dest_render_frame_host| and |render_frame_host_| if necessary On 2014/07/29 17:46:57, Charlie Reis wrote: > // PlzNavigate? It is used both by the current code path and PlzNavigate. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:448: SiteInstance* GetSiteInstanceForNavigation(const NavigationEntryImpl& entry); On 2014/07/29 17:46:56, Charlie Reis wrote: > Why is this different than GetSiteInstanceForEntry? Should it be declared close > to that one if it's just a parallel implementation for PlzNavigate? This one compares the current site instance as well as whether we should force swap the instances, and is used both in PlzNavigate and the current Navigate code path. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:510: scoped_ptr<RenderFrameHostImpl> pending_render_frame_host_; On 2014/07/29 17:46:56, Charlie Reis wrote: > We should clarify that this won't exist in PlzNavigate, where we'll use the > speculative_render_frame_host_ instead. Done. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:519: // A RenderFrameHost speculatively created when receiving a navigation On 2014/07/29 17:46:56, Charlie Reis wrote: > // PlzNavigate: Done.
Thanks for the updates. I do think it's easier to read the code this way, and it should help distinguish things for other readers. I seem to have missed some design changes from the initial plans, though, and it's not clear to me what the navigation timeline will look like in the current plans. Is there an updated design doc somewhere? I have some concerns about things like going to the renderer first. Unfortunately, I'll be visiting family Thursday-Monday, so my next reply will be on Tuesday. Sorry that I wasn't able to get this finished before then. https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:193: // IPC to the renderer to ask it to navigate. If no live renderer is present, On 2014/07/30 13:10:25, clamy wrote: > On 2014/07/29 17:46:56, Charlie Reis wrote: > > Why would we send a RequestNavigation IPC to the renderer if it were live? I > > thought we didn't need to send navigations through the renderer any more. > > Actually it was decided that we would still sent it to the renderer for the > renderer to apply certain modifications it would need. But instead of going > through FrameLoader::load like we do actually, the renderer would just send back > a FrameHostMsg_BeginNavigation and we would start the navigation on the browser > thread. Seems like that defeats several advantages of the refactoring: it's safer not to tell the possibly exploited current renderer where we're going (especially for cross-site URLs), and it's faster not to have to wait for the current renderer to reply. I'd like to push back on this decision, but we can discuss it outside this CL. I'm guessing there's reasons for things like extensions, but maybe we can think of alternative solutions. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.h (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:27: // A url to be subsequently requested by the renderer as the main resource for Naming it |stream_url| helps, thanks. Maybe "A unique url to be..." would help further. (I would have said "A unique blob URL to be..." in the comment, but I'm ok if Nasko wants to hide that detail.) https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/navigator_impl.cc:346: // RenderFrameHostManager in browser initiated navigation (part of project Drop the "part of project PlzNavigate" here, since these params are used in the normal case as well. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/navigator_impl.cc:355: // As part of the PlzNavigate project, the RenderFrameHosts are no longer // PlzNavigate: RenderFrameHosts are no longer... https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:49: FrameHostMsg_BeginNavigation_Params BeginNavigationFromNavigate( // PlzNavigate https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:58: // TODO(clamy): This should be modified to take into account caching policy Is this modeled after some existing code? I worry about them diverging and the TODOs growing stale, so maybe it's worth mentioning the code it's based on. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:607: frame_tree_node_->navigator()->GetController()->GetPendingEntry())); We didn't have a dependency on GetPendingEntry before, since it was passed in. (Navigate could be called even when there wasn't a pending entry.) Can we pass it in the entry here? https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:628: frame_tree_node_->navigator()->GetController()->GetPendingEntry(); Again, we didn't depend on GetPendingEntry() being correct at the time of commit in the past, since it's possible that it didn't exist (e.g., in-page navigations that commit synchronously). (There's one hack that looks at it in NavigatorImpl::DidNavigate for OOPIF, but that's going away.) There's a separate controller_->RendererDidNavigate call in NavigatorImpl::DidNavigate for updating the entry. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:666: // TODO(clamy): the rfh_to_navigate should now send a commit IPC to the I'm having trouble following the planned IPC timeline. Is there an updated design doc that covers the planned behavior? https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:450: // Returns the SiteInstance to use for the navigation. I see-- this is a wrapper for GetSiteInstanceForEntry, which becomes a helper function that is only called from here? In that case, we should definitely declare this right above GetSiteInstanceForEntry and make it clear that callers want this and not GetSiteInstanceForEntry. I would almost recommend combining them, except that GetSiteInstanceForEntry is complicated enough already. Let's just make it clear that it's solely a helper function. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1867: // Browser-side navigation: Test that a proper NavigationRequest is created by // PlzNavigate: https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1869: TEST_F(RenderFrameHostManagerTest, BrowserSideNavigationBeginNavigation) { As before, let's settle on something consistent to make it easier for others to follow. PlzNavigateBeginNavigation? (here and in the tests below) https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1875: // eventually. Mention why we have to do it later for now, since it's not clear at the moment. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1888: subframe_rfh->frame_tree_node()->navigator()->GetController()->LoadURL( The main frame and subframe share the same NavigationController. Why do we need this here and contents()->GetController().LoadURL below? https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1916: // Browser side navigation: Test that RequestNavigation creates a // PlzNavigate: (here and in all the tests below) https://chromiumcodereview.appspot.com/379143002/diff/160001/content/public/c... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/public/c... content/public/common/content_switches.cc:320: // Use the experimental browser-side navigation path (PlzNavigate project). // PlzNavigate: Use the...
Thanks for the precisions. There is a design doc with a timeline here: https://docs.google.com/document/d/1cSW8fpJIUnibQKU8TMwLE5VxYZPh4u4LNu_wtkok8.... Our priority for the quarter is to have a prototype for the case where we have no live renderer, so the points of design where we go to the renderer and back at the start of the navigation could easily be discussed in parallel. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.h (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:27: // A url to be subsequently requested by the renderer as the main resource for On 2014/07/30 23:37:32, Charlie Reis wrote: > Naming it |stream_url| helps, thanks. Maybe "A unique url to be..." would help > further. (I would have said "A unique blob URL to be..." in the comment, but > I'm ok if Nasko wants to hide that detail.) Done. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/navigator_impl.cc:346: // RenderFrameHostManager in browser initiated navigation (part of project On 2014/07/30 23:37:32, Charlie Reis wrote: > Drop the "part of project PlzNavigate" here, since these params are used in the > normal case as well. Done. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/navigator_impl.cc:355: // As part of the PlzNavigate project, the RenderFrameHosts are no longer On 2014/07/30 23:37:32, Charlie Reis wrote: > // PlzNavigate: RenderFrameHosts are no longer... Done. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:49: FrameHostMsg_BeginNavigation_Params BeginNavigationFromNavigate( On 2014/07/30 23:37:32, Charlie Reis wrote: > // PlzNavigate Done. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:58: // TODO(clamy): This should be modified to take into account caching policy On 2014/07/30 23:37:32, Charlie Reis wrote: > Is this modeled after some existing code? I worry about them diverging and the > TODOs growing stale, so maybe it's worth mentioning the code it's based on. There is currently no unified code to do this on the renderer side (it is sprinkled about everywhere in blink). One of the OKRs for this quarter is to actually have this unified in content/common. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:607: frame_tree_node_->navigator()->GetController()->GetPendingEntry())); On 2014/07/30 23:37:33, Charlie Reis wrote: > We didn't have a dependency on GetPendingEntry before, since it was passed in. > (Navigate could be called even when there wasn't a pending entry.) Can we pass > it in the entry here? We can pass it in the case where we call it from Navigate. However, OnBeginNavigation can be called following a FrameHostMsg_BeginNavigation message coming from the renderer, in which case we don't have access to a navigation entry (and as of now FrameHostMsg_BeginNavigation does not give you enough parameters to create one). I reckon that this hackish, but I thought we could go back to this specific point after we have solved some outstanding questions regarding the start of the navigation when a renderer is present (eg do we fold FrameHostMsg_OpenURL into FrameHostMsg_BeginNavigation ?). Would a TODO: remove this dependency on GetPendingEntry be ok for the present? https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:628: frame_tree_node_->navigator()->GetController()->GetPendingEntry(); On 2014/07/30 23:37:32, Charlie Reis wrote: > Again, we didn't depend on GetPendingEntry() being correct at the time of commit > in the past, since it's possible that it didn't exist (e.g., in-page navigations > that commit synchronously). (There's one hack that looks at it in > NavigatorImpl::DidNavigate for OOPIF, but that's going away.) > > There's a separate controller_->RendererDidNavigate call in > NavigatorImpl::DidNavigate for updating the entry. In the current design, in-page navigations should still be handled on the renderer side (it is actually one of the reasons for going through the renderer at all in the first place). For passing the navigation entry, I guess the NavigationEntry we have in OnBeginNavigation could be stored in the NavigationRequest and reused there. It needs to be updated at that point to the url we navigated to after the redirects, in order to correctly determine which RFH to use. We should probably end up calling controller_->RendererDidNavigate in this function at one point, but we refrained from that for now, because we need to look at what exactely the CommitProvisionalLoad listeners in Blink do, and if they can handle being called after we already committed on the browser side. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:666: // TODO(clamy): the rfh_to_navigate should now send a commit IPC to the On 2014/07/30 23:37:32, Charlie Reis wrote: > I'm having trouble following the planned IPC timeline. Is there an updated > design doc that covers the planned behavior? Yes at https://docs.google.com/document/d/1cSW8fpJIUnibQKU8TMwLE5VxYZPh4u4LNu_wtkok8.... https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:450: // Returns the SiteInstance to use for the navigation. On 2014/07/30 23:37:33, Charlie Reis wrote: > I see-- this is a wrapper for GetSiteInstanceForEntry, which becomes a helper > function that is only called from here? > > In that case, we should definitely declare this right above > GetSiteInstanceForEntry and make it clear that callers want this and not > GetSiteInstanceForEntry. I would almost recommend combining them, except that > GetSiteInstanceForEntry is complicated enough already. Let's just make it clear > that it's solely a helper function. Done. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1867: // Browser-side navigation: Test that a proper NavigationRequest is created by On 2014/07/30 23:37:33, Charlie Reis wrote: > // PlzNavigate: Done. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1869: TEST_F(RenderFrameHostManagerTest, BrowserSideNavigationBeginNavigation) { On 2014/07/30 23:37:33, Charlie Reis wrote: > As before, let's settle on something consistent to make it easier for others to > follow. PlzNavigateBeginNavigation? > > (here and in the tests below) This was the initial name, but I was asked by jam@ to change it to something that did not feature the project name, such as BrowserSideNavigation (which is now the prefix for all the related test). https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1875: // eventually. On 2014/07/30 23:37:33, Charlie Reis wrote: > Mention why we have to do it later for now, since it's not clear at the moment. Done. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1888: subframe_rfh->frame_tree_node()->navigator()->GetController()->LoadURL( On 2014/07/30 23:37:33, Charlie Reis wrote: > The main frame and subframe share the same NavigationController. Why do we need > this here and contents()->GetController().LoadURL below? This is a hack to get a pending_navigation_entry in the controller, which we need in BeginNavigation. It should eventually go away. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1916: // Browser side navigation: Test that RequestNavigation creates a On 2014/07/30 23:37:33, Charlie Reis wrote: > // PlzNavigate: > (here and in all the tests below) Done. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/public/c... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/public/c... content/public/common/content_switches.cc:320: // Use the experimental browser-side navigation path (PlzNavigate project). On 2014/07/30 23:37:33, Charlie Reis wrote: > // PlzNavigate: Use the... Done.
Thanks-- the design doc helps. I feel like there's a lot of design-level discussions we should be having here before the code gets too complex. Some of the classes involved here have roles, like NavigatorImpl shielding RenderFrameHostManager and NavigationController both from others and from each other. That's eroding here, but should be easy to fix. Is it worthwhile to find a time for a VC? That might help us agree on an approach more quickly than the 24-hour code review turnaround, as well as discuss the "send to the renderer first" point. Some additional thoughts below. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:607: frame_tree_node_->navigator()->GetController()->GetPendingEntry())); On 2014/08/05 11:52:44, clamy wrote: > On 2014/07/30 23:37:33, Charlie Reis wrote: > > We didn't have a dependency on GetPendingEntry before, since it was passed in. > > > (Navigate could be called even when there wasn't a pending entry.) Can we > pass > > it in the entry here? > > We can pass it in the case where we call it from Navigate. However, > OnBeginNavigation can be called following a FrameHostMsg_BeginNavigation message > coming from the renderer, in which case we don't have access to a navigation > entry (and as of now FrameHostMsg_BeginNavigation does not give you enough > parameters to create one). I don't follow this part. Today, we already receive navigation requests from the renderer but don't need RFHM to know about the pending entry, because NavigatorImpl takes care of those details. > I reckon that this hackish, but I thought we could go > back to this specific point after we have solved some outstanding questions > regarding the start of the navigation when a renderer is present (eg do we fold > FrameHostMsg_OpenURL into FrameHostMsg_BeginNavigation ?). Would a TODO: remove > this dependency on GetPendingEntry be ok for the present? The point I was trying to make is that RenderFrameHostManager should generally not have to interact with NavigationController. NavigatorImpl is responsible for making sure that both NavigationController and RenderFrameHostManager do the right things at each stage. There's a few places where it leaks through today (e.g., a GetLastCommittedEntry call), but I think we should try to keep up this separation to keep the code modular and somewhat less spaghetti-like. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:628: frame_tree_node_->navigator()->GetController()->GetPendingEntry(); On 2014/08/05 11:52:44, clamy wrote: > On 2014/07/30 23:37:32, Charlie Reis wrote: > > Again, we didn't depend on GetPendingEntry() being correct at the time of > commit > > in the past, since it's possible that it didn't exist (e.g., in-page > navigations > > that commit synchronously). (There's one hack that looks at it in > > NavigatorImpl::DidNavigate for OOPIF, but that's going away.) > > > > There's a separate controller_->RendererDidNavigate call in > > NavigatorImpl::DidNavigate for updating the entry. > > In the current design, in-page navigations should still be handled on the > renderer side (it is actually one of the reasons for going through the renderer > at all in the first place). (Tangent: We should be able to determine if a navigation is in-page in the browser process and only send it to the renderer in that case.) > For passing the navigation entry, I guess the > NavigationEntry we have in OnBeginNavigation could be stored in the > NavigationRequest and reused there. It needs to be updated at that point to the > url we navigated to after the redirects, in order to correctly determine which > RFH to use. > > We should probably end up calling controller_->RendererDidNavigate in this > function at one point, but we refrained from that for now, because we need to > look at what exactely the CommitProvisionalLoad listeners in Blink do, and if > they can handle being called after we already committed on the browser side. NavigatorImpl should be the one updating the NavigationController. https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1888: subframe_rfh->frame_tree_node()->navigator()->GetController()->LoadURL( On 2014/08/05 11:52:44, clamy wrote: > On 2014/07/30 23:37:33, Charlie Reis wrote: > > The main frame and subframe share the same NavigationController. Why do we > need > > this here and contents()->GetController().LoadURL below? > > This is a hack to get a pending_navigation_entry in the controller, which we > need in BeginNavigation. It should eventually go away. Ok. I was just wondering why we had different ways of getting to it, so it looks good now. https://chromiumcodereview.appspot.com/379143002/diff/180001/content/browser/... File content/browser/frame_host/navigator.h (right): https://chromiumcodereview.appspot.com/379143002/diff/180001/content/browser/... content/browser/frame_host/navigator.h:123: virtual void CheckWebUIRendererDoesNotDisplayNormalURL( This is really a private helper function. We shouldn't need to expose it on Navigator. See my comment in RFHM::CommitPending. https://chromiumcodereview.appspot.com/379143002/diff/180001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:603: // Create a speculative RFH for the navigation if necessary. "if necessary" is confusing, since it's never necessary (if I understand correctly). https://chromiumcodereview.appspot.com/379143002/diff/180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:605: SiteInstance* current_instance = render_frame_host_->GetSiteInstance(); These SiteInstance* pointers get very subtle, as we're finding in https://codereview.chromium.org/446613003/. We either need to put them straight into scoped_refptrs or justify why that isn't necessary in a comment (as in UpdateStateForNavigate). Same in CommitNavigation below. https://chromiumcodereview.appspot.com/379143002/diff/180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:664: frame_tree_node_->navigator()->CheckWebUIRendererDoesNotDisplayNormalURL( This feels awkward to me. I don't think we should be exposing an internal check from NavigatorImpl to RenderFrameHostManager. NavigatorImpl should be the entry point (not RFHM), and it should be doing this check before calling CommitNavigation. https://chromiumcodereview.appspot.com/379143002/diff/180001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/180001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:529: scoped_ptr<RenderFrameHostImpl> speculative_render_frame_host_; I'm having trouble distinguishing this from pending_render_frame_host_. We should emphasize that it's purely an optimization and doesn't need to exist. Actually, I do wonder if it would be better to add it in a later CL. That would make the important parts of this CL easier to follow and make it clear to others that the later CL is just about optimization and not something necessary for functionality.
Here is a new version of the patch, which removes the dependency on NavigationControllerImpl::pending_entry_ as well as the speculative RFH optimization. PTAL. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:44: NavigationEntryImpl entry_; I am currently storing the NavigationEntry we need for CommitNavigation here, but I don't think it is very optimal (since we end up doing a copy of it). Another option would be to store it in the navigator. Wdyt? https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:641: SetRenderFrameHost(pending_render_frame_host_.Pass()); Right now the newly created RenderFrameHost for the navigation is placed in pending_render_frame_host_. We will eventually want to remove that, but I think it is better to wait to see how the speculative renderer spawning optimization ends up being implemented before doing so.
Great! I found this easier to follow, which might just be from reading it enough times. :) I was able to get down into the low-level parts of the review, so I think we're getting close to finished. Next set of questions below. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:17: // Holds the parameters determined during the load of a navigation request Let's add "// PlzNavigate: " here. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:18: // until receiving the response. Initialized on the IO thread, then passed to nit: until the response is received. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:16: // A UI thread object that owns a navigation request until it commits. It Is this class just for PlzNavigate? If so, let's add a // PlzNavigate comment. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:44: NavigationEntryImpl entry_; On 2014/08/08 12:02:46, clamy wrote: > I am currently storing the NavigationEntry we need for CommitNavigation here, > but I don't think it is very optimal (since we end up doing a copy of it). > Another option would be to store it in the navigator. Wdyt? Yeah, we can't store a copy because NavigationEntries are mutable. I'm not sure we need to store it at all. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigator.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigator.h:133: // Commit the navigation in the |render_frame_host|. See my request on RFHM::CommitPending about a more explicit description. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigator_impl.cc:656: scoped_ptr<NavigationEntryImpl> entry( I don't think this flow is quite right yet. In the past, navigation requests came into NavigationController::LoadURL (via a convoluted OpenURL chain which handles things like opening in a new tab, etc). NavigationController created a NavigationEntry and assigned it to be the pending one, and then Navigator heard about it via NavigateToPendingEntry. Navigator never needed to create the entry itself. In fact, it looks like you're already planning to go through LoadURL for browser-initiated navigations, as shown in the unit tests. If this is for renderer-initiated navigations (in an attempt to skip the crazy OpenURL chain of events), then maybe it makes more sense for this to call NavigationController::LoadURL? That would take care of setting up the right pending entry state. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigator_impl.cc:676: // TODO(clamy): investigate if the NavigationController should be made aware Sorry-- if I suggested this, I was confused by the name (thinking the commit had already happened). Assuming this happens when the response is ready to commit and before anything has actually committed in the renderer, the NavigationController doesn't need to know about it. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:51: FrameHostMsg_BeginNavigation_Params BeginNavigationFromNavigate( Let's add a comment here, since it's confusing to see a function that transforms a renderer-bound message (FrameMsg) into a browser-bound message (FrameHostMsg). I think the intention is that it's simulating the response from the renderer when the renderer doesn't exist? https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:244: InitRenderFrameHostsBeforeNavigation(render_frame_host_.get()); I don't see why we're making this call in RequestNavigation. We don't have a destination RenderFrameHost yet, and passing the current RFH as |dest_render_frame_host| makes the code in InitRenderFrameHostsBeforeNavigation do unexpected things (e.g., skip the sad tab check and unexpectedly do more if the current RFH is dead). Is there a reason it's needed? (If not, we probably don't need to make it into a helper function.) https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:616: navigation_request_->BeginNavigation(params.request_body); Looks like this goes to RDH::NavigationRequest, which is NOTIMPLEMENTED. How does the test pass? https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:626: navigation_request_->UpdateEntryForCommit(info.navigation_url); Since we haven't actually committed yet, I don't think we want to update the NavigationEntry yet. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:630: SiteInstance* new_instance = GetSiteInstanceForNavigation( I'm really excited about waiting until this point to pick the SiteInstance! No more getting it wrong due to server redirects. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:634: if (current_instance != new_instance) { We may want a TODO to update how pending WebUI objects are handled. (See the code in the similar spot of UpdateStateForNavigate.) https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:638: // TODO(clamy): The old RenderFrameHost should be asked to run its unload Side note: I'm thinking we want to wait until the navigation has committed in the renderer for this. I'm working on that in http://crbug.com/402020. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:641: SetRenderFrameHost(pending_render_frame_host_.Pass()); On 2014/08/08 12:02:46, clamy wrote: > Right now the newly created RenderFrameHost for the navigation is placed in > pending_render_frame_host_. We will eventually want to remove that, but I think > it is better to wait to see how the speculative renderer spawning optimization > ends up being implemented before doing so. Acknowledged. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:991: RenderFrameHostImpl* RenderFrameHostManager:: It feels awkward to return a raw pointer of something that's stored in pending_render_frame_host_'s scoped_ptr as a side effect. Perhaps we should make this void and just have callers check pending_render_frame_host_ afterward. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1152: if (!swapped_out) { Nit: I don't think this change is necessary. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:326: // PlzNavigate: Used to start a navigation. Let's explain whether this happens before or after RequestNavigation. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:335: // PlzNavigate: Used to signal the commit of a navigation request. In retrospect, this comment isn't very clear, partly because of how the code used to work. The name sounds like CommitPending, which occurs *after* the navigation has committed in the renderer. In contrast, this function is closer to the current OnCrossSiteResponse, which means the network response is ready to commit but hasn't yet. I'm not sure I have a better suggestion for the name (CommitNavigation and DidCommitNavigation might end up as a reasonable pair of functions), but we should at least be very explicit about which point of the timeline this represents in the comment. (This also applies in navigator.h.) https://chromiumcodereview.appspot.com/379143002/diff/240001/content/common/f... File content/common/frame_messages.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/common/f... content/common/frame_messages.h:274: IPC_STRUCT_BEGIN(FrameHostMsg_BeginNavigation_Params) // PlzNavigate
Thanks! I think the issue of the NavigationEntryImpl is still not clear. We need it at commit time to properly determine which site instance to use. However, it was at best passed to RequestNavigation (which is called through NavigatorImpl::LoadUrl). We can't depend on pending_navigation_entry_ in the NavigationController being correct, especially as this is supposed to work for subframes as well. So I think we need to store it somewhere (navigator?). Wdyt? https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:17: // Holds the parameters determined during the load of a navigation request On 2014/08/08 20:25:27, Charlie Reis wrote: > Let's add "// PlzNavigate: " here. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:18: // until receiving the response. Initialized on the IO thread, then passed to On 2014/08/08 20:25:27, Charlie Reis wrote: > nit: until the response is received. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:16: // A UI thread object that owns a navigation request until it commits. It On 2014/08/08 20:25:27, Charlie Reis wrote: > Is this class just for PlzNavigate? If so, let's add a // PlzNavigate comment. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:44: NavigationEntryImpl entry_; On 2014/08/08 20:25:27, Charlie Reis wrote: > On 2014/08/08 12:02:46, clamy wrote: > > I am currently storing the NavigationEntry we need for CommitNavigation here, > > but I don't think it is very optimal (since we end up doing a copy of it). > > Another option would be to store it in the navigator. Wdyt? > > Yeah, we can't store a copy because NavigationEntries are mutable. I'm not sure > we need to store it at all. We need a navigation entry inside CommitNavigation. I don't really see how to have one unless it is stored somewhere. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigator.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigator.h:133: // Commit the navigation in the |render_frame_host|. On 2014/08/08 20:25:27, Charlie Reis wrote: > See my request on RFHM::CommitPending about a more explicit description. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigator_impl.cc:656: scoped_ptr<NavigationEntryImpl> entry( On 2014/08/08 20:25:28, Charlie Reis wrote: > I don't think this flow is quite right yet. In the past, navigation requests > came into NavigationController::LoadURL (via a convoluted OpenURL chain which > handles things like opening in a new tab, etc). NavigationController created a > NavigationEntry and assigned it to be the pending one, and then Navigator heard > about it via NavigateToPendingEntry. Navigator never needed to create the entry > itself. > > In fact, it looks like you're already planning to go through LoadURL for > browser-initiated navigations, as shown in the unit tests. > > If this is for renderer-initiated navigations (in an attempt to skip the crazy > OpenURL chain of events), then maybe it makes more sense for this to call > NavigationController::LoadURL? That would take care of setting up the right > pending entry state. We are still going through LoadURL for browser initiated navigations. For renderer-initiated navigations, we have not decided yet whether we should make the OpenURL and the BeginNavigation IPC should actually be just one IPC. So the creation of the NavigationEntryImpl below is more of a hack until we get better visibility of how exactely renderer-initiated navigations are going to work. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/navigator_impl.cc:676: // TODO(clamy): investigate if the NavigationController should be made aware On 2014/08/08 20:25:28, Charlie Reis wrote: > Sorry-- if I suggested this, I was confused by the name (thinking the commit had > already happened). > > Assuming this happens when the response is ready to commit and before anything > has actually committed in the renderer, the NavigationController doesn't need to > know about it. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:51: FrameHostMsg_BeginNavigation_Params BeginNavigationFromNavigate( On 2014/08/08 20:25:28, Charlie Reis wrote: > Let's add a comment here, since it's confusing to see a function that transforms > a renderer-bound message (FrameMsg) into a browser-bound message (FrameHostMsg). > I think the intention is that it's simulating the response from the renderer > when the renderer doesn't exist? Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:244: InitRenderFrameHostsBeforeNavigation(render_frame_host_.get()); On 2014/08/08 20:25:29, Charlie Reis wrote: > I don't see why we're making this call in RequestNavigation. We don't have a > destination RenderFrameHost yet, and passing the current RFH as > |dest_render_frame_host| makes the code in InitRenderFrameHostsBeforeNavigation > do unexpected things (e.g., skip the sad tab check and unexpectedly do more if > the current RFH is dead). > > Is there a reason it's needed? (If not, we probably don't need to make it into > a helper function.) Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:616: navigation_request_->BeginNavigation(params.request_body); On 2014/08/08 20:25:28, Charlie Reis wrote: > Looks like this goes to RDH::NavigationRequest, which is NOTIMPLEMENTED. How > does the test pass? Apparently NOTIMPLEMENTED just prints an error message and does not cause the test to fail, even on Debug builds. However, in the future it would be better to somehow mock the call to RDH::NavigationRequest. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:626: navigation_request_->UpdateEntryForCommit(info.navigation_url); On 2014/08/08 20:25:28, Charlie Reis wrote: > Since we haven't actually committed yet, I don't think we want to update the > NavigationEntry yet. We don't get the new site instance right if we don't update the url. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:634: if (current_instance != new_instance) { On 2014/08/08 20:25:28, Charlie Reis wrote: > We may want a TODO to update how pending WebUI objects are handled. (See the > code in the similar spot of UpdateStateForNavigate.) Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:638: // TODO(clamy): The old RenderFrameHost should be asked to run its unload On 2014/08/08 20:25:28, Charlie Reis wrote: > Side note: I'm thinking we want to wait until the navigation has committed in > the renderer for this. I'm working on that in http://crbug.com/402020. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:991: RenderFrameHostImpl* RenderFrameHostManager:: On 2014/08/08 20:25:28, Charlie Reis wrote: > It feels awkward to return a raw pointer of something that's stored in > pending_render_frame_host_'s scoped_ptr as a side effect. Perhaps we should > make this void and just have callers check pending_render_frame_host_ afterward. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1152: if (!swapped_out) { On 2014/08/08 20:25:29, Charlie Reis wrote: > Nit: I don't think this change is necessary. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:326: // PlzNavigate: Used to start a navigation. On 2014/08/08 20:25:29, Charlie Reis wrote: > Let's explain whether this happens before or after RequestNavigation. Done. https://chromiumcodereview.appspot.com/379143002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:335: // PlzNavigate: Used to signal the commit of a navigation request. On 2014/08/08 20:25:29, Charlie Reis wrote: > In retrospect, this comment isn't very clear, partly because of how the code > used to work. The name sounds like CommitPending, which occurs *after* the > navigation has committed in the renderer. In contrast, this function is closer > to the current OnCrossSiteResponse, which means the network response is ready to > commit but hasn't yet. > > I'm not sure I have a better suggestion for the name (CommitNavigation and > DidCommitNavigation might end up as a reasonable pair of functions), but we > should at least be very explicit about which point of the timeline this > represents in the comment. > > (This also applies in navigator.h.) Is the comment now clearer? https://chromiumcodereview.appspot.com/379143002/diff/240001/content/common/f... File content/common/frame_messages.h (right): https://chromiumcodereview.appspot.com/379143002/diff/240001/content/common/f... content/common/frame_messages.h:274: IPC_STRUCT_BEGIN(FrameHostMsg_BeginNavigation_Params) On 2014/08/08 20:25:29, Charlie Reis wrote: > // PlzNavigate Done.
On 2014/08/12 12:13:16, clamy wrote: > Thanks! I think the issue of the NavigationEntryImpl is still not clear. We need > it at commit time to properly determine which site instance to use. However, it > was at best passed to RequestNavigation (which is called through > NavigatorImpl::LoadUrl). We can't depend on pending_navigation_entry_ in the > NavigationController being correct, especially as this is supposed to work for > subframes as well. So I think we need to store it somewhere (navigator?). Wdyt? > I think storing the NavigationEntry outside of NavigationController is problematic and we shouldn't do it. If we store a copy it will get stale when other operations update the one in NavigationController, and if we store a pointer to the pending entry we risk having it deleted underneath us. I'm getting the sense that RenderFrameHostManager has an unnecessary dependency on the entry itself, which is causing us trouble as we try to rearrange things. Most of what it needs the entry for is the URL and SiteInstance (and occasionally a few other things), which could probably be passed in. Removing that dependency would be work for a different CL, though. Can you help me understand the problem with using the NavigationController's pending entry in the short term? I'm confused by your mention of subframe navigations, since I don't see how the PlzNavigate code handles those yet. (If we're really just creating a standalone entry to pass to RFHM that will never end up in NavigationController, then that argues for removing RFHM's dependency on the entry. Maybe making that explicit and storing the throw-away NavigationEntry on NavigationRequest in the short term would work, as long as we make that code go away.) https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:664: // TODO(clamy): Update how pending WebUI objects rae handled. nit: are https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:327: // direclty by RequestNavigation when there is no live-renderer. Otherwise, it nit: directly nit: no hyphen in live renderer
Some drive-by nits. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/navigator_impl.h (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/navigator_impl.h:78: nit: no new line needed https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:57: "POST" : "GET"; nit: these should fit in the previous line https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:331: void OnBeginNavigation(const FrameHostMsg_BeginNavigation_Params& params, nit: it will be nice to put all PlzNavigate methods close to each other in the file instead of randomly spread around. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1919: main_test_rfh()->frame_tree_node()->render_manager()); nit: two more spaces for indent https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1930: main_test_rfh()->frame_tree_node()->render_manager()); nit: main_test_rfh()->frame_tree_node()->render_manager() is used in a few places. It might be more readable to have a local variable for it and use it, as it will not change across navigations. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1932: // The main rfh should not have been changed and we should not have a nit: capitalize rfh
Right now we are creating a copy of the NavigationEntry that is stored in NavigationRequest and is deleted when NavigationRequest goes away (currently in CommitNavigation, but probably that should be when the renderer has actually navigated). I agree that it should not be a definitive solution, and will work on removing the dependency on the NavigationEntry in a separate CL (it is mostly to determine which SiteInstance to use). The main problem with using the pending entry, is that this code (OnBeginNavigation and CommitNavigation at least) should be mostly the same for main frames and subframes navigation. AFAIU all frames within a tree share the same NavigationController, and the pending entry is meant to represent the main frame navigation. So if we use it, that means we can only use our code for main frames, which we don't want. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/navigator_impl.h (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/navigator_impl.h:78: On 2014/08/13 00:08:00, nasko wrote: > nit: no new line needed Done. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:57: "POST" : "GET"; On 2014/08/13 00:08:00, nasko wrote: > nit: these should fit in the previous line Done. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:664: // TODO(clamy): Update how pending WebUI objects rae handled. On 2014/08/12 19:04:50, Charlie Reis wrote: > nit: are Done. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:327: // direclty by RequestNavigation when there is no live-renderer. Otherwise, it On 2014/08/12 19:04:50, Charlie Reis wrote: > nit: directly > nit: no hyphen in live renderer Done. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:331: void OnBeginNavigation(const FrameHostMsg_BeginNavigation_Params& params, On 2014/08/13 00:08:00, nasko wrote: > nit: it will be nice to put all PlzNavigate methods close to each other in the > file instead of randomly spread around. Done. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1919: main_test_rfh()->frame_tree_node()->render_manager()); On 2014/08/13 00:08:00, nasko wrote: > nit: two more spaces for indent Done. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1930: main_test_rfh()->frame_tree_node()->render_manager()); On 2014/08/13 00:08:00, nasko wrote: > nit: main_test_rfh()->frame_tree_node()->render_manager() is used in a few > places. It might be more readable to have a local variable for it and use it, as > it will not change across navigations. Done. https://chromiumcodereview.appspot.com/379143002/diff/260001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1932: // The main rfh should not have been changed and we should not have a On 2014/08/13 00:08:00, nasko wrote: > nit: capitalize rfh Done.
PTAL at the rebase on top of https://chromiumcodereview.appspot.com/471603002/ (the patch that removes the dependency on NavigationEntryImpl to get a SiteInstance).
Awesome-- that's so much nicer without having to pass otherwise-unneeded NavigationEntries around. LGTM. https://chromiumcodereview.appspot.com/379143002/diff/340001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/340001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:649: // TODO(clamy): Store the site instance, if the navigation is view source mode Why will we need to do this? https://chromiumcodereview.appspot.com/379143002/diff/340001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/340001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:443: // Returns an appropriate SiteInstance object for the given |dest_url|, Nit: Don't forget to call this one out as a helper function for GetSiteInstanceForNavigation, which got lost in the rebase.
Thnaks! https://chromiumcodereview.appspot.com/379143002/diff/340001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/379143002/diff/340001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:649: // TODO(clamy): Store the site instance, if the navigation is view source mode On 2014/08/15 06:13:47, Charlie Reis wrote: > Why will we need to do this? We may need to store some of them when we get the NavigationEntry (in RequestNavigation) to reuse here. I updated the comment since it wasn't clear. https://chromiumcodereview.appspot.com/379143002/diff/340001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/379143002/diff/340001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:443: // Returns an appropriate SiteInstance object for the given |dest_url|, On 2014/08/15 06:13:47, Charlie Reis wrote: > Nit: Don't forget to call this one out as a helper function for > GetSiteInstanceForNavigation, which got lost in the rebase. Done.
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/379143002/360001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/379143002/360001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/379143002/360001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/379143002/360001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/379143002/360001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/379143002/380001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Message was sent while issue was closed.
Committed patchset #17 (380001) as 290570 |
