Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1157863005:
Use WebFrame::loadRequest for reloads and history navigations (Closed)
|
Created:
5 years, 7 months ago by clamy Modified:
5 years, 6 months ago Reviewers:
Avi (use Gerrit), carlosk, nasko, Fabrice (no longer in Chrome), Nate Chapin, Charlie Reis CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse WebFrame::loadRequest for reloads and history navigations
This CL follows the blink interface changes of
https://codereview.chromium.org/1156473002/. It removes the uses of
WebFrame::reload, WebFrame::reloadWithOverrideURL and WebFrame::loadHistoryItem
in favor of using WebFrame::loadRequest for all kinds of navigations.
PlzNavigate: reload and history navigations are properly recognized as such at
commit time when browser-side navigation is enabled.
BUG=490713
Committed: https://crrev.com/8751a8d7a2f64e8678d61f58edfcbb4dd83d4fdb
Cr-Commit-Position: refs/heads/master@{#333485}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Rebase #Patch Set 3 : Adressed comments #
Total comments: 4
Patch Set 4 : Rebase #Patch Set 5 : Addressed Charlie's comments #
Total comments: 10
Patch Set 6 : Rebase #Patch Set 7 : Addressed Charlie's comments + rebase on blink patch #
Total comments: 6
Patch Set 8 : Rebase #Patch Set 9 : Addressed comments #Patch Set 10 : Rebase #
Messages
Total messages: 16 (3 generated)
@avi, creis, japhet: PTAL @nasko, carlosk, fdegans: FYI This is the chromium side patch of https://codereview.chromium.org/1156473002/. Note that currently it only switches to using loadRequest in RenderFrameImpl:;navigateInternal and in the HistoryController. I'd like to have your opinion on the general design before removing all usage of reload/loadHistoryItem.
Hmm, I don't think this is a good time to be making these changes. I'm going to be moving most of HistoryController (especially RecursiveGoToItem) to the browser process very soon, which significantly affects this code. I have some concerns about how this works in the meantime, but we can put those off if we decide to wait for the move. https://codereview.chromium.org/1157863005/diff/1/content/public/test/render_... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/1157863005/diff/1/content/public/test/render_... content/public/test/render_view_test.cc:552: impl->GetMainRenderFrame()->OnCommitNavigation( This feels strange to pass both common_params.url and common_params. I thought the first URL was supposed to be a stream identifier? https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... File content/renderer/history_controller.cc (right): https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.cc:106: frame->loadRequest(request, WebFrame::WebFrameLoadTypeBackForward, The old names seemed like a clearer pair to me (SameDocumentLoad vs DifferentDocumentLoad). https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.cc:123: RecursiveGoToEntry(main_frame, same_document_loads, I'd like to avoid this if we can. I'm trying to move RecursiveGoToEntry to the browser process in the next week or two. https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... File content/renderer/history_controller.h (right): https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.h:114: // Returns the WebHistoryItem corresponding to |target_entry|. If no item is This sentence doesn't make sense, since a HistoryEntry has a tree of HistoryItems. There isn't one WebHistoryItem corresponding to it, unless you're looking for the one that matches |target_frame|. If that's the case, it sounds like we might be able to replace this method with target_entry->GetItemForFrame(target_frame)? https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.h:115: // found, then returns an empty WebHistoryItem. Also updates load_type to nit: |load_type| I'm not a fan of methods that do multiple things (especially using both return types and out parameters) if we can avoid it. https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.h:118: blink::WebHistoryItem FindItemForFrame( I'd like to avoid adding new code to HistoryController at this point if possible, especially code that depends on RecursiveGoToEntry. I'm on the brink of moving that to the browser process. https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4442: Send(new FrameHostMsg_DidDropNavigation(routing_id_)); I don't understand this case. When would this happen? https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4463: Send(new FrameHostMsg_DidDropNavigation(routing_id_)); It feels awkward to have this in multiple places, especially when we didn't need it before. https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4478: } else { No else needed after a return. https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4480: render_view_->history_controller()->FindItemForFrame( This isn't going to make sense soon, and I'm not sure I understand it now. Soon, the browser process will be telling each frame which history item to load directly. For now, we only get to NavigateInternal for the main frame, so this would only find a single item for the main frame. How would we handle the case of going back in a subframe?
Thanks! I may not have been clear in my description of the patch, but the goal of this is just to start working on history navigation support for PlzNavigate, by having the right interface in blink. I intend to have further patches to enable history navigation support in subframes, most likely after the code in HistoryController has been moved to the browser. https://codereview.chromium.org/1157863005/diff/1/content/public/test/render_... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/1157863005/diff/1/content/public/test/render_... content/public/test/render_view_test.cc:552: impl->GetMainRenderFrame()->OnCommitNavigation( On 2015/05/27 23:34:35, Charlie Reis wrote: > This feels strange to pass both common_params.url and common_params. I thought > the first URL was supposed to be a stream identifier? Yes. This was testing code that I removed in the latest patch set that I forgot to upload before sending the cL out for review :). https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... File content/renderer/history_controller.cc (right): https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.cc:106: frame->loadRequest(request, WebFrame::WebFrameLoadTypeBackForward, On 2015/05/27 23:34:35, Charlie Reis wrote: > The old names seemed like a clearer pair to me (SameDocumentLoad vs > DifferentDocumentLoad). I brought back the WebHistoryLoadType following comments by Nate in the other patch. https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.cc:123: RecursiveGoToEntry(main_frame, same_document_loads, On 2015/05/27 23:34:35, Charlie Reis wrote: > I'd like to avoid this if we can. I'm trying to move RecursiveGoToEntry to the > browser process in the next week or two. Done. https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... File content/renderer/history_controller.h (right): https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.h:114: // Returns the WebHistoryItem corresponding to |target_entry|. If no item is On 2015/05/27 23:34:36, Charlie Reis wrote: > This sentence doesn't make sense, since a HistoryEntry has a tree of > HistoryItems. There isn't one WebHistoryItem corresponding to it, unless you're > looking for the one that matches |target_frame|. > > If that's the case, it sounds like we might be able to replace this method with > target_entry->GetItemForFrame(target_frame)? Ah I didn't know we had this method. The only thing remaining is to determine if the history load is a same document load or not, though I can assume it's always a different document load for the moment in PlzNavigate. https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.h:115: // found, then returns an empty WebHistoryItem. Also updates load_type to On 2015/05/27 23:34:36, Charlie Reis wrote: > nit: |load_type| > > I'm not a fan of methods that do multiple things (especially using both return > types and out parameters) if we can avoid it. I'm removing the method in the next patch set. https://codereview.chromium.org/1157863005/diff/1/content/renderer/history_co... content/renderer/history_controller.h:118: blink::WebHistoryItem FindItemForFrame( On 2015/05/27 23:34:36, Charlie Reis wrote: > I'd like to avoid adding new code to HistoryController at this point if > possible, especially code that depends on RecursiveGoToEntry. I'm on the brink > of moving that to the browser process. Done. https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4442: Send(new FrameHostMsg_DidDropNavigation(routing_id_)); On 2015/05/27 23:34:36, Charlie Reis wrote: > I don't understand this case. When would this happen? Following interface changes in the blink patch this is no longer needed. https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4463: Send(new FrameHostMsg_DidDropNavigation(routing_id_)); On 2015/05/27 23:34:36, Charlie Reis wrote: > It feels awkward to have this in multiple places, especially when we didn't need > it before. I can remove it. It just seemed weird to me that the renderer did not inform the browser it did not proceed with the navigation it was asked to do. https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4478: } else { On 2015/05/27 23:34:36, Charlie Reis wrote: > No else needed after a return. Done. https://codereview.chromium.org/1157863005/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:4480: render_view_->history_controller()->FindItemForFrame( On 2015/05/27 23:34:36, Charlie Reis wrote: > This isn't going to make sense soon, and I'm not sure I understand it now. > Soon, the browser process will be telling each frame which history item to load > directly. > > For now, we only get to NavigateInternal for the main frame, so this would only > find a single item for the main frame. How would we handle the case of going > back in a subframe? With PlzNavigate we also get to NavigateInternal on subframes (we get there when the response for the navigation was received). If we get the history item from the browser it's even better since we would just set item_for_history_navigation to whatever we received in the meantime (and no need to find the relevant one). Once you're done, I will just have to make sure that the proper history item is sent to the subframes, hence the TODO. I hope what we are doing is clearer in the new patchset with the call to GetHistoryNodeForFrame.
Thanks-- this seems more reasonable to me. One request for making the method a bit easier to maintain. https://codereview.chromium.org/1157863005/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4413: Send(new FrameHostMsg_DidDropNavigation(routing_id_)); This method doesn't feel like it handles early returns well. We keep having to reset the pending nav params and possibly send DidDropNavigation. That's bad for things like code duplication, potentially missing cases, and updating the code in the future. I think this is a case where it would be much easier to maintain if there were a single block at the end that (1) either called loadRequest or sent DidDropNavigation, and (2) reset the pending nav params (and did any other cleanup we might need in the future). Is it possible to structure it this way, maybe with else blocks or a bool to decide which action to take for (1) if necessary? (We could also use another level of helper methods, though that seems a bit awkward.) https://codereview.chromium.org/1157863005/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4437: load_type = blink::BackForward; Worth noting that this won't fully work today (probably only for the main frame, even if there are subframes), but it seems like it's in the right direction for where we're heading. Sounds ok to me for now, since it's just in PlzNavigate.
Thanks! https://codereview.chromium.org/1157863005/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4413: Send(new FrameHostMsg_DidDropNavigation(routing_id_)); On 2015/06/02 05:41:42, Charlie Reis wrote: > This method doesn't feel like it handles early returns well. We keep having to > reset the pending nav params and possibly send DidDropNavigation. That's bad > for things like code duplication, potentially missing cases, and updating the > code in the future. > > I think this is a case where it would be much easier to maintain if there were a > single block at the end that (1) either called loadRequest or sent > DidDropNavigation, and (2) reset the pending nav params (and did any other > cleanup we might need in the future). > > Is it possible to structure it this way, maybe with else blocks or a bool to > decide which action to take for (1) if necessary? (We could also use another > level of helper methods, though that seems a bit awkward.) I've tried to do something with a boolean and if/else conditions. I'm just a bit worried that we end up with lots of intricated if conditions. https://codereview.chromium.org/1157863005/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4437: load_type = blink::BackForward; On 2015/06/02 05:41:42, Charlie Reis wrote: > Worth noting that this won't fully work today (probably only for the main frame, > even if there are subframes), but it seems like it's in the right direction for > where we're heading. Sounds ok to me for now, since it's just in PlzNavigate. Acknowledged. I'm waiting for the HistoryController change before looking at the situation with subframes in PlzNavigate.
Ok, LGTM with nits (if the UpdateFrameNavigationTiming thing isn't a problem). https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4390: bool perform_load_url = false; nit: should_load_request Also move up higher, probably before load_type. Right now it's under the reload comment, but it's not specific to that. https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4396: if (!browser_side_navigation) { Sanity check: Is RELOAD_ORIGINAL_REQUEST_URL handled earlier for PlzNavigate? That would make sense to me. https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4427: // once the HistoryController has moved to the browser. nit: Move to previous line. https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4471: UpdateFrameNavigationTiming(frame_, request_params.browser_navigation_start, What are the implications of calling this before the loadRequest call? https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4486: } Ah. Looks like you skipped the "else send DidDropNavigation" because of the HistoryController::GoToEntry case. I'm ok with that. It's no worse than what we have today, and we can possibly add it when GoToEntry moves to the browser process.
Thanks! The NavigationTiming thing seemed to be a problem, so I moved it back below the load call. https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4390: bool perform_load_url = false; On 2015/06/02 16:27:45, Charlie Reis wrote: > nit: should_load_request > > Also move up higher, probably before load_type. Right now it's under the reload > comment, but it's not specific to that. Done. https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4396: if (!browser_side_navigation) { On 2015/06/02 16:27:45, Charlie Reis wrote: > Sanity check: Is RELOAD_ORIGINAL_REQUEST_URL handled earlier for PlzNavigate? > That would make sense to me. That should be handled on the browser-side. We get here when the request as already been made and the response is ready. https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4427: // once the HistoryController has moved to the browser. On 2015/06/02 16:27:45, Charlie Reis wrote: > nit: Move to previous line. Done. https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4471: UpdateFrameNavigationTiming(frame_, request_params.browser_navigation_start, On 2015/06/02 16:27:45, Charlie Reis wrote: > What are the implications of calling this before the loadRequest call? That could be a problem since it checks for the provisional data source of the frame. Moved it to after the loadRequest call for normal navigations. https://codereview.chromium.org/1157863005/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:4486: } On 2015/06/02 16:27:45, Charlie Reis wrote: > Ah. Looks like you skipped the "else send DidDropNavigation" because of the > HistoryController::GoToEntry case. I'm ok with that. It's no worse than what > we have today, and we can possibly add it when GoToEntry moves to the browser > process. Yes that was my thought process as well.
Just a couple of drive-by nits. https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:4489: // time nit: combine this line with the next https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:4497: if (load_type == blink::WebFrameLoadType::Standard) nit: need {} around multiline body
Thanks. LGTM once this and Nasko's comments are fixed. https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:4387: bool perform_load_url = false; Please change this to should_load_request.
Thanks! https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:4387: bool perform_load_url = false; On 2015/06/03 20:40:26, Charlie Reis wrote: > Please change this to should_load_request. Done. https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:4489: // time On 2015/06/03 19:45:36, nasko wrote: > nit: combine this line with the next Done. https://codereview.chromium.org/1157863005/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:4497: if (load_type == blink::WebFrameLoadType::Standard) On 2015/06/03 19:45:37, nasko wrote: > nit: need {} around multiline body Done.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1157863005/#ps170001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1157863005/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/8751a8d7a2f64e8678d61f58edfcbb4dd83d4fdb Cr-Commit-Position: refs/heads/master@{#333485} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
