Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(347)

Unified Diff: chrome/browser/extensions/api/web_navigation/web_navigation_api.cc

Issue 2545133002: PlzNavigate: Fix ordering of onBeforeNavigate and onCreatedNavigationTarget. (Closed)
Patch Set: Created 4 years ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
diff --git a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
index 6d45ec5856295bb365085ebcbf3cdc14a43bc81d..30e1836dd7934613b8a7d6ddd6bc2b6dbdb6166f 100644
--- a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
+++ b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
@@ -271,11 +271,28 @@ void WebNavigationTabObserver::DidStartNavigation(
return;
}
- helpers::DispatchOnBeforeNavigate(navigation_handle);
+ pending_on_before_navigate_event_ =
+ helpers::CreateOnBeforeNavigateEvent(navigation_handle);
+
+ // Only dispatch the onBeforeNavigate event if the associated WebContents
+ // is already added to the tab strip. Otherwise the event should be delayed
+ // and sent after the addition, to preserve the ordering of events.
+ if (ExtensionTabUtil::GetTabById(
Devlin 2016/12/02 20:17:11 I don't love that this is how we check this. It m
nasko 2016/12/02 21:40:45 I think there will be more fallout if we did that
Devlin 2016/12/02 21:58:31 // TODO(nasko|devlin): We have to do this check be
nasko 2016/12/02 23:41:39 Done.
+ ExtensionTabUtil::GetTabId(web_contents()),
+ Profile::FromBrowserContext(web_contents()->GetBrowserContext()),
Devlin 2016/12/02 20:17:11 nit: this can take a browser context, so no need f
nasko 2016/12/02 21:40:45 Done.
+ false, NULL, NULL, NULL, NULL)) {
Devlin 2016/12/02 20:17:11 nit: nullptr in new code.
nasko 2016/12/02 21:40:45 Done.
+ DispatchOnBeforeNavigate();
+ }
}
void WebNavigationTabObserver::DidFinishNavigation(
content::NavigationHandle* navigation_handle) {
+ // If there has been a DidStartNavigation call before the tab was ready to
+ // dispatch events, ensure that it is sent before processing the
+ // DidFinishNavigation.
+ // Note: This is exercised by WebNavigationApiTest.TargetBlankIncognito.
+ DispatchOnBeforeNavigate();
Devlin 2016/12/02 20:17:11 Does this not have the same possible race? Or why
nasko 2016/12/02 21:40:45 This guarantees that if we are going to send a com
Devlin 2016/12/02 21:58:31 If the TabAdded notification is sent before DidSta
nasko 2016/12/02 23:41:39 The BrowserContext passed in should already be the
+
if (navigation_handle->HasCommitted() && !navigation_handle->IsErrorPage()) {
HandleCommit(navigation_handle);
return;
@@ -391,6 +408,17 @@ void WebNavigationTabObserver::WebContentsDestroyed() {
registrar_.RemoveAll();
}
+void WebNavigationTabObserver::DispatchOnBeforeNavigate() {
+ if (!pending_on_before_navigate_event_.get())
+ return;
+
+ Profile* profile =
+ Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+ EventRouter* event_router = EventRouter::Get(profile);
Devlin 2016/12/02 20:17:11 nit: use browser context directly.
nasko 2016/12/02 21:40:45 Done.
+ if (profile && event_router)
Devlin 2016/12/02 20:17:11 can profile/context be null? (Or event router, fo
nasko 2016/12/02 21:40:45 I don't know whether the context can be null, poss
+ event_router->BroadcastEvent(std::move(pending_on_before_navigate_event_));
+}
+
void WebNavigationTabObserver::HandleCommit(
content::NavigationHandle* navigation_handle) {
bool is_reference_fragment_navigation = IsReferenceFragmentNavigation(

Powered by Google App Engine
This is Rietveld 408576698