Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
914223002:
PlzNavigate: Updated navigation cancel policy for renderer-initiated requests. (Closed)
|
Created:
5 years, 10 months ago by carlosk Modified:
5 years, 10 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: Updated navigation cancel policy for renderer-initiated requests.
Now the navigation cancellation logic should conform to what has been described
in the "PlzNavigate: Navigation cancellation" design document [1]. Added some
new tests to exercise the cancellation cases described in the document.
Note: This is being developed on top of http://crrev.com/912833002.
[1] https://docs.google.com/a/chromium.org/document/d/1lO7_fgppFTDd8PSQfIb888z7vODcw2Sc8r7h5Rbiwcg/edit#
BUG=376014
Committed: https://crrev.com/f470789ee8bfc010c29df458601ca0a33efbb2ca
Cr-Commit-Position: refs/heads/master@{#317083}
Patch Set 1 : #Patch Set 2 : Two comment changes #
Total comments: 16
Patch Set 3 : Some fixes and a new test added. #
Total comments: 4
Patch Set 4 : Uploaded again, now properly based off of crrev.com/912833002 . #Patch Set 5 : More testing. #
Total comments: 6
Patch Set 6 : Comment changes. #
Total comments: 20
Patch Set 7 : Changes from CR comments. #
Total comments: 2
Patch Set 8 : Reverted method name; comment updates. #
Messages
Total messages: 34 (14 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
carlosk@chromium.org changed reviewers: + clamy@chromium.org, fdegans@chromium.org
carlosk@chromium.org changed required reviewers: + clamy@chromium.org
clamy@: PTAL. fdegans@: FYI.
Thanks! It should be quite good once the issue with browser-initiated navigations is fixed. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:713: ongoing_navigation_request->begin_params().has_user_gesture && I don't think this currently works. In browser initiated navigations we explicitly set has_user_gesture to false (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...). The condition there would be (ongoing_navigation_request->browser_initiated() || ongoing_navigation_request->begin_params().has_user_gesture). https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:715: // The renderer navigation request is canceled iff a) there is an ongoing This does not cancel the renderer-initiated navigation, so I think that a better phrasing would be: The ongoing navigation is not canceled iff... https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:720: // In all other cases the current navigation, if any, is canceled and a new nit: add an empty line before the comment. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:721: // NavigationRequest is created and store it in the map. s/store it/stored https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:532: // beoforeUnload ACK. nit: s/beoforeUnload/beforeUnload https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:554: EXPECT_TRUE(request2->begin_params().has_user_gesture); Maybe add a check that loader1 was properly destroyed? I think this is why you declared it as a WeakPtr. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:589: EXPECT_TRUE(request->begin_params().has_user_gesture); Could you also expend this test to check that a browser-initiated request does not get canceled by a non user-initiated renderer-initiated navigation? Spoiler: this is not currently the case as far as I can read the code.
Thanks! Good catch on the request-ignoring logic clamy@! https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:713: ongoing_navigation_request->begin_params().has_user_gesture && On 2015/02/11 17:22:29, clamy wrote: > I don't think this currently works. In browser initiated navigations we > explicitly set has_user_gesture to false > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...). > > The condition there would be (ongoing_navigation_request->browser_initiated() || > ongoing_navigation_request->begin_params().has_user_gesture). And I even knew about it... So I need one more test! Test added and bug fixed. It was also good to go over them once more because I found a few issues with the existing tests. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:715: // The renderer navigation request is canceled iff a) there is an ongoing On 2015/02/11 17:22:30, clamy wrote: > This does not cancel the renderer-initiated navigation, so I think that a better > phrasing would be: The ongoing navigation is not canceled iff... I did this instead: s/canceled/ignored https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:720: // In all other cases the current navigation, if any, is canceled and a new On 2015/02/11 17:22:29, clamy wrote: > nit: add an empty line before the comment. Done. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:721: // NavigationRequest is created and store it in the map. On 2015/02/11 17:22:30, clamy wrote: > s/store it/stored Done. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:532: // beoforeUnload ACK. On 2015/02/11 17:22:30, clamy wrote: > nit: s/beoforeUnload/beforeUnload Done. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:554: EXPECT_TRUE(request2->begin_params().has_user_gesture); On 2015/02/11 17:22:30, clamy wrote: > Maybe add a check that loader1 was properly destroyed? I think this is why you > declared it as a WeakPtr. Yes and done. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:589: EXPECT_TRUE(request->begin_params().has_user_gesture); On 2015/02/11 17:22:30, clamy wrote: > Could you also expend this test to check that a browser-initiated request does > not get canceled by a non user-initiated renderer-initiated navigation? Spoiler: > this is not currently the case as far as I can read the code. Done. As a new test.
Thanks! A few more nits and it should be good to go. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:715: // The renderer navigation request is canceled iff a) there is an ongoing On 2015/02/12 12:21:05, carlosk wrote: > On 2015/02/11 17:22:30, clamy wrote: > > This does not cancel the renderer-initiated navigation, so I think that a > better > > phrasing would be: The ongoing navigation is not canceled iff... > > I did this instead: s/canceled/ignored The change did not appear in the diff :) https://codereview.chromium.org/914223002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:723: // NavigationRequest is created and storeed in the map. nit: s/storeed/stored https://codereview.chromium.org/914223002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:527: RendererUserInitiatedNavigationCancel) { I am thinking that we may want to have these tests commit the final navigation (whether its the new one or the old one) to make sure everything is in order there.
Thanks for another round or reviews. https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:715: // The renderer navigation request is canceled iff a) there is an ongoing On 2015/02/12 18:08:06, clamy wrote: > On 2015/02/12 12:21:05, carlosk wrote: > > On 2015/02/11 17:22:30, clamy wrote: > > > This does not cancel the renderer-initiated navigation, so I think that a > > better > > > phrasing would be: The ongoing navigation is not canceled iff... > > > > I did this instead: s/canceled/ignored > > The change did not appear in the diff :) Done now. Sorry about that. https://codereview.chromium.org/914223002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:723: // NavigationRequest is created and storeed in the map. On 2015/02/12 18:08:06, clamy wrote: > nit: s/storeed/stored Done. https://codereview.chromium.org/914223002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:527: RendererUserInitiatedNavigationCancel) { On 2015/02/12 18:08:07, clamy wrote: > I am thinking that we may want to have these tests commit the final navigation > (whether its the new one or the old one) to make sure everything is in order > there. Done. This made me find a few more test improvements points.
Thnaks! Lgtm with a few nits. https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:718: // is not user-initiated. nit: I think it would be more readable if this comment was above the if condition since it explains it. https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:575: // Request the RenderFrameHost to commit. nit: I would rephrase that as "Have the RenderFrameHost commit the navigation" (as in the other tests that just landed). Same below. https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:589: // Confirm that the committed RFH is the latest speculative one. nit: I would rephrase as "Confirm that the RFH that navigated is..."
New patchsets have been uploaded after l-g-t-m from clamy@chromium.org
carlosk@chromium.org changed reviewers: + nasko@chromium.org
carlosk@chromium.org changed required reviewers: + nasko@chromium.org
Thanks. nasko@: PTAL. https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:718: // is not user-initiated. On 2015/02/13 14:56:35, clamy wrote: > nit: I think it would be more readable if this comment was above the if > condition since it explains it. Done. https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:575: // Request the RenderFrameHost to commit. On 2015/02/13 14:56:35, clamy wrote: > nit: I would rephrase that as "Have the RenderFrameHost commit the navigation" > (as in the other tests that just landed). Same below. Done, here and everywhere else. https://codereview.chromium.org/914223002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:589: // Confirm that the committed RFH is the latest speculative one. On 2015/02/13 14:56:35, clamy wrote: > nit: I would rephrase as "Confirm that the RFH that navigated is..." Done, here and everywhere else.
carlosk@chromium.org changed reviewers: + creis@chromium.org
carlosk@chromium.org changed required reviewers: + creis@chromium.org
creis@: PTAL (adding you as it seems nasko@ will be OoO for a while).
Thanks-- this should bring some sanity to the navigation canceling logic. A few comments below. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:706: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( Let's put a comment at the top of this function saying "This is a renderer-initiated navigation." That part is implicit in the comment below, but it's worthwhile to call it out. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:713: // The renderer navigation request is ignored iff a) there is an ongoing nit: Please keep the term "renderer-initiated navigation" to be consistent with other code. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:724: // NavigationRequest is created and stored in the map. Can you mention where it gets canceled? (I don't see cancelation mentioned in the comment of CreateRendererInitiated. Is there code somewhere that tells the network stack, etc, that the old navigation should be stopped?) https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:527: // PlzNavigate: Test that a navigation is canceled if another renderer-initiated This test is about a browser-initiated navigation being canceled by a renderer-initiated user-initiated navigation, right? The comment doesn't make that clear (i.e., it sounds like it's describing the RendererNonUserInitiatedNavigationCancelSimilarNavigation test). https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:543: main_test_rfh()->SendBeforeUnloadACK(true); Note: You may conflict with clamy's https://codereview.chromium.org/903043002/. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:710: // Now receive a 2nd similar request that should replace the current one. Is there a way to check the loader, as in the first test?
https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:724: // NavigationRequest is created and stored in the map. On 2015/02/17 23:48:29, Charlie Reis wrote: > Can you mention where it gets canceled? > > (I don't see cancelation mentioned in the comment of CreateRendererInitiated. > Is there code somewhere that tells the network stack, etc, that the old > navigation should be stopped?) This is done upon destruction of the old NavigationRequest. It owns a NavigationURLLoader, and the destruction of the NavigationURLLoader triggers a call to the cancelation logic of the network stack.
Thanks. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:706: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/02/17 23:48:29, Charlie Reis wrote: > Let's put a comment at the top of this function saying "This is a > renderer-initiated navigation." That part is implicit in the comment below, but > it's worthwhile to call it out. I thought it was better to actually rename the method. Should I also rename the IPC and its handler method in RFHI? https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:713: // The renderer navigation request is ignored iff a) there is an ongoing On 2015/02/17 23:48:29, Charlie Reis wrote: > nit: Please keep the term "renderer-initiated navigation" to be consistent with > other code. Done. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:724: // NavigationRequest is created and stored in the map. On 2015/02/18 12:45:33, clamy wrote: > On 2015/02/17 23:48:29, Charlie Reis wrote: > > Can you mention where it gets canceled? > > > > (I don't see cancelation mentioned in the comment of CreateRendererInitiated. > > Is there code somewhere that tells the network stack, etc, that the old > > navigation should be stopped?) > > This is done upon destruction of the old NavigationRequest. It owns a > NavigationURLLoader, and the destruction of the NavigationURLLoader triggers a > call to the cancelation logic of the network stack. I changed the comment to clarify that. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:527: // PlzNavigate: Test that a navigation is canceled if another renderer-initiated On 2015/02/17 23:48:29, Charlie Reis wrote: > This test is about a browser-initiated navigation being canceled by a > renderer-initiated user-initiated navigation, right? The comment doesn't make > that clear (i.e., it sounds like it's describing the > RendererNonUserInitiatedNavigationCancelSimilarNavigation test). Yes, that's correct. I updated this comment to be specific to the test case. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:543: main_test_rfh()->SendBeforeUnloadACK(true); On 2015/02/17 23:48:29, Charlie Reis wrote: > Note: You may conflict with clamy's https://codereview.chromium.org/903043002/. Thanks. Yes, I was aware. As she is almost landing that one, I'll have to make the due adjustments here. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:710: // Now receive a 2nd similar request that should replace the current one. On 2015/02/17 23:48:30, Charlie Reis wrote: > Is there a way to check the loader, as in the first test? Done.
Thanks. LGTM if you change RendererInitiatedNavigationRequest back to OnBeginNavigation. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:706: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/02/18 15:28:25, carlosk wrote: > On 2015/02/17 23:48:29, Charlie Reis wrote: > > Let's put a comment at the top of this function saying "This is a > > renderer-initiated navigation." That part is implicit in the comment below, > but > > it's worthwhile to call it out. > > I thought it was better to actually rename the method. Should I also rename the > IPC and its handler method in RFHI? No, I actually think the old OnBeginNavigation name was better, because it matches our OnFoo naming convention for IPC message handlers. Please change it back. A comment should be sufficient to call attention to that fact. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:724: // NavigationRequest is created and stored in the map. On 2015/02/18 15:28:25, carlosk wrote: > On 2015/02/18 12:45:33, clamy wrote: > > On 2015/02/17 23:48:29, Charlie Reis wrote: > > > Can you mention where it gets canceled? > > > > > > (I don't see cancelation mentioned in the comment of > CreateRendererInitiated. > > > Is there code somewhere that tells the network stack, etc, that the old > > > navigation should be stopped?) > > > > This is done upon destruction of the old NavigationRequest. It owns a > > NavigationURLLoader, and the destruction of the NavigationURLLoader triggers a > > call to the cancelation logic of the network stack. > > I changed the comment to clarify that. Acknowledged. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:527: // PlzNavigate: Test that a navigation is canceled if another renderer-initiated On 2015/02/18 15:28:25, carlosk wrote: > On 2015/02/17 23:48:29, Charlie Reis wrote: > > This test is about a browser-initiated navigation being canceled by a > > renderer-initiated user-initiated navigation, right? The comment doesn't make > > that clear (i.e., it sounds like it's describing the > > RendererNonUserInitiatedNavigationCancelSimilarNavigation test). > > Yes, that's correct. I updated this comment to be specific to the test case. Acknowledged. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:710: // Now receive a 2nd similar request that should replace the current one. On 2015/02/18 15:28:26, carlosk wrote: > On 2015/02/17 23:48:30, Charlie Reis wrote: > > Is there a way to check the loader, as in the first test? > > Done. Acknowledged. https://codereview.chromium.org/914223002/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:527: // PlzNavigate: Test that a browser initiated navigation is canceled if a nit: hypenate
New patchsets have been uploaded after l-g-t-m from creis@chromium.org
Thanks again. Just want to better understand the naming thing. https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:706: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/02/19 00:58:00, Charlie Reis wrote: > On 2015/02/18 15:28:25, carlosk wrote: > > On 2015/02/17 23:48:29, Charlie Reis wrote: > > > Let's put a comment at the top of this function saying "This is a > > > renderer-initiated navigation." That part is implicit in the comment below, > > but > > > it's worthwhile to call it out. > > > > I thought it was better to actually rename the method. Should I also rename > the > > IPC and its handler method in RFHI? > > No, I actually think the old OnBeginNavigation name was better, because it > matches our OnFoo naming convention for IPC message handlers. Please change it > back. > > A comment should be sufficient to call attention to that fact. I reverted the rename and added the comment. But please help me understand your reasoning. Why the old name when this name is clearer in stating what this method is actually meant for (now, after the refactor)? I have nothing against comments but having an better suited method name is an order of magnitude better. We could prefix this (or a similar) name with "On". As I stated I was also willing to change the IPC/handler method names to make them match. https://codereview.chromium.org/914223002/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/914223002/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:527: // PlzNavigate: Test that a browser initiated navigation is canceled if a On 2015/02/19 00:58:00, Charlie Reis wrote: > nit: hypenate Done.
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914223002/180001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
carlosk@chromium.org changed required reviewers: - nasko@chromium.org
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914223002/180001
https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:706: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/02/19 10:45:18, carlosk wrote: > On 2015/02/19 00:58:00, Charlie Reis wrote: > > On 2015/02/18 15:28:25, carlosk wrote: > > > On 2015/02/17 23:48:29, Charlie Reis wrote: > > > > Let's put a comment at the top of this function saying "This is a > > > > renderer-initiated navigation." That part is implicit in the comment > below, > > > but > > > > it's worthwhile to call it out. > > > > > > I thought it was better to actually rename the method. Should I also rename > > the > > > IPC and its handler method in RFHI? > > > > No, I actually think the old OnBeginNavigation name was better, because it > > matches our OnFoo naming convention for IPC message handlers. Please change > it > > back. > > > > A comment should be sufficient to call attention to that fact. > > I reverted the rename and added the comment. > > But please help me understand your reasoning. Why the old name when this name is > clearer in stating what this method is actually meant for (now, after the > refactor)? I have nothing against comments but having an better suited method > name is an order of magnitude better. > > We could prefix this (or a similar) name with "On". As I stated I was also > willing to change the IPC/handler method names to make them match. Sure, I'm happy to explain my reasoning: Names are indeed important, and they should be clear, concise, and consistent with other names when possible. I would argue that OnBeginNavigation accurately describes a request from the renderer process to begin a navigation. I would say that the name was misleading in the past, because it was also used for browser-initiated navigations. That's no longer the case, so I don't think we need the more verbose "RendererInitiatedNavigationRequest" (which is not consise nor consistent with our other method names). Instead, the comment seems sufficient to me for verifying that the name means what it sounds like. Hope that helps.
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f470789ee8bfc010c29df458601ca0a33efbb2ca Cr-Commit-Position: refs/heads/master@{#317083}
Message was sent while issue was closed.
Thanks! https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/914223002/diff/140001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:706: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/02/19 18:42:12, Charlie Reis wrote: > On 2015/02/19 10:45:18, carlosk wrote: > > On 2015/02/19 00:58:00, Charlie Reis wrote: > > > On 2015/02/18 15:28:25, carlosk wrote: > > > > On 2015/02/17 23:48:29, Charlie Reis wrote: > > > > > Let's put a comment at the top of this function saying "This is a > > > > > renderer-initiated navigation." That part is implicit in the comment > > below, > > > > but > > > > > it's worthwhile to call it out. > > > > > > > > I thought it was better to actually rename the method. Should I also > rename > > > the > > > > IPC and its handler method in RFHI? > > > > > > No, I actually think the old OnBeginNavigation name was better, because it > > > matches our OnFoo naming convention for IPC message handlers. Please change > > it > > > back. > > > > > > A comment should be sufficient to call attention to that fact. > > > > I reverted the rename and added the comment. > > > > But please help me understand your reasoning. Why the old name when this name > is > > clearer in stating what this method is actually meant for (now, after the > > refactor)? I have nothing against comments but having an better suited method > > name is an order of magnitude better. > > > > We could prefix this (or a similar) name with "On". As I stated I was also > > willing to change the IPC/handler method names to make them match. > > Sure, I'm happy to explain my reasoning: > > Names are indeed important, and they should be clear, concise, and consistent > with other names when possible. I would argue that OnBeginNavigation accurately > describes a request from the renderer process to begin a navigation. > > I would say that the name was misleading in the past, because it was also used > for browser-initiated navigations. That's no longer the case, so I don't think > we need the more verbose "RendererInitiatedNavigationRequest" (which is not > consise nor consistent with our other method names). Instead, the comment seems > sufficient to me for verifying that the name means what it sounds like. > > Hope that helps. Acknowledged. Thanks. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
