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

Issue 2545133002: PlzNavigate: Fix ordering of onBeforeNavigate and onCreatedNavigationTarget. (Closed)

Created:
4 years ago by nasko
Modified:
4 years ago
Reviewers:
Devlin, alexmos
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

PlzNavigate: Fix ordering of onBeforeNavigate and onCreatedNavigationTarget. This CL fixes the ordering of onCreatedNavigationTarget and onBeforeNavigate to be correct in all cases of navigating in new tabs. Since navigating in new tabs is done through chrome::Navigate with the proper disposition, the navigation in that case is kicked off before the tab is added to the tabstrip. With PlzNavigate, the difference is that the WebContentsObserver::DidStartNavigation is fired in the same event loop, so it causes onBeforeNavigate to be dispatched before onCreatedNavigationTarget. Prior to PlzNavigate, the navigation had to go to the renderer process, which starts it there and WebContentsObserver::DidStartNavigation is fired in the next iteration of the event loop, which accidentally gives us the right ordering. BUG=575230 Committed: https://crrev.com/2d7f7dc70c5984a92ef96b45e4d4019296112d4d Cr-Commit-Position: refs/heads/master@{#436471}

Patch Set 1 #

Total comments: 20

Patch Set 2 : Fixes based on review. #

Total comments: 2

Patch Set 3 : Another round of review fixes. #

Total comments: 2

Patch Set 4 : Remove fixed tests from filter. #

Patch Set 5 : Check for null the EventRouter before dispatching events. #

Patch Set 6 : Rebase on ToT. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -8 lines) Patch
M chrome/browser/extensions/api/web_navigation/web_navigation_api.h View 1 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/web_navigation/web_navigation_api.cc View 1 2 3 4 2 chunks +35 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.h View 1 3 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc View 1 2 4 chunks +19 lines, -4 lines 0 comments Download
M testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 32 (17 generated)
nasko
Hey Alex and Devlin, Can you review this CL for me? It changes webNavigation API ...
4 years ago (2016-12-02 18:40:27 UTC) #2
Devlin
I'm a little bit worried about this because it seems like we're manipulating the API ...
4 years ago (2016-12-02 19:30:37 UTC) #7
nasko
On 2016/12/02 19:30:37, Devlin wrote: > I'm a little bit worried about this because it ...
4 years ago (2016-12-02 19:50:08 UTC) #8
Devlin
On 2016/12/02 19:50:08, nasko wrote: > On 2016/12/02 19:30:37, Devlin wrote: > > I'm a ...
4 years ago (2016-12-02 20:03:39 UTC) #9
Devlin
https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc#newcode280 chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:280: if (ExtensionTabUtil::GetTabById( I don't love that this is how ...
4 years ago (2016-12-02 20:17:11 UTC) #10
nasko
https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc#newcode280 chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:280: if (ExtensionTabUtil::GetTabById( On 2016/12/02 20:17:11, Devlin wrote: > I ...
4 years ago (2016-12-02 21:40:45 UTC) #11
Devlin
https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc#newcode280 chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:280: if (ExtensionTabUtil::GetTabById( On 2016/12/02 21:40:45, nasko wrote: > On ...
4 years ago (2016-12-02 21:58:31 UTC) #13
nasko
https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc#newcode280 chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:280: if (ExtensionTabUtil::GetTabById( On 2016/12/02 21:58:31, Devlin wrote: > On ...
4 years ago (2016-12-02 23:41:40 UTC) #14
alexmos
This looks fine, just a quick question - is this covered by any tests that ...
4 years ago (2016-12-03 00:03:46 UTC) #17
nasko
> This looks fine, just a quick question - is this covered by any tests ...
4 years ago (2016-12-03 00:19:03 UTC) #18
alexmos
LGTM
4 years ago (2016-12-03 00:24:23 UTC) #19
Devlin
lgtm
4 years ago (2016-12-05 22:47:48 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2545133002/100001
4 years ago (2016-12-05 22:51:00 UTC) #27
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years ago (2016-12-06 00:23:00 UTC) #30
commit-bot: I haz the power
4 years ago (2016-12-06 00:26:55 UTC) #32
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/2d7f7dc70c5984a92ef96b45e4d4019296112d4d
Cr-Commit-Position: refs/heads/master@{#436471}

Powered by Google App Engine
This is Rietveld 408576698