Index: content/browser/frame_host/render_frame_host_impl.cc |
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc |
index d9327b01b394d38379f1426313c482be02f553c7..22cd4ac44e3fbdb7be51df04aa064cbfbb0b6064 100644 |
--- a/content/browser/frame_host/render_frame_host_impl.cc |
+++ b/content/browser/frame_host/render_frame_host_impl.cc |
@@ -1179,70 +1179,26 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) { |
return; |
} |
- // If the URL or |was_within_same_page| does not match what the |
- // NavigationHandle expects, treat the commit as a new navigation. This can |
- // happen if an ongoing slow same-process navigation is interwoven with a |
- // synchronous renderer-initiated navigation. |
- // TODO(csharrison): Data navigations loaded with LoadDataWithBaseURL get |
- // reset here, because the NavigationHandle tracks the URL but the |
- // validated_params.url tracks the data. The trick of saving the old entry ids |
- // for these navigations should go away when this is properly handled. See |
- // crbug.com/588317. |
- int entry_id_for_data_nav = 0; |
- bool is_renderer_initiated = true; |
- if (navigation_handle_ && |
- ((navigation_handle_->GetURL() != validated_params.url) || |
- navigation_handle_->IsSamePage() != |
- validated_params.was_within_same_page)) { |
- // Make sure that the pending entry was really loaded via |
- // LoadDataWithBaseURL and that it matches this handle. |
- NavigationEntryImpl* pending_entry = |
- NavigationEntryImpl::FromNavigationEntry( |
- frame_tree_node()->navigator()->GetController()->GetPendingEntry()); |
- bool pending_entry_matches_handle = |
- pending_entry && |
- pending_entry->GetUniqueID() == |
- navigation_handle_->pending_nav_entry_id(); |
- // TODO(csharrison): The pending entry's base url should equal |
- // |validated_params.base_url|. This is not the case for loads with invalid |
- // base urls. |
Charlie Reis
2016/11/03 22:02:22
Is there a reason not to preserve this TODO?
clamy
2016/11/04 14:18:21
Nope, it got lost in code reordering. I brought it
|
- if (navigation_handle_->GetURL() == validated_params.base_url && |
- pending_entry_matches_handle && |
- !pending_entry->GetBaseURLForDataURL().is_empty()) { |
- entry_id_for_data_nav = navigation_handle_->pending_nav_entry_id(); |
- is_renderer_initiated = pending_entry->is_renderer_initiated(); |
+ // PlzNavigate |
+ if (!navigation_handle_ && IsBrowserSideNavigationEnabled()) { |
+ // PlzNavigate: the browser has not been notified about the start of the |
+ // load in this renderer yet. Do it now. |
Charlie Reis
2016/11/03 22:02:22
nit: Add something like: (e.g., for same-page navi
clamy
2016/11/04 14:18:21
Done.
|
+ if (!is_loading()) { |
+ bool was_loading = frame_tree_node()->frame_tree()->IsLoading(); |
+ is_loading_ = true; |
+ frame_tree_node()->DidStartLoading(true, was_loading); |
} |
- navigation_handle_.reset(); |
+ pending_commit_ = false; |
} |
- // Synchronous renderer-initiated navigations will send a |
- // DidCommitProvisionalLoad IPC without a prior DidStartProvisionalLoad |
- // message. Or in addition, the if block above can reset the NavigationHandle |
- // in cases it doesn't match the expected commit. |
- if (!navigation_handle_) { |
- // There is no pending NavigationEntry in these cases, so pass 0 as the |
- // nav_id. If the previous handle was a prematurely aborted navigation |
- // loaded via LoadDataWithBaseURL, propogate the entry id. |
- navigation_handle_ = NavigationHandleImpl::Create( |
- validated_params.url, frame_tree_node_, is_renderer_initiated, |
- validated_params.was_within_same_page, validated_params.is_srcdoc, |
- base::TimeTicks::Now(), entry_id_for_data_nav, |
- false); // started_from_context_menu |
- // PlzNavigate |
- if (IsBrowserSideNavigationEnabled()) { |
- // PlzNavigate: synchronous loads happen in the renderer, and the browser |
- // has not been notified about the start of the load yet. Do it now. |
- if (!is_loading()) { |
- bool was_loading = frame_tree_node()->frame_tree()->IsLoading(); |
- is_loading_ = true; |
- frame_tree_node()->DidStartLoading(true, was_loading); |
- } |
- pending_commit_ = false; |
- } |
- } |
+ // Find the appropriate NavigationHandle for this navigation. |
+ std::unique_ptr<NavigationHandleImpl> navigation_handle = |
+ FindNavigationHandleForCommit(validated_params); |
+ DCHECK(navigation_handle); |
accessibility_reset_count_ = 0; |
- frame_tree_node()->navigator()->DidNavigate(this, validated_params); |
+ frame_tree_node()->navigator()->DidNavigate(this, validated_params, |
+ std::move(navigation_handle)); |
// Since we didn't early return, it's safe to keep the commit state. |
commit_state_resetter.disable(); |
@@ -3189,4 +3145,70 @@ void RenderFrameHostImpl::DeleteWebBluetoothService() { |
web_bluetooth_service_.reset(); |
} |
+std::unique_ptr<NavigationHandleImpl> |
+RenderFrameHostImpl::FindNavigationHandleForCommit( |
+ const FrameHostMsg_DidCommitProvisionalLoad_Params& params) { |
+ // If this is a same-page navigation, there isn't an existing NavigationHandle |
+ // to use for the navigation. Create one, but don't reset any NavigationHandle |
+ // tracking an ongoing navigation, since this may lead to the cancellation of |
+ // the navigation. |
+ if (params.was_within_same_page) { |
+ // There is no pending NavigationEntry in these cases, so pass 0 as the |
+ // nav_id. |
Charlie Reis
2016/11/03 22:02:22
nit: pending_nav_entry_id
clamy
2016/11/04 14:18:21
Done.
|
+ return NavigationHandleImpl::Create( |
+ params.url, frame_tree_node_, true, // is_renderer_initiated |
Charlie Reis
2016/11/03 22:02:22
Hmm, is_renderer_initiated isn't necessarily true
clamy
2016/11/04 14:18:21
That's a good point, though in this case we don't
Charlie Reis
2016/11/04 17:30:53
Good point. I think missing the canceled-pending-
|
+ params.was_within_same_page, params.is_srcdoc, base::TimeTicks::Now(), |
+ 0, false); // started_from_context_menu |
+ } |
+ |
+ // Determine if the current NavigationHandle can be used. |
+ if (navigation_handle_ && navigation_handle_->GetURL() == params.url) { |
+ DCHECK(!navigation_handle_->IsSamePage()); |
+ return std::move(navigation_handle_); |
+ } |
+ |
+ // If the URL does not match what the NavigationHandle expects, treat the |
+ // commit as a new navigation. This can happen when loading a Data |
+ // navigation with LoadDataWithBaseURL. |
+ // |
+ // TODO(csharrison): Data navigations loaded with LoadDataWithBaseURL get |
+ // reset here, because the NavigationHandle tracks the URL but the params.url |
+ // tracks the data. The trick of saving the old entry ids for these |
+ // navigations should go away when this is properly handled. |
+ // See crbug.com/588317. |
+ int entry_id_for_data_nav = 0; |
+ bool is_renderer_initiated = true; |
+ |
+ // Make sure that the pending entry was really loaded via LoadDataWithBaseURL |
+ // and that it matches this handle. TODO(csharrison): The pending entry's |
+ // base url should equal |params.base_url|. This is not the case for loads |
+ // with invalid base urls. |
+ if (navigation_handle_) { |
+ NavigationEntryImpl* pending_entry = |
+ NavigationEntryImpl::FromNavigationEntry( |
+ frame_tree_node()->navigator()->GetController()->GetPendingEntry()); |
+ bool pending_entry_matches_handle = |
+ pending_entry && |
+ pending_entry->GetUniqueID() == |
+ navigation_handle_->pending_nav_entry_id(); |
+ if (navigation_handle_->GetURL() == params.base_url && |
+ pending_entry_matches_handle && |
+ !pending_entry->GetBaseURLForDataURL().is_empty()) { |
+ entry_id_for_data_nav = navigation_handle_->pending_nav_entry_id(); |
+ is_renderer_initiated = pending_entry->is_renderer_initiated(); |
+ } |
+ |
+ // Reset any existing NavigationHandle. |
+ navigation_handle_.reset(); |
+ } |
+ |
+ // There is no pending NavigationEntry in these cases, so pass 0 as the |
+ // nav_id. If the previous handle was a prematurely aborted navigation loaded |
Charlie Reis
2016/11/03 22:02:22
nit: pending_nav_entry_id
clamy
2016/11/04 14:18:21
Done.
|
+ // via LoadDataWithBaseURL, propagate the entry id. |
+ return NavigationHandleImpl::Create( |
+ params.url, frame_tree_node_, is_renderer_initiated, |
+ params.was_within_same_page, params.is_srcdoc, base::TimeTicks::Now(), |
+ entry_id_for_data_nav, false); // started_from_context_menu |
+} |
+ |
} // namespace content |