Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
946543003:
PlzNavigate: have renderer-initiated navigations be same-process (Closed)
|
Created:
5 years, 10 months ago by carlosk Modified:
5 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: PlzNavigate: have renderer-initiated navigations be same-process.
To comply with the current navigation behavior this change makes it so
that render-initiated navigations do not create a new renderer. Once
OOPIF add proper support, this change should be reverted.
BUG=440266
Committed: https://crrev.com/42f59e67e6c4d58a913d48b3a81600cdcd3ced88
Cr-Commit-Position: refs/heads/master@{#320763}
Patch Set 1 : #
Total comments: 36
Patch Set 2 : Integrating changes from other refactors and fully fixing affected tests. #
Total comments: 10
Patch Set 3 : New test, moved out change and other minor changes. #
Total comments: 8
Patch Set 4 : Comment changes and browsertest fix. #
Total comments: 10
Patch Set 5 : Comment changes. #
Total comments: 2
Patch Set 6 : Minor test and comment changes. #Patch Set 7 : Removed SiteInstance direct URL check from tests. #
Total comments: 13
Patch Set 8 : Rebase #Patch Set 9 : Comment changes #
Total comments: 2
Messages
Total messages: 30 (8 generated)
Patchset #1 (id:1) has been deleted
carlosk@chromium.org changed reviewers: + clamy@chromium.org, fdegans@chromium.org
carlosk@chromium.org changed required reviewers: + clamy@chromium.org
clamy@, fdegans@: PTAL. Please see description and inline comments. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:347: main_test_rfh()->RendererRequestIfPlzNavigate(urls[0], true); For each SendNavigate call the intent might be different as it might be: * A reply from the renderer carrying on with a browser-initiated navigation * Same as above but after one or more server redirects happened * A renderer-initiated user-initiated navigation * A renderer-initiated non-user-initiated navigation As those cases are handled differently in PlzNavigate world I did my best to interpret each situation. I made comments on a few cases but please let me know if you notice something I missed. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:1017: main_test_rfh()->RendererRequestIfPlzNavigate(kExistingURL, true); This one I am really unsure about. From test comment it seems like a non-user-initiated navigation. Now from the referred bug's description it seems like it would be triggered by a re-layout of the page that I think could be caused by the user re-sizing the window so user initiated? I assumed this was the case. Ideas? https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:1282: SimulateServerRedirectIfPlzNavigate(main_test_rfh(), url2); I guess that "a reload navigation produces a new page" means that the user did hit Reload but the server redirected it to a new page. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:1572: SimulateServerRedirectIfPlzNavigate(main_test_rfh(), url3); Same as previous comment for "a reload navigation produces a new page" but now for "back". https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:2667: std::string()); The previous URL value here seemed to not be what the author meant to use. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:2849: main_test_rfh()->RendererRequestIfPlzNavigate(url3, true); Again not really sure about these ones in this test. I assumed they were all renderer-initiated requests. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_delegate.h (left): https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_delegate.h:91: RenderFrameHostImpl* render_frame_host, I removed this parameter because a) it was not being used anyway and b) it was not even available where PlzNavigate needed to make that call. https://codereview.chromium.org/946543003/diff/20001/content/test/test_render... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/946543003/diff/20001/content/test/test_render... content/test/test_render_frame_host.cc:244: CHECK(url_loader); These two replacements with CHECK are a bugfix as the previously used ASSERTs there will *not* trigger a test failure (see https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_S...)
Thanks! A few comments. Also for the commit name, maybe have renderer-initiated navigations be same-process? On the first line of the description "this change makes it so". I think the list of tests being fixed should not necessarily be in the description of the commit (but you can include it in the description you send to reviewers). Also remove the last line, since a patch should not include related refactorings (they should be in another patch). https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:249: void SimulateServerRedirectIfPlzNavigate(TestRenderFrameHost* test_rfh, Please rename that method SimulateServerRedirect and add a // PlzNavigate comment above. We do not want to have the name PlzNavigate anywhere in function names and variables. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:265: CHECK that the request status is STARTED here (the patch on data urls introduce a path that may result in the request going from WAITING_FOR_RENDERER_RESPONSE to RESPONSE_STARTED). https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:1017: main_test_rfh()->RendererRequestIfPlzNavigate(kExistingURL, true); On 2015/02/20 19:16:51, carlosk wrote: > This one I am really unsure about. From test comment it seems like a > non-user-initiated navigation. > Now from the referred bug's description it seems like it would be triggered by a > re-layout of the page that I think could be caused by the user re-sizing the > window so user initiated? I assumed this was the case. > Ideas? The way blink determines if a request has a user gesture is whether it was processing a user gesture at the same time. So if it is coming from a resize then I guess it would be user initiated, but it could also be coming from pure javascript in which case it would not. In any case, we expect a commit here, so I guess it should probably stay as user-initiated. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:1282: SimulateServerRedirectIfPlzNavigate(main_test_rfh(), url2); On 2015/02/20 19:16:51, carlosk wrote: > I guess that "a reload navigation produces a new page" means that the user did > hit Reload but the server redirected it to a new page. Acknowledged. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:1572: SimulateServerRedirectIfPlzNavigate(main_test_rfh(), url3); On 2015/02/20 19:16:51, carlosk wrote: > Same as previous comment for "a reload navigation produces a new page" but now > for "back". Acknowledged. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:2667: std::string()); On 2015/02/20 19:16:51, carlosk wrote: > The previous URL value here seemed to not be what the author meant to use. Acknowledged. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:2849: main_test_rfh()->RendererRequestIfPlzNavigate(url3, true); On 2015/02/20 19:16:51, carlosk wrote: > Again not really sure about these ones in this test. I assumed they were all > renderer-initiated requests. Acknowledged. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_delegate.h (left): https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_delegate.h:91: RenderFrameHostImpl* render_frame_host, On 2015/02/20 19:16:51, carlosk wrote: > I removed this parameter because a) it was not being used anyway and b) it was > not even available where PlzNavigate needed to make that call. Is this necessary for this patch? If not, I would prefer to have it in a different CL. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:860: if (delegate_) As in the comment in the header file, I don't think this is necessary for this patch, and would prefer to have it in another CL. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:561: // TODO(carlosk): Once OOPIF adds support for renderer-initiated cross-process I think the TODO is not needed here. The test will fail anyway if we reintroduce cross-process renderer-initiated navigations, and I'm not sure the TODO will be understandable a few months from now. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:563: ASSERT_FALSE(GetSpeculativeRenderFrameHost(node)); I would use an EXPECT instead of an ASSERT here. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:601: // navigations, re-add existence and update checks for the speculative RFH. Same here, no need for the TODO here and below. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:613: ASSERT_FALSE(GetSpeculativeRenderFrameHost(node)); I would use an EXPECT here and below. https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:707: // TODO(carlosk): Remove the renderer-initiated if-check below once OOPIF I would rephrase the comment as: Once there is support for cross-process scripting remove the check for renderer-initiated navigations... https://codereview.chromium.org/946543003/diff/20001/content/test/test_render... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/946543003/diff/20001/content/test/test_render... content/test/test_render_frame_host.cc:214: void TestRenderFrameHost::PrepareForCommit(const GURL& url) { Now that url is not used in that function, it should be removed from the parameters (it was only included because of the SendBeginNavigation there before). https://codereview.chromium.org/946543003/diff/20001/content/test/test_render... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/946543003/diff/20001/content/test/test_render... content/test/test_render_frame_host.h:89: void RendererRequestIfPlzNavigate(const GURL& url, bool has_user_gesture); This function should not have PlzNavigate in its name (instead add a comment with // PlzNavigate on the line above). Also its name is not very clear, how about SendRendererInitiatedNavigationRequest?
After a couple of refactoring CLs that were split off of this one, this CL looks
way simpler. Quite some comments don't apply anymore because of that for which I
replied stating it.
These previously failing tests now pass:
NavigationControllerTest.ReloadOriginalRequestURL
NavigationControllerTest.Forward
NavigationControllerTest.Forward_GeneratesNewPage
NavigationControllerTest.LoadURL_NewPending
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:249: void
SimulateServerRedirectIfPlzNavigate(TestRenderFrameHost* test_rfh,
On 2015/02/23 10:52:34, clamy wrote:
> Please rename that method SimulateServerRedirect and add a // PlzNavigate
> comment above. We do not want to have the name PlzNavigate anywhere in
function
> names and variables.
Change was moved to another CL.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:265:
On 2015/02/23 10:52:34, clamy wrote:
> CHECK that the request status is STARTED here (the patch on data urls
introduce
> a path that may result in the request going from WAITING_FOR_RENDERER_RESPONSE
> to RESPONSE_STARTED).
Change was moved to another CL.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1017:
main_test_rfh()->RendererRequestIfPlzNavigate(kExistingURL, true);
On 2015/02/23 10:52:34, clamy wrote:
> On 2015/02/20 19:16:51, carlosk wrote:
> > This one I am really unsure about. From test comment it seems like a
> > non-user-initiated navigation.
> > Now from the referred bug's description it seems like it would be triggered
by
> a
> > re-layout of the page that I think could be caused by the user re-sizing the
> > window so user initiated? I assumed this was the case.
> > Ideas?
>
> The way blink determines if a request has a user gesture is whether it was
> processing a user gesture at the same time. So if it is coming from a resize
> then I guess it would be user initiated, but it could also be coming from pure
> javascript in which case it would not. In any case, we expect a commit here,
so
> I guess it should probably stay as user-initiated.
Ack. Also with the current cancellation policy a non-user-initiated request here
would be ignored.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_delegate.h (left):
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_delegate.h:91: RenderFrameHostImpl*
render_frame_host,
On 2015/02/23 10:52:34, clamy wrote:
> On 2015/02/20 19:16:51, carlosk wrote:
> > I removed this parameter because a) it was not being used anyway and b) it
was
> > not even available where PlzNavigate needed to make that call.
>
> Is this necessary for this patch? If not, I would prefer to have it in a
> different CL.
Change was moved to another CL.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:860: if (delegate_)
On 2015/02/23 10:52:34, clamy wrote:
> As in the comment in the header file, I don't think this is necessary for this
> patch, and would prefer to have it in another CL.
Change was moved to another CL.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl_unittest.cc (right):
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:561: // TODO(carlosk):
Once OOPIF adds support for renderer-initiated cross-process
On 2015/02/23 10:52:34, clamy wrote:
> I think the TODO is not needed here. The test will fail anyway if we
reintroduce
> cross-process renderer-initiated navigations, and I'm not sure the TODO will
be
> understandable a few months from now.
Done.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:563:
ASSERT_FALSE(GetSpeculativeRenderFrameHost(node));
On 2015/02/23 10:52:34, clamy wrote:
> I would use an EXPECT instead of an ASSERT here.
Done here and in 2 other places below.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:601: // navigations,
re-add existence and update checks for the speculative RFH.
On 2015/02/23 10:52:34, clamy wrote:
> Same here, no need for the TODO here and below.
Done.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:613:
ASSERT_FALSE(GetSpeculativeRenderFrameHost(node));
On 2015/02/23 10:52:34, clamy wrote:
> I would use an EXPECT here and below.
Done.
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):
https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:707: // TODO(carlosk):
Remove the renderer-initiated if-check below once OOPIF
On 2015/02/23 10:52:34, clamy wrote:
> I would rephrase the comment as:
> Once there is support for cross-process scripting remove the check for
> renderer-initiated navigations...
Done.
https://codereview.chromium.org/946543003/diff/20001/content/test/test_render...
File content/test/test_render_frame_host.cc (right):
https://codereview.chromium.org/946543003/diff/20001/content/test/test_render...
content/test/test_render_frame_host.cc:214: void
TestRenderFrameHost::PrepareForCommit(const GURL& url) {
On 2015/02/23 10:52:34, clamy wrote:
> Now that url is not used in that function, it should be removed from the
> parameters (it was only included because of the SendBeginNavigation there
> before).
Change was moved to another CL.
https://codereview.chromium.org/946543003/diff/20001/content/test/test_render...
File content/test/test_render_frame_host.h (right):
https://codereview.chromium.org/946543003/diff/20001/content/test/test_render...
content/test/test_render_frame_host.h:89: void
RendererRequestIfPlzNavigate(const GURL& url, bool has_user_gesture);
On 2015/02/23 10:52:34, clamy wrote:
> This function should not have PlzNavigate in its name (instead add a comment
> with // PlzNavigate on the line above). Also its name is not very clear, how
> about SendRendererInitiatedNavigationRequest?
Change was moved to another CL.
https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl_unittest.cc (right):
https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:958:
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
I removed the call to SendNavigate because in the context of this test I didn't
find anything that needed to be verified after it. Any suggestions?
Thanks! It's looking good, a few minor comments. https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:857: if (delegate_) This does not seem related to the change at hand (unlike adding the proper calls to SendRendererInitiatedNavigation in the unit tests). Could you have it in a separate patch? https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:155: // PlzNavigate: Test a complete renderer-initiated same-site navigation. Could you add a test for a complete renderer-initiated navigation between two urls that would be cross-site if it were browser-initiated to match this test here? https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:545: // Confirm a speculative RenderFrameHost was created. nit: "Confirm that" https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:560: // Confirm that a the speculative RenderFrameHost was destroyed. Remove the "a" from the comment. https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:958: EXPECT_FALSE(GetSpeculativeRenderFrameHost(node)); On 2015/03/04 19:42:41, carlosk wrote: > I removed the call to SendNavigate because in the context of this test I didn't > find anything that needed to be verified after it. Any suggestions? Acknowledged.
Thanks! https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:857: if (delegate_) On 2015/03/05 10:44:04, clamy wrote: > This does not seem related to the change at hand (unlike adding the proper calls > to SendRendererInitiatedNavigation in the unit tests). Could you have it in a > separate patch? Yes, I noticed the apparent oddness of this before but I wasn't sure about removing it. But I just ran a simple test that made this clear: * Running content_unittests with this fix isolated from the rest results in one fixed test * Running content_unittests with all other changes but this fix results in all other expected test to succeed And this concludes its effects are completely isolated. I'll create a new CL for this one. https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:155: // PlzNavigate: Test a complete renderer-initiated same-site navigation. On 2015/03/05 10:44:05, clamy wrote: > Could you add a test for a complete renderer-initiated navigation between two > urls that would be cross-site if it were browser-initiated to match this test > here? Done. https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:545: // Confirm a speculative RenderFrameHost was created. On 2015/03/05 10:44:05, clamy wrote: > nit: "Confirm that" Done. https://codereview.chromium.org/946543003/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:560: // Confirm that a the speculative RenderFrameHost was destroyed. On 2015/03/05 10:44:05, clamy wrote: > Remove the "a" from the comment. Done.
Thanks! A few comments. You should also modify BrowserSideNavigationBrowserTest.RendererInitiatedCrossSiteNavigation so that it expects the RFH not to change on the renderer-initiated navigation. https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:858: delegate_->DidStartNavigationToPendingEntry(entry.GetURL(), reload_type); Don't forget to remove this :). https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:197: // PlzNavigate: Test a complete renderer-initiated cross-site navigation. I think you cannot call it a cross-site navigation since it does not change SiteInstances. How about "Test that a navigation that should be cross-site does not result in a SiteInstance swap if it is renderer-initiated."? https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:235: // As the RenderFrameHost didn't change, the SiteInstance is still the same. Technically this is more the reverse: the SiteInstance did not change so the RenderFrameHost is still the same. I'm not sure about adding this last check, this is more like testing that RenderFrameHost works as expected rather than testing naviagtion IMO. Same below. https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:723: // TODO(carlosk): Once there is support for cross-process scripting remove the I think you should add a little more explanation on why this is safe to do. Something like: "Renderer-initiated navigations that may require a SiteInstance swap are sent to the browser via the OpenURL IPC, and are afterwards treated as browser-initiated navigations. The only NavigationRequests marked as renderer-initiated were created after receiving a BeginNavigation IPC, and should proceed in the same renderer that sent the IPC."
Patchset #4 (id:80001) has been deleted
Thanks. Especially for the tip on the browsertest. https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:858: delegate_->DidStartNavigationToPendingEntry(entry.GetURL(), reload_type); On 2015/03/06 13:43:09, clamy wrote: > Don't forget to remove this :). Sorry about that. *Really* done now. https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:197: // PlzNavigate: Test a complete renderer-initiated cross-site navigation. On 2015/03/06 13:43:09, clamy wrote: > I think you cannot call it a cross-site navigation since it does not change > SiteInstances. How about "Test that a navigation that should be cross-site does > not result in a SiteInstance swap if it is renderer-initiated."? Done. https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:235: // As the RenderFrameHost didn't change, the SiteInstance is still the same. On 2015/03/06 13:43:09, clamy wrote: > Technically this is more the reverse: the SiteInstance did not change so the > RenderFrameHost is still the same. I'm not sure about adding this last check, > this is more like testing that RenderFrameHost works as expected rather than > testing naviagtion IMO. Same below. Acknowledged; I updated the comment here and the similar one down below. Even though it's not strictly needed for these cases I kept the check to a) make this one look like a mirror from the same-site test (just above) that it was based off and b) for "educational" reasons, to make it clear for someone looking at the test that the SiteInstance didn't change and that there's a mismatch with the navigated URL. https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:723: // TODO(carlosk): Once there is support for cross-process scripting remove the On 2015/03/06 13:43:09, clamy wrote: > I think you should add a little more explanation on why this is safe to do. > Something like: "Renderer-initiated navigations that may require a SiteInstance > swap are sent to the browser via the OpenURL IPC, and are afterwards treated as > browser-initiated navigations. The only NavigationRequests marked as > renderer-initiated were created after receiving a BeginNavigation IPC, and > should proceed in the same renderer that sent the IPC." Done. I didn't realized/remembered that situation but indeed an "open in new tab" renderer initiated navigation will follow that other path.
Thanks! https://codereview.chromium.org/946543003/diff/100001/content/browser/browser... File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/browser... content/browser/browser_side_navigation_browsertest.cc:157: // The RenderFrameHost should have changed. Change the comment to "should not have changed." https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:237: // As the SiteInstance didn't change, the site URL is still the same as Maybe "The SiteInstance did not change, therefore its site URL is still the same as the one used to create it before." But to me, a more interesting test (since the site URL is a bit limitated) would be to compare the original SiteInstance ID with the one at the end and they should match, hence making it same-site. https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:728: // the IPC due to the condition below. Once there is support for cross-process I prefer leaving the former TODO here.
Thanks again. https://codereview.chromium.org/946543003/diff/100001/content/browser/browser... File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/browser... content/browser/browser_side_navigation_browsertest.cc:157: // The RenderFrameHost should have changed. On 2015/03/06 15:57:50, clamy wrote: > Change the comment to "should not have changed." Done. https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:237: // As the SiteInstance didn't change, the site URL is still the same as On 2015/03/06 15:57:50, clamy wrote: > Maybe "The SiteInstance did not change, therefore its site URL is still the same > as the one used to create it before." But to me, a more interesting test (since > the site URL is a bit limitated) would be to compare the original SiteInstance > ID with the one at the end and they should match, hence making it same-site. I updated the comment. The information I think is important to convey here is that even though this last navigation was made to an apparent "http://google.com" site URL, the SiteInstance is still pointing to "http://chromium.org". Switching to a site_id comparison would not present that fact as clearly. https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:728: // the IPC due to the condition below. Once there is support for cross-process On 2015/03/06 15:57:50, clamy wrote: > I prefer leaving the former TODO here. Done.
Thanks! https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:237: // As the SiteInstance didn't change, the site URL is still the same as On 2015/03/06 16:34:08, carlosk wrote: > On 2015/03/06 15:57:50, clamy wrote: > > Maybe "The SiteInstance did not change, therefore its site URL is still the > same > > as the one used to create it before." But to me, a more interesting test > (since > > the site URL is a bit limitated) would be to compare the original SiteInstance > > ID with the one at the end and they should match, hence making it same-site. > > I updated the comment. > > The information I think is important to convey here is that even though this > last navigation was made to an apparent "http://google.com" site URL, the > SiteInstance is still pointing to "http://chromium.org". Switching to a site_id > comparison would not present that fact as clearly. I would argue that the important information is that the SiteInstance did not change. Yes its site did not change, because once you bind a SiteInstance to a site, the site cannot change anymore (see https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...). However this is more of a SiteInstance implementation detail than an essential feature to test for. Secondly all tests dealing specifically with the swapping or not of SiteInstances use the SiteInstance ID, if only because using Site URLs is not a reliable way to do SiteInstances comparison. https://codereview.chromium.org/946543003/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:728: // the IPC due to the condition below. Once there is support for cross-process No I meant the TODO should start at "Once there is support..." The first part is an explanation of what is currently happening, so not a TODO.
Thanks. https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:237: // As the SiteInstance didn't change, the site URL is still the same as On 2015/03/06 16:58:15, clamy wrote: > On 2015/03/06 16:34:08, carlosk wrote: > > On 2015/03/06 15:57:50, clamy wrote: > > > Maybe "The SiteInstance did not change, therefore its site URL is still the > > same > > > as the one used to create it before." But to me, a more interesting test > > (since > > > the site URL is a bit limitated) would be to compare the original > SiteInstance > > > ID with the one at the end and they should match, hence making it same-site. > > > > I updated the comment. > > > > The information I think is important to convey here is that even though this > > last navigation was made to an apparent "http://google.com" site URL, the > > SiteInstance is still pointing to "http://chromium.org". Switching to a > site_id > > comparison would not present that fact as clearly. > > I would argue that the important information is that the SiteInstance did not > change. Yes its site did not change, because once you bind a SiteInstance to a > site, the site cannot change anymore (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...). > However this is more of a SiteInstance implementation detail than an essential > feature to test for. You and I know how about the immutability of SiteInstance's site URL after it's set. But I'm suggesting this test would help whoever doesn't know that yet (like last-year-me for instance). I agree that the important fact is that the SiteInstance didn't change but just checking for site Ids doesn't make it clear that the site URL didn't change either. IMO this is the more surprising fact for a newcomer who's looking at these tests for the 1st time (likely before looking at SiteInstance::SetSite documentation, which is not directly referenced from here). > Secondly all tests dealing specifically with the swapping or not of > SiteInstances use the SiteInstance ID, if only because using Site URLs is not a > reliable way to do SiteInstances comparison. Indeed they do, but for a different reason: they are testing for RFH instance correctness. I replaced all direct RFH pointer comparisons from these tests with site Id comparisons because they were safer (see http://crbug.com/438651). This is not my objective with this check. So to try and also resolve your concern, I'm adding the site id comparison check along with the site URL check. WDYT? https://codereview.chromium.org/946543003/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:728: // the IPC due to the condition below. Once there is support for cross-process On 2015/03/06 16:58:16, clamy wrote: > No I meant the TODO should start at "Once there is support..." The first part is > an explanation of what is currently happening, so not a TODO. Sorry, me too. I forgot to save the file after updating it... :/
Thanks! https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:237: // As the SiteInstance didn't change, the site URL is still the same as On 2015/03/06 17:44:45, carlosk (OoO until March 16th) wrote: > On 2015/03/06 16:58:15, clamy wrote: > > On 2015/03/06 16:34:08, carlosk wrote: > > > On 2015/03/06 15:57:50, clamy wrote: > > > > Maybe "The SiteInstance did not change, therefore its site URL is still > the > > > same > > > > as the one used to create it before." But to me, a more interesting test > > > (since > > > > the site URL is a bit limitated) would be to compare the original > > SiteInstance > > > > ID with the one at the end and they should match, hence making it > same-site. > > > > > > I updated the comment. > > > > > > The information I think is important to convey here is that even though this > > > last navigation was made to an apparent "http://google.com" site URL, the > > > SiteInstance is still pointing to "http://chromium.org". Switching to a > > site_id > > > comparison would not present that fact as clearly. > > > > I would argue that the important information is that the SiteInstance did not > > change. Yes its site did not change, because once you bind a SiteInstance to a > > site, the site cannot change anymore (see > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...). > > However this is more of a SiteInstance implementation detail than an essential > > feature to test for. > > You and I know how about the immutability of SiteInstance's site URL after it's > set. But I'm suggesting this test would help whoever doesn't know that yet (like > last-year-me for instance). > > I agree that the important fact is that the SiteInstance didn't change but just > checking for site Ids doesn't make it clear that the site URL didn't change > either. IMO this is the more surprising fact for a newcomer who's looking at > these tests for the 1st time (likely before looking at SiteInstance::SetSite > documentation, which is not directly referenced from here). > > > Secondly all tests dealing specifically with the swapping or not of > > SiteInstances use the SiteInstance ID, if only because using Site URLs is not > a > > reliable way to do SiteInstances comparison. > > Indeed they do, but for a different reason: they are testing for RFH instance > correctness. I replaced all direct RFH pointer comparisons from these tests with > site Id comparisons because they were safer (see http://crbug.com/438651). This > is not my objective with this check. > > So to try and also resolve your concern, I'm adding the site id comparison check > along with the site URL check. WDYT? Thanks for adding the site instance ID check. My concern with keeping the url CHECK is that people reading the test may misinterpret it as "This is how we should behave" instead of "This is how we currently behave due to implementation details". Therefore I would like to see it removed.
Thanks. https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:237: // As the SiteInstance didn't change, the site URL is still the same as On 2015/03/16 13:51:18, clamy wrote: > On 2015/03/06 17:44:45, carlosk (OoO until March 16th) wrote: > > On 2015/03/06 16:58:15, clamy wrote: > > > On 2015/03/06 16:34:08, carlosk wrote: > > > > On 2015/03/06 15:57:50, clamy wrote: > > > > > Maybe "The SiteInstance did not change, therefore its site URL is still > > the > > > > same > > > > > as the one used to create it before." But to me, a more interesting test > > > > (since > > > > > the site URL is a bit limitated) would be to compare the original > > > SiteInstance > > > > > ID with the one at the end and they should match, hence making it > > same-site. > > > > > > > > I updated the comment. > > > > > > > > The information I think is important to convey here is that even though > this > > > > last navigation was made to an apparent "http://google.com" site URL, the > > > > SiteInstance is still pointing to "http://chromium.org". Switching to a > > > site_id > > > > comparison would not present that fact as clearly. > > > > > > I would argue that the important information is that the SiteInstance did > not > > > change. Yes its site did not change, because once you bind a SiteInstance to > a > > > site, the site cannot change anymore (see > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...). > > > However this is more of a SiteInstance implementation detail than an > essential > > > feature to test for. > > > > You and I know how about the immutability of SiteInstance's site URL after > it's > > set. But I'm suggesting this test would help whoever doesn't know that yet > (like > > last-year-me for instance). > > > > I agree that the important fact is that the SiteInstance didn't change but > just > > checking for site Ids doesn't make it clear that the site URL didn't change > > either. IMO this is the more surprising fact for a newcomer who's looking at > > these tests for the 1st time (likely before looking at SiteInstance::SetSite > > documentation, which is not directly referenced from here). > > > > > Secondly all tests dealing specifically with the swapping or not of > > > SiteInstances use the SiteInstance ID, if only because using Site URLs is > not > > a > > > reliable way to do SiteInstances comparison. > > > > Indeed they do, but for a different reason: they are testing for RFH instance > > correctness. I replaced all direct RFH pointer comparisons from these tests > with > > site Id comparisons because they were safer (see http://crbug.com/438651). > This > > is not my objective with this check. > > > > So to try and also resolve your concern, I'm adding the site id comparison > check > > along with the site URL check. WDYT? > > Thanks for adding the site instance ID check. My concern with keeping the url > CHECK is that people reading the test may misinterpret it as "This is how we > should behave" instead of "This is how we currently behave due to implementation > details". Therefore I would like to see it removed. Agreed and removed.
Thanks! Lgtm.
carlosk@chromium.org changed reviewers: + nasko@chromium.org
nasko@: PTAL
carlosk@chromium.org changed required reviewers: + nasko@chromium.org
This looks good! Just a few nits and a couple of questions. https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:792: main_test_rfh()->SendRendererInitiatedNavigationRequest(kNewURL, true); This line seems a bit out of logical order with the "PrepareForCommit" step right before it. Also, there is another PrepareForCommit right after this one. Why do we need two? https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (left): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:583: EXPECT_EQ(site_instance_id_2, main_test_rfh()->GetSiteInstance()->GetId()); Why did you remove this SiteInstance check? If we expect to reuse a SiteInstance, why not change the check appropriately instead of removing it? https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:238: // The SiteInstance did not change. nit: Empty line before the comment. https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:761: // The SiteInstance did not change. nit: Empty line before comment. https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:724: // sent to the browser via the OpenURL IPC, and are afterwards treated as nit: No need for comma before "and". https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:725: // browser-initiated navigations. The only NavigationRequests marked as nit: s/The only//
Thanks. https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:792: main_test_rfh()->SendRendererInitiatedNavigationRequest(kNewURL, true); On 2015/03/16 16:22:42, nasko wrote: > This line seems a bit out of logical order with the "PrepareForCommit" step > right before it. Also, there is another PrepareForCommit right after this one. > Why do we need two? It seems to me that the 1st call to PrepareForCommit does get the ongoing navigation to the initial state mentioned in the comment above ("after the beforeunload but before it commits") and the 2nd one executes the new action ("do a new navigation"). I agree this is confusing so I'm adding a line break and splitting that comment. WDYT? https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (left): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:583: EXPECT_EQ(site_instance_id_2, main_test_rfh()->GetSiteInstance()->GetId()); On 2015/03/16 16:22:42, nasko wrote: > Why did you remove this SiteInstance check? If we expect to reuse a > SiteInstance, why not change the check appropriately instead of removing it? This check was added to confirm that the RenderFrameHost that committed was the speculative one instead of using unsafe pointer checks (http://crbug.com/438651), and not to verify the SiteInstance was maintained. I removed it because there's no other RFH involved in the navigation after this change but the current one. Testing that a RFH doesn't have its SiteInstance changed doesn't seem useful in this test. https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:238: // The SiteInstance did not change. On 2015/03/16 16:22:42, nasko wrote: > nit: Empty line before the comment. Done. https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:761: // The SiteInstance did not change. On 2015/03/16 16:22:42, nasko wrote: > nit: Empty line before comment. Done. https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:724: // sent to the browser via the OpenURL IPC, and are afterwards treated as On 2015/03/16 16:22:42, nasko wrote: > nit: No need for comma before "and". Done. https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:725: // browser-initiated navigations. The only NavigationRequests marked as On 2015/03/16 16:22:42, nasko wrote: > nit: s/The only// Done.
LGTM https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/160001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:792: main_test_rfh()->SendRendererInitiatedNavigationRequest(kNewURL, true); On 2015/03/16 17:25:00, carlosk (OoO until March 16th) wrote: > On 2015/03/16 16:22:42, nasko wrote: > > This line seems a bit out of logical order with the "PrepareForCommit" step > > right before it. Also, there is another PrepareForCommit right after this one. > > Why do we need two? > > It seems to me that the 1st call to PrepareForCommit does get the ongoing > navigation to the initial state mentioned in the comment above ("after the > beforeunload but before it commits") and the 2nd one executes the new action > ("do a new navigation"). > > I agree this is confusing so I'm adding a line break and splitting that comment. > WDYT? Ah, makes it a lot more clear. Thanks! https://codereview.chromium.org/946543003/diff/200001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/200001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:790: // After the beforeunload but before it commits... nit: Add a space after "commits".
Thanks. https://codereview.chromium.org/946543003/diff/200001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/946543003/diff/200001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:790: // After the beforeunload but before it commits... On 2015/03/16 17:38:26, nasko wrote: > nit: Add a space after "commits". Leaving as is at it seems more English-like to follow the last word immediately with punctuation.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org Link to the patchset: https://codereview.chromium.org/946543003/#ps200001 (title: "Comment changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/946543003/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/42f59e67e6c4d58a913d48b3a81600cdcd3ced88 Cr-Commit-Position: refs/heads/master@{#320763} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
