Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
475783002:
PlzNavigate: add cancel navigation logic for uncommitted requests (Closed)
DescriptionPlzNavigate: add cancel navigation logic for uncommitted requests
Allows navigation requests to be canceled before they are committed when they become stale.
BUG=397998
Committed: https://crrev.com/57c81e9cbddce2d2938ac5a80164aecb6c1b51f2
Cr-Commit-Position: refs/heads/master@{#293484}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed CR comments #Patch Set 3 : Added tests for the navigation request ID #
Total comments: 1
Patch Set 4 : Merge with upstream changes; todo comment regarding navigation request IDs #Patch Set 5 : Ignores commits of stale navigation requests #
Total comments: 10
Patch Set 6 : Retrying for the weird link errors; fixed nits. #Patch Set 7 : Fixed the linking issue. #
Total comments: 25
Patch Set 8 : Addressed CR comments #
Total comments: 2
Patch Set 9 : Addressed Nasko's comments #
Total comments: 4
Patch Set 10 : Added the actual calls to cancel the navigation request #
Total comments: 27
Patch Set 11 : Addressed CR comments and removed the weird Observer. Trying to get this CL through and have the mo… #
Total comments: 10
Patch Set 12 : Addressed CR comments #Patch Set 13 : Test assertion improvemennts #Patch Set 14 : Resolving merge conflict #Patch Set 15 : Fixed failed test #Messages
Total messages: 49 (7 generated)
Pleas take a look
Thnaks! A few comments below. https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... content/browser/frame_host/navigation_request.cc:33: int64 NavigationRequest::next_navigation_request_id_ = 1; This should be changed to 0 as we use pre-increment (see comment below). https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... content/browser/frame_host/navigation_request.cc:37: : navigation_request_id_(next_navigation_request_id_++), The styleguide mandates using ++next_navigation_request_id_ (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...) https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... content/browser/frame_host/navigation_request.h:35: return navigation_request_id_; Please write this on one line since it fits: int64 navigation_request_id() const { return navigation_request_id_; } https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... content/browser/frame_host/navigation_request.h:46: nit: unnecessary blank line. https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/loade... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/loade... content/browser/loader/resource_dispatcher_host_impl.h:243: void NavigationRequest(const NavigationRequestInfo& info, While you are at it, could you change the name to StartNavigationRequest? I think it would match CancelNavigationRequest better. Also add the navigation_request_id as a parameter, since it will be used there as well.
Thanks. And here are the respective changes. https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... content/browser/frame_host/navigation_request.cc:33: int64 NavigationRequest::next_navigation_request_id_ = 1; On 2014/08/14 13:19:06, clamy wrote: > This should be changed to 0 as we use pre-increment (see comment below). Done. https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... content/browser/frame_host/navigation_request.cc:37: : navigation_request_id_(next_navigation_request_id_++), On 2014/08/14 13:19:06, clamy wrote: > The styleguide mandates using ++next_navigation_request_id_ (see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...) Done. https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... content/browser/frame_host/navigation_request.h:35: return navigation_request_id_; On 2014/08/14 13:19:06, clamy wrote: > Please write this on one line since it fits: > int64 navigation_request_id() const { return navigation_request_id_; } Done. https://chromiumcodereview.appspot.com/475783002/diff/1/content/browser/frame... content/browser/frame_host/navigation_request.h:46: On 2014/08/14 13:19:06, clamy wrote: > nit: unnecessary blank line. Done.
Thanks! One more styleguide issue. https://chromiumcodereview.appspot.com/475783002/diff/40001/content/browser/f... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/40001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1858: const int64 firstNavRequestID = 1; Please rename this variable as kFirstNavRequestId (styleguide mandates that constant names should be prefixed with a k: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names).
PTAL?
PTAL once more?
Thanks! lgtm if you remove the code that is commented in the new test. https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1679: // after the navigation is committed successfully nit: add a '.' at the end of the line. https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1704: // Confirms that a stale commit is disconsidered by the RHFM. nit: s/disconsidered/ignored https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1710: //EXPECT_EQ(kUrl0, main_test_rfh()->GetSiteInstance()->GetSiteURL()); Please remove the commented code. https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1716: //EXPECT_EQ(kUrl2, main_test_rfh()->GetSiteInstance()->GetSiteURL()); Please remove the commented code. https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/l... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/l... content/browser/loader/resource_dispatcher_host_impl.h:241: // Called by NavigationRequest to start a navigation request in the node nit: could you add a //PlzNavigate line before that comment?
Dear new reviewers: could you PTAL at my 1st CL to Chromium? :) https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1679: // after the navigation is committed successfully On 2014/08/21 11:48:52, clamy wrote: > nit: add a '.' at the end of the line. Done. https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1704: // Confirms that a stale commit is disconsidered by the RHFM. On 2014/08/21 11:48:52, clamy wrote: > nit: s/disconsidered/ignored Done. https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1710: //EXPECT_EQ(kUrl0, main_test_rfh()->GetSiteInstance()->GetSiteURL()); On 2014/08/21 11:48:52, clamy wrote: > Please remove the commented code. Done. https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/f... content/browser/frame_host/render_frame_host_manager_unittest.cc:1716: //EXPECT_EQ(kUrl2, main_test_rfh()->GetSiteInstance()->GetSiteURL()); On 2014/08/21 11:48:52, clamy wrote: > Please remove the commented code. Done. https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/l... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://chromiumcodereview.appspot.com/475783002/diff/80001/content/browser/l... content/browser/loader/resource_dispatcher_host_impl.h:241: // Called by NavigationRequest to start a navigation request in the node On 2014/08/21 11:48:52, clamy wrote: > nit: could you add a //PlzNavigate line before that comment? Done.
Great job on your first CL! I've added a few comments and nits. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:11: #include "base/time/time.h" nit: Is this include really needed? https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_request.cc:25: void OnDestructNavigationRequest(int64 navigation_request_id, nit: I'd call this OnCancel* instead of OnDestruct*, as you aren't destroying a NavigationRequest here. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_request.h:22: // the navigation is started (if that's finally required) I don't see a clear reason to assign id at time of start instead of time of creating the object. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:574: navigation_request_.reset(new NavigationRequest( nit: I'd add a comment here to say that deleting the existing navigation request will cancel it. This way is non-obvious what is happening. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1685: // Initialization. nit: Some spacing between comments and code makes it easier to read. I'd add a line before the comment and in a few places in the below comments. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.h:247: int64 frame_node_id); nit: frame_tree_node_id. I missed to add this comment in previous reviews, so NavigationRequest uses it, but we should probably rename it next time it is touched. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.h:251: // provided |navigation_request_id| from the node identified by nit: s/from/in/
Thanks Nasko. Could you PTAL again? https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:11: #include "base/time/time.h" On 2014/08/21 21:53:43, nasko wrote: > nit: Is this include really needed? It is for the: base::TimeTicks browser_navigation_start; member. This include not being there was in fact an error that wasn't showing up because this header was not being used anywhere. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_request.cc:25: void OnDestructNavigationRequest(int64 navigation_request_id, On 2014/08/21 21:53:43, nasko wrote: > nit: I'd call this OnCancel* instead of OnDestruct*, as you aren't destroying a > NavigationRequest here. I called it that way because this method is being invoked form inside NavigationRequest's destructor. Using Cancel ins this case seems incorrect because the navigation request might have been committed. WDYT? https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_request.h:22: // the navigation is started (if that's finally required) On 2014/08/21 21:53:43, nasko wrote: > I don't see a clear reason to assign id at time of start instead of time of > creating the object. This is related to some conversations me and clamy were having regarding the various ways a navigation might be started (and interrupted and ignored in some cases) in the future and even though I don't like this either it seems like it'll be required. If you prefer I can remove this comment as it doesn't apply to the current state of the PlzNavigate implementation. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:574: navigation_request_.reset(new NavigationRequest( On 2014/08/21 21:53:44, nasko wrote: > nit: I'd add a comment here to say that deleting the existing navigation request > will cancel it. This way is non-obvious what is happening. Comment added. But note that right now we're "cancelling" everything when in fact not all NR destructor calls will indeed cause a cancellation (the request might even have been committed). See comment at line 44 @ content/browser/frame_host/navigation_request.cc . https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1685: // Initialization. On 2014/08/21 21:53:44, nasko wrote: > nit: Some spacing between comments and code makes it easier to read. I'd add a > line before the comment and in a few places in the below comments. Done! I felt that as well but was uncertain about the coding style guidelines in this case. :) https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.h:247: int64 frame_node_id); On 2014/08/21 21:53:44, nasko wrote: > nit: frame_tree_node_id. I missed to add this comment in previous reviews, so > NavigationRequest uses it, but we should probably rename it next time it is > touched. I'm not sure I understood you... You meant I shouldn't rename this now and let's wait for the next time we touch this, right? https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.h:251: // provided |navigation_request_id| from the node identified by On 2014/08/21 21:53:44, nasko wrote: > nit: s/from/in/ Done.
LGTM with a few comments. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_before_commit_info.h:11: #include "base/time/time.h" On 2014/08/22 13:43:02, carlosk wrote: > On 2014/08/21 21:53:43, nasko wrote: > > nit: Is this include really needed? > > It is for the: > > base::TimeTicks browser_navigation_start; > > member. This include not being there was in fact an error that wasn't showing up > because this header was not being used anywhere. Acknowledged. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_request.cc:25: void OnDestructNavigationRequest(int64 navigation_request_id, On 2014/08/22 13:43:03, carlosk wrote: > On 2014/08/21 21:53:43, nasko wrote: > > nit: I'd call this OnCancel* instead of OnDestruct*, as you aren't destroying > a > > NavigationRequest here. > > I called it that way because this method is being invoked form inside > NavigationRequest's destructor. Using Cancel ins this case seems incorrect > because the navigation request might have been committed. WDYT? If canceling is incorrect, then there should be a TODO indicating this and that it should be fixed in the future. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_request.h:22: // the navigation is started (if that's finally required) On 2014/08/22 13:43:03, carlosk wrote: > On 2014/08/21 21:53:43, nasko wrote: > > I don't see a clear reason to assign id at time of start instead of time of > > creating the object. > > This is related to some conversations me and clamy were having regarding the > various ways a navigation might be started (and interrupted and ignored in some > cases) in the future and even though I don't like this either it seems like > it'll be required. > > If you prefer I can remove this comment as it doesn't apply to the current state > of the PlzNavigate implementation. Either leave it in place with explanation why we have to do it in the future or just remove it for now. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:574: navigation_request_.reset(new NavigationRequest( On 2014/08/22 13:43:03, carlosk wrote: > On 2014/08/21 21:53:44, nasko wrote: > > nit: I'd add a comment here to say that deleting the existing navigation > request > > will cancel it. This way is non-obvious what is happening. > > Comment added. > > But note that right now we're "cancelling" everything when in fact not all NR > destructor calls will indeed cause a cancellation (the request might even have > been committed). See comment at line 44 @ > content/browser/frame_host/navigation_request.cc . If the navigation was committed, I would expect the navigation_request_ member to be cleared, since it should be used only between starting a request and committing it. So if one does exist, it must be cancelled (through the act of destructing it). https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1685: // Initialization. On 2014/08/22 13:43:03, carlosk wrote: > On 2014/08/21 21:53:44, nasko wrote: > > nit: Some spacing between comments and code makes it easier to read. I'd add a > > line before the comment and in a few places in the below comments. > > Done! > > I felt that as well but was uncertain about the coding style guidelines in this > case. :) Guiding rule is that the code must be readable. If you feel it is hard to read/comprehend, then change it. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.h:247: int64 frame_node_id); On 2014/08/22 13:43:03, carlosk wrote: > On 2014/08/21 21:53:44, nasko wrote: > > nit: frame_tree_node_id. I missed to add this comment in previous reviews, so > > NavigationRequest uses it, but we should probably rename it next time it is > > touched. > > I'm not sure I understood you... You meant I shouldn't rename this now and let's > wait for the next time we touch this, right? Yes. https://chromiumcodereview.appspot.com/475783002/diff/140001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:576: // point, what might cause the cancellation of that request. nit: s/what/which/
Thanks Nasco. I'll talk with clamy next week about the "when to destroy NavigationRequest" instances and will keep you posted. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_request.cc:25: void OnDestructNavigationRequest(int64 navigation_request_id, On 2014/08/22 14:00:34, nasko wrote: > On 2014/08/22 13:43:03, carlosk wrote: > > On 2014/08/21 21:53:43, nasko wrote: > > > nit: I'd call this OnCancel* instead of OnDestruct*, as you aren't > destroying > > a > > > NavigationRequest here. > > > > I called it that way because this method is being invoked form inside > > NavigationRequest's destructor. Using Cancel ins this case seems incorrect > > because the navigation request might have been committed. WDYT? > > If canceling is incorrect, then there should be a TODO indicating this and that > it should be fixed in the future. I thought a bit more and I renamed it to Cancel* as you suggested initially. I had pre-assumed we might do more than canceling in it but right now we simply don't know about that and it looks better this way (smells like too much designing ahead). And the TODO has always been there! :) https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/navigation_request.h:22: // the navigation is started (if that's finally required) On 2014/08/22 14:00:34, nasko wrote: > On 2014/08/22 13:43:03, carlosk wrote: > > On 2014/08/21 21:53:43, nasko wrote: > > > I don't see a clear reason to assign id at time of start instead of time of > > > creating the object. > > > > This is related to some conversations me and clamy were having regarding the > > various ways a navigation might be started (and interrupted and ignored in > some > > cases) in the future and even though I don't like this either it seems like > > it'll be required. > > > > If you prefer I can remove this comment as it doesn't apply to the current > state > > of the PlzNavigate implementation. > > Either leave it in place with explanation why we have to do it in the future or > just remove it for now. Removing it for now then. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:574: navigation_request_.reset(new NavigationRequest( On 2014/08/22 14:00:34, nasko wrote: > On 2014/08/22 13:43:03, carlosk wrote: > > On 2014/08/21 21:53:44, nasko wrote: > > > nit: I'd add a comment here to say that deleting the existing navigation > > request > > > will cancel it. This way is non-obvious what is happening. > > > > Comment added. > > > > But note that right now we're "cancelling" everything when in fact not all NR > > destructor calls will indeed cause a cancellation (the request might even have > > been committed). See comment at line 44 @ > > content/browser/frame_host/navigation_request.cc . > > If the navigation was committed, I would expect the navigation_request_ member > to be cleared, since it should be used only between starting a request and > committing it. So if one does exist, it must be cancelled (through the act of > destructing it). I did talk with clamy about this specific thing and IIRC she had some reasons for the request not to be cleared out like that. She's OoO today so I suggest we postpone this discussion for next week. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1685: // Initialization. On 2014/08/22 14:00:34, nasko wrote: > On 2014/08/22 13:43:03, carlosk wrote: > > On 2014/08/21 21:53:44, nasko wrote: > > > nit: Some spacing between comments and code makes it easier to read. I'd add > a > > > line before the comment and in a few places in the below comments. > > > > Done! > > > > I felt that as well but was uncertain about the coding style guidelines in > this > > case. :) > > Guiding rule is that the code must be readable. If you feel it is hard to > read/comprehend, then change it. Acknowledged. https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://chromiumcodereview.appspot.com/475783002/diff/120001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.h:247: int64 frame_node_id); On 2014/08/22 14:00:34, nasko wrote: > On 2014/08/22 13:43:03, carlosk wrote: > > On 2014/08/21 21:53:44, nasko wrote: > > > nit: frame_tree_node_id. I missed to add this comment in previous reviews, > so > > > NavigationRequest uses it, but we should probably rename it next time it is > > > touched. > > > > I'm not sure I understood you... You meant I shouldn't rename this now and > let's > > wait for the next time we touch this, right? > > Yes. Acknowledged. https://chromiumcodereview.appspot.com/475783002/diff/140001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:576: // point, what might cause the cancellation of that request. On 2014/08/22 14:00:34, nasko wrote: > nit: s/what/which/ Done.
Thanks Nasko. I'll talk with clamy next week about the "when to destroy NavigationRequest" instances and will keep you posted.
lgtm with minor nit. Sorry for the delay. https://codereview.chromium.org/475783002/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/475783002/diff/160001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:39: static int64 next_navigation_request_id_; Nit: This may as well be a global in navigation_request.cc and not in the header file.
Patchset #10 (id:180001) has been deleted
Patchset #10 (id:200001) has been deleted
PT-one-more-L. These last changes took a little longer. They include: * Nasko and Camille's conclusion on how to work out the life-cycle of NavigationRequests * Proper calls to CancelNavigation when a) a request is made stale (by another request) and b) when the RFHM is destroyed * Updated tests covering these changes. Note that I didn't know how to test the cancellation call for case b) and I'm not really sure it's that important. Finally I feel that what was supposed to be just a tiny step for PlzNavigate has now become an actual step. :) https://chromiumcodereview.appspot.com/475783002/diff/160001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/160001/content/browser/... content/browser/frame_host/navigation_request.h:39: static int64 next_navigation_request_id_; On 2014/08/25 17:31:34, David Benjamin wrote: > Nit: This may as well be a global in navigation_request.cc and not in the header > file. Done.
Thanks! I think it is getting close to what we want in the end, however some work needs to be done on the NavigationRequestObserver. People tend to have certain assumptions on how a class called FooObserver behaves, and your implementation breaks those. To get an idea of what is expected, you can have a look at WebContentsObserver and WebContentsImpl. https://chromiumcodereview.appspot.com/475783002/diff/160001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/160001/content/browser/... content/browser/frame_host/navigation_request.h:27: const NavigationRequestInfo& info() const { return info_; } Why did you move those methods? https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.cc:46: navigation_request_->SetSingleObserver(NULL); This should be replaced by a call to RemoveObserver. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.cc:60: single_observer_->set_navigation_request(NULL); nit: it may actually be unnecessary to set the the navigation request to NULL here. However, in that case, the implementation of RequestDestroyed should set it to null. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.h:28: class Observer { The CONTENT_EXPORT should be placed before the class declaration rather than in front of the methods. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.h:30: CONTENT_EXPORT Observer(); The constructors and the destructor should be protected. Also the empty constructor is not used anywhere, so I think it should be removed. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.h:42: void set_navigation_request(NavigationRequest* nv) { With further reading of the code, it is apparently possible to make NavigationRequest a friend of this class. In that case this setter should be removed. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.h:76: Observer* single_observer_; I really don't like this single observer pattern. The other observer classes in the code follow a different pattern in which you can have any number of observers, stored in an ObserverList (see base/observer_list.h), with two functions AddObserver and RemoveObserver. I think the NavigationRequestObserver should follow this pattern as well, because other people will not expect that adding an observer to the class will remove the previous one. The ObserverList also comes with a FOR_EACH_OBSERVER macro that should be used to call the appropriate functions on each observer. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:602: if (navigation_request_.get()) Please add a comment explaining that if we have a non-commited navigation request in flight, it should be canceled. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1812: int cancelCounter; I would rather have a boolean than a counter here. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1813: nit: unnecessary line. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1815: nit: unnecessary line. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1869: // Confirms both 1st and 2nd observers are at the expected states I would replace this comment with something a bit more explicit: The first request should have been canceled and destroyed. The second request should have not been canceled neither destroyed. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1880: // Confirms both 1st and 2nd observers are at the expected states I would replace the comment with something like: The second request should have been destroyed but not canceled.
nasko@google.com changed reviewers: + nasko@google.com
Sorry to drag this so long Carlos. I am somewhat confused about the observer you added, how it fits, and what is the goal for it beyond testing. Can you elaborate? https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.cc:17: int64 next_navigation_request_id_ = 0; nit: make this one static https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.h:28: class Observer { This class doesn't seem to follow very well how other observers works. I don't know of any case we have where we hardcode only one observer. Also, if it is only used for testing, it should be clearly labeled as such. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:445: navigation_request_.reset(); This seems a bit fragile to me. Once we tell the renderer that the navigation has committed, we don't need to hold the nav request in RFHM, do we? Since this is in response to renderer telling us it committed something, there can be race conditions between quick navigations.
I created the Observer only for testing purposes (at least for now there's no other use for it) because I couldn't find a way to actually detect if the proper navigation cancellation calls: * From RHFM::~RenderFrameHostManager and ::OnBeginNavigation * Into NavigationRequest::CancelNavigation * And from there finally switching to the IO thread and calling ResourceDispatcherHost::CancelNavigationRequest I talked to Camille and there was apparently no way already in place to mock ans override the calls to the IO thread, which would be the ideal situation for this test. And this gives makes me think we're not actually doing unit test, right? I'll further investigate into the RFHM test setup but any advice in this case -- am I going the right-clank-project-way? -- is much appreciated.
I see two options now to get this this test done: * I could extend ResourceDispatcherHostImpl and override CancelNavigationRequest * I could update both ResourceDispatcherHostImpl and ResourceDispatcherHostDelegate so to hook the delegate-checks for the new methods we are introducing with PlzNavigate (mainly CancelNavigationRequest for now) so that I can create a TestResourceDispatcherHostDelegate and hook it at test time to detect the proper calls are made I feel like the 2nd one is the best approach as it's already in use in other tests and the hook-point for setting the delegate already exists (see web_navigation_apitest.cc, plugin_browsertest.cc and resource_dispatcher_host_unittest.cc). First one might be simpler though or I might be wrong about the proper role of the ResourceDispatcherHostDelegate. But I also feel like this CL is growing a bit too big and I'd suggest I should break it in two: * I'll initially remove all the observer code and revert the test to what it used to be (so it won't test the cancel calls) * In a following CL I'll add the testing of the cancel calls WDYT? On 2014/08/29 11:18:28, carlosk wrote: > I created the Observer only for testing purposes (at least for now there's no > other use for it) because I couldn't find a way to actually detect if the proper > navigation cancellation calls: > * From RHFM::~RenderFrameHostManager and ::OnBeginNavigation > * Into NavigationRequest::CancelNavigation > * And from there finally switching to the IO thread and calling > ResourceDispatcherHost::CancelNavigationRequest > > I talked to Camille and there was apparently no way already in place to mock ans > override the calls to the IO thread, which would be the ideal situation for this > test. And this gives makes me think we're not actually doing unit test, right? > > I'll further investigate into the RFHM test setup but any advice in this case -- > am I going the right-clank-project-way? -- is much appreciated.
Thanks! I did reply some of the comments do I'd please ask you to take a look at those. The non replied ones are related to the Observer that as I suggested could be removed for now. So as I think it will not "live to see the light of the day" I won't reply to them right not. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.cc:17: int64 next_navigation_request_id_ = 0; On 2014/08/28 15:55:05, (Do not use) nasko wrote: > nit: make this one static I added the static. But from what I read pre-pending static to a global variable makes it inaccessible from other compilation units. But doesn't declaring it inside an anonymous namespace achieves the same goal? Also, is there any styleguide rule or just practical rule against me just defining this as a static variable inside the scope of the function that sets new IDs? Ex: void foo() { (...) static int new_id_source = 0; my_id = ++new_id_source; (...) } https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.cc:60: single_observer_->set_navigation_request(NULL); On 2014/08/28 12:17:01, clamy wrote: > nit: it may actually be unnecessary to set the the navigation request to NULL > here. However, in that case, the implementation of RequestDestroyed should set > it to null. Agreed it should be moved out of here. It is necessary to do it though because if the observer is destroyed after the request it will try to un-register itself using that pointer (it it's not set to NULL). https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.h:30: CONTENT_EXPORT Observer(); On 2014/08/28 12:17:01, clamy wrote: > The constructors and the destructor should be protected. > > Also the empty constructor is not used anywhere, so I think it should be > removed. Done. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:445: navigation_request_.reset(); On 2014/08/28 15:55:05, (Do not use) nasko wrote: > This seems a bit fragile to me. Once we tell the renderer that the navigation > has committed, we don't need to hold the nav request in RFHM, do we? Since this > is in response to renderer telling us it committed something, there can be race > conditions between quick navigations. I think this was what was discussed between you Nasko and Camille in this week's meeting which I wasn't able to attend(*). From what Camille told me I understood you two agreed the correct place to destroy the NavigationRequest instance was at the DidNavigateFrame call. Did I get something wrong? (*): Really sorry about that but it was due to an urgent matter regarding the relocation of my belongings. :/ https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:602: if (navigation_request_.get()) On 2014/08/28 12:17:01, clamy wrote: > Please add a comment explaining that if we have a non-commited navigation > request in flight, it should be canceled. Done. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1812: int cancelCounter; On 2014/08/28 12:17:01, clamy wrote: > I would rather have a boolean than a counter here. In this case I prefer counters instead of booleans because I more easily can verify they are not called anymore when they shouldn't. WDYT? https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1813: On 2014/08/28 12:17:01, clamy wrote: > nit: unnecessary line. Done. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1815: On 2014/08/28 12:17:01, clamy wrote: > nit: unnecessary line. Done. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1869: // Confirms both 1st and 2nd observers are at the expected states On 2014/08/28 12:17:01, clamy wrote: > I would replace this comment with something a bit more explicit: > The first request should have been canceled and destroyed. > The second request should have not been canceled neither destroyed. Done. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1880: // Confirms both 1st and 2nd observers are at the expected states On 2014/08/28 12:17:01, clamy wrote: > I would replace the comment with something like: > The second request should have been destroyed but not canceled. Done.
Comments addressed and weird Observer removed. I'm trying to submit this CL now and in the next one add the in depth testing of the calls to ResourceDispatcherHost in the IO thread (using a test implementation of a ResourceDispatcherHostDelegate). PTAL? Thanks! https://chromiumcodereview.appspot.com/475783002/diff/160001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/160001/content/browser/... content/browser/frame_host/navigation_request.h:27: const NavigationRequestInfo& info() const { return info_; } On 2014/08/28 12:17:01, clamy wrote: > Why did you move those methods? I just wanted to group the accessors/getters altogether in a single group; it seemed easier to read the file this way. WDYT?
Comments addressed and weird Observer removed. I'm trying to submit this CL now and in the next one add the in depth testing of the calls to ResourceDispatcherHost in the IO thread (using a test implementation of a ResourceDispatcherHostDelegate). PTAL? Thanks!
LGTM with a few nits. https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.cc:17: int64 next_navigation_request_id_ = 0; On 2014/08/29 15:54:43, carlosk wrote: > On 2014/08/28 15:55:05, (Do not use) nasko wrote: > > nit: make this one static > > I added the static. > > But from what I read pre-pending static to a global variable makes it > inaccessible from other compilation units. But doesn't declaring it inside an > anonymous namespace achieves the same goal? This is a good point. Use whichever you prefer. > Also, is there any styleguide rule or just practical rule against me just > defining this as a static variable inside the scope of the function that sets > new IDs? Ex: > > void foo() { > (...) > static int new_id_source = 0; > my_id = ++new_id_source; > (...) > } I don't know of a style rule about that. https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/navigation_before_commit_info.cc:10: : navigation_request_id(0) {} nit: Why not initialize this to -1 or some other invalid value, such that we can ensure that a valid value has been written to the variable before using it? https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:24: nit: no need for empty line https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:40: int64 frame_node_id() const { return frame_node_id_; } nit: frame_tree_node_id https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1838: EXPECT_EQ(main_test_rfh(), rfh1); I think it is better to compare SiteInstances of those, since rfh1 will likely be deleted if an actual navigation was performed, resulting in use-after-free bug and invalid test, since noone will invalidate the value of rfh1.
Thanks! LGTM
So I think we got to a pretty nice state now. The tests are now able to actually check for the SiteInstance GURL as the PlzNavigate implementation is setting them properly. You might wanna check how I did my is-site-instance-equal checks though. If that's OK this CL should be GtG. :) https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/220001/content/browser/... content/browser/frame_host/navigation_request.cc:17: int64 next_navigation_request_id_ = 0; On 2014/09/02 14:10:56, nasko wrote: > On 2014/08/29 15:54:43, carlosk wrote: > > On 2014/08/28 15:55:05, (Do not use) nasko wrote: > > > nit: make this one static > > > > I added the static. > > > > But from what I read pre-pending static to a global variable makes it > > inaccessible from other compilation units. But doesn't declaring it inside an > > anonymous namespace achieves the same goal? > > This is a good point. Use whichever you prefer. > > > Also, is there any styleguide rule or just practical rule against me just > > defining this as a static variable inside the scope of the function that sets > > new IDs? Ex: > > > > void foo() { > > (...) > > static int new_id_source = 0; > > my_id = ++new_id_source; > > (...) > > } > > I don't know of a style rule about that. Acknowledged. https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... File content/browser/frame_host/navigation_before_commit_info.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/navigation_before_commit_info.cc:10: : navigation_request_id(0) {} On 2014/09/02 14:10:57, nasko wrote: > nit: Why not initialize this to -1 or some other invalid value, such that we can > ensure that a valid value has been written to the variable before using it? Done. https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:24: On 2014/09/02 14:10:57, nasko wrote: > nit: no need for empty line Done. https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/navigation_request.h:40: int64 frame_node_id() const { return frame_node_id_; } On 2014/09/02 14:10:57, nasko wrote: > nit: frame_tree_node_id Done. https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1838: EXPECT_EQ(main_test_rfh(), rfh1); On 2014/09/02 14:10:57, nasko wrote: > I think it is better to compare SiteInstances of those, since rfh1 will likely > be deleted if an actual navigation was performed, resulting in use-after-free > bug and invalid test, since noone will invalidate the value of rfh1. Done! When I initially wrote this test the that wasn't still possible as PlzNavigate wasn't properly setting the SiteInstance yet, hence the (now removed) TODO at line 1801.
https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1838: EXPECT_EQ(main_test_rfh(), rfh1); On 2014/09/02 15:30:09, carlosk wrote: > On 2014/09/02 14:10:57, nasko wrote: > > I think it is better to compare SiteInstances of those, since rfh1 will likely > > be deleted if an actual navigation was performed, resulting in use-after-free > > bug and invalid test, since noone will invalidate the value of rfh1. > > Done! > > When I initially wrote this test the that wasn't still possible as PlzNavigate > wasn't properly setting the SiteInstance yet, hence the (now removed) TODO at > line 1801. You can just capture the site instance of an RFH into a variable and later on do EXPECT_EQ(). See CreateSwappedOutOpenerRVHs for an example or other tests in this file.
Latest and greatest with the small improvement on the test assertions. PTAL? https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/475783002/diff/240001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1838: EXPECT_EQ(main_test_rfh(), rfh1); On 2014/09/02 17:20:28, (Do not use) nasko wrote: > On 2014/09/02 15:30:09, carlosk wrote: > > On 2014/09/02 14:10:57, nasko wrote: > > > I think it is better to compare SiteInstances of those, since rfh1 will > likely > > > be deleted if an actual navigation was performed, resulting in > use-after-free > > > bug and invalid test, since noone will invalidate the value of rfh1. > > > > Done! > > > > When I initially wrote this test the that wasn't still possible as PlzNavigate > > wasn't properly setting the SiteInstance yet, hence the (now removed) TODO at > > line 1801. > > You can just capture the site instance of an RFH into a variable and later on do > EXPECT_EQ(). See CreateSwappedOutOpenerRVHs for an example or other tests in > this file. Executed as discussed with Nasko.
New test code LGTM
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/carlosk@chromium.org/475783002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
content/browser/frame_host/render_frame_host_manager_unittest.cc:
While running git apply --index -p1;
error: patch failed:
content/browser/frame_host/render_frame_host_manager_unittest.cc:1788
error: content/browser/frame_host/render_frame_host_manager_unittest.cc: patch
does not apply
Patch: content/browser/frame_host/render_frame_host_manager_unittest.cc
Index: content/browser/frame_host/render_frame_host_manager_unittest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc
b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index
acf5f139c4767ecea696694d8bceea8537a41bcd..9e53e6ccf2f3af4700b03d77d5f8318371deb2fd
100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -1698,6 +1698,7 @@ TEST_F(RenderFrameHostManagerTest,
BrowserSideNavigationBeginNavigation) {
const GURL kUrl1("http://www.google.com/");
const GURL kUrl2("http://www.chromium.org/");
const GURL kUrl3("http://www.gmail.com/");
+ const int64 kFirstNavRequestID = 1;
// TODO(clamy): we should be enabling browser side navigations here
// when CommitNavigation is properly implemented.
@@ -1722,6 +1723,7 @@ TEST_F(RenderFrameHostManagerTest,
BrowserSideNavigationBeginNavigation) {
kUrl1, subframe_request->info().first_party_for_cookies);
EXPECT_FALSE(subframe_request->info().is_main_frame);
EXPECT_TRUE(subframe_request->info().parent_is_main_frame);
+ EXPECT_EQ(kFirstNavRequestID, subframe_request->navigation_request_id());
// Simulate a BeginNavigation IPC on the main frame.
contents()->GetMainFrame()->SendBeginNavigationWithURL(kUrl3);
@@ -1732,6 +1734,7 @@ TEST_F(RenderFrameHostManagerTest,
BrowserSideNavigationBeginNavigation) {
EXPECT_EQ(kUrl3, main_request->info().first_party_for_cookies);
EXPECT_TRUE(main_request->info().is_main_frame);
EXPECT_FALSE(main_request->info().parent_is_main_frame);
+ EXPECT_EQ(kFirstNavRequestID + 1, main_request->navigation_request_id());
}
// PlzNavigate: Test that RequestNavigation creates a NavigationRequest and
that
@@ -1788,9 +1791,59 @@ TEST_F(RenderFrameHostManagerTest,
NavigationBeforeCommitInfo commit_info;
commit_info.navigation_url = kUrl2;
+ commit_info.navigation_request_id = main_request->navigation_request_id();
render_manager->CommitNavigation(commit_info);
- main_request = GetNavigationRequestForRenderFrameManager(render_manager);
EXPECT_NE(main_test_rfh(), rfh);
}
+// PlzNavigate: Test that a navigation commit is ignored if another request has
+// been issued in the meantime.
+// TODO(carlosk): add checks to assert that the cancel call was sent to
+// ResourceDispatcherHost in the IO thread by extending
+// ResourceDispatcherHostDelegate (like in cross_site_transfer_browsertest.cc
+// and plugin_browsertest.cc).
+TEST_F(RenderFrameHostManagerTest,
+ BrowserSideNavigationIgnoreStaleNavigationCommit) {
+ const GURL kUrl0("http://www.wikipedia.org/");
+ const GURL kUrl0_site = SiteInstance::GetSiteForURL(browser_context(),
kUrl0);
+ const GURL kUrl1("http://www.chromium.org/");
+ const GURL kUrl2("http://www.google.com/");
+ const GURL kUrl2_site = SiteInstance::GetSiteForURL(browser_context(),
kUrl2);
+
+ // Initialization.
+ contents()->NavigateAndCommit(kUrl0);
+ RenderFrameHostManager* render_manager =
+ main_test_rfh()->frame_tree_node()->render_manager();
+ EnableBrowserSideNavigation();
+ EXPECT_EQ(kUrl0_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
+
+ // Request navigation to the 1st URL and gather data.
+ main_test_rfh()->SendBeginNavigationWithURL(kUrl1);
+ NavigationRequest* request1 =
+ GetNavigationRequestForRenderFrameManager(render_manager);
+ ASSERT_TRUE(request1);
+ int64 request_id1 = request1->navigation_request_id();
+
+ // Request navigation to the 2nd URL and gather more data.
+ main_test_rfh()->SendBeginNavigationWithURL(kUrl2);
+ NavigationRequest* request2 =
+ GetNavigationRequestForRenderFrameManager(render_manager);
+ ASSERT_TRUE(request2);
+ int64 request_id2 = request2->navigation_request_id();
+ EXPECT_NE(request_id1, request_id2);
+
+ // Confirms that a stale commit is ignored by the RHFM.
+ NavigationBeforeCommitInfo nbc_info;
+ nbc_info.navigation_url = kUrl1;
+ nbc_info.navigation_request_id = request_id1;
+ render_manager->CommitNavigation(nbc_info);
+ EXPECT_EQ(kUrl0_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
+
+ // Confirms that a valid, request-matching commit is correctly processed.
+ nbc_info.navigation_url = kUrl2;
+ nbc_info.navigation_request_id = request_id2;
+ render_manager->CommitNavigation(nbc_info);
+ EXPECT_EQ(kUrl2_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
+}
+
} // namespace content
I had a merge issue and that's why I'm submitting a new version. Weird thing is that when I rebased locally I did't get any merging to resolve manually... Also, I'm getting an error in test RenderFrameHostManagerTest::BrowserSideNavigationRequestNavigationNoLiveRenderer where it is expected that the RenderViewHost instance should have been initialized but apparently it's not. It seems related to a submit by clamy. I'm just being hopeful and trying to submit the CL again. :)
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/carlosk@chromium.org/475783002/300001
On 2014/09/04 13:43:19, carlosk wrote: > I had a merge issue and that's why I'm submitting a new version. Weird thing is > that when I rebased locally I did't get any merging to resolve manually... > > Also, I'm getting an error in test > > RenderFrameHostManagerTest::BrowserSideNavigationRequestNavigationNoLiveRenderer > > where it is expected that the RenderViewHost instance should have been > initialized but apparently it's not. It seems related to a submit by clamy. > > I'm just being hopeful and trying to submit the CL again. :) The failing EXPECT came from this CL: https://chromiumcodereview.appspot.com/536473002/
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/carlosk@chromium.org/475783002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/carlosk@chromium.org/475783002/320001
Message was sent while issue was closed.
Committed patchset #15 (id:320001) as 7f063f6e44f47bf1912fb3275536b713d44cdb6b
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/57c81e9cbddce2d2938ac5a80164aecb6c1b51f2 Cr-Commit-Position: refs/heads/master@{#293484} |
