Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2545133002:
PlzNavigate: Fix ordering of onBeforeNavigate and onCreatedNavigationTarget. (Closed)
DescriptionPlzNavigate: 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. #
Messages
Total messages: 32 (17 generated)
nasko@chromium.org changed reviewers: + alexmos@chromium.org, rdevlin.cronin@chromium.org
Hey Alex and Devlin, Can you review this CL for me? It changes webNavigation API implementation to cache locally the onBeforeNavigate event and dispatch it after the tab has been added to the tabstrip. It fixes all tests to now pass with PlzNavigate. Thanks in advance! Nasko
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I'm a little bit worried about this because it seems like we're manipulating the API to preserve ordering even though that ordering isn't necessarily accurate. Isn't the navigation request created before the navigation target in this case? What's wrong with dispatching them as we see them? Developers already sometimes have to deal with stuff like this (e.g. prerendering tabs), and I'm worried that artificially delaying events like this will get us into trouble.
On 2016/12/02 19:30:37, Devlin wrote: > I'm a little bit worried about this because it seems like we're manipulating the > API to preserve ordering even though that ordering isn't necessarily accurate. Why isn't the ordering accurate? IMHO this is just a Chrome implementation detail that sucks and I'd love to fix, but have failed due to the complex interactions between different parts. In reality, we MUST have created a WebContents/tab in order to navigate it. The fact that we delay the tab addition to after navigating it is an implementation detail. Also, we could fix it too, if extensions didn't rely on tabs being in the tabstrip, but that is another can of worms. > Isn't the navigation request created before the navigation target in this case? > What's wrong with dispatching them as we see them? Does it make sense to see a onBeforeNavigate for a tab you haven't heard exist? > Developers already sometimes have to deal with stuff like this (e.g. > prerendering tabs), and I'm worried that artificially delaying events like this > will get us into trouble. Why do we have to make life complex on extension devs? Also, I expect that we could break existing extensions which expect the onCreatedNavigationTarget to always occur before onBeforeNavigate. This is one case where I think not breaking existing behavior is actually desirable : ).
On 2016/12/02 19:50:08, nasko wrote: > On 2016/12/02 19:30:37, Devlin wrote: > > I'm a little bit worried about this because it seems like we're manipulating > the > > API to preserve ordering even though that ordering isn't necessarily accurate. > > > Why isn't the ordering accurate? IMHO this is just a Chrome implementation > detail that sucks and I'd love to fix, but have failed due to the complex > interactions between different parts. In reality, we MUST have created a > WebContents/tab in order to navigate it. The fact that we delay the tab addition > to after navigating it is an implementation detail. > > Also, we could fix it too, if extensions didn't rely on tabs being in the > tabstrip, but that is another can of worms. > > > Isn't the navigation request created before the navigation target in this > case? > > What's wrong with dispatching them as we see them? > > Does it make sense to see a onBeforeNavigate for a tab you haven't heard exist? > > > Developers already sometimes have to deal with stuff like this (e.g. > > prerendering tabs), and I'm worried that artificially delaying events like > this > > will get us into trouble. > > Why do we have to make life complex on extension devs? Also, I expect that we > could break existing extensions which expect the onCreatedNavigationTarget to > always occur before onBeforeNavigate. This is one case where I think not > breaking existing behavior is actually desirable : ). Talked offline - I was mixing complexities with the RFH not existing, not the tab not existing. That alleviates my concerns. :) On with the review!
https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:280: if (ExtensionTabUtil::GetTabById( I don't love that this is how we check this. It makes me worried if we ever - Include unattached tabs in GetTabById() - Are correctly able to dispatch the oncreated event before the tab is added to the tab strip that this will behave incorrectly. But since we use this logic elsewhere, it would have to be a bulk update. Maybe add a TODO to some effect? WDYT? https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:282: Profile::FromBrowserContext(web_contents()->GetBrowserContext()), nit: this can take a browser context, so no need for Profile casting. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:283: false, NULL, NULL, NULL, NULL)) { nit: nullptr in new code. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:294: DispatchOnBeforeNavigate(); Does this not have the same possible race? Or why wouldn't we have dispatched this once we dispatched the target created event? https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:417: EventRouter* event_router = EventRouter::Get(profile); nit: use browser context directly. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:418: if (profile && event_router) can profile/context be null? (Or event router, for that matter?) If it can, I think we'll crash on EventRouter::Get(). https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.h:75: void DispatchOnBeforeNavigate(); nit: Can we rename this to be DispatchPendingOnBeforeNavigate(), DispatchCached..., Dispatch...IfPending, etc, to make it clear from call sites that this conditionally does it only if we have one stored, rather than unconditionally sending a new event? https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:92: EventRouter* event_router = EventRouter::Get(profile); Why do we need event router?
https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:280: if (ExtensionTabUtil::GetTabById( On 2016/12/02 20:17:11, Devlin wrote: > I don't love that this is how we check this. It makes me worried if we ever > - Include unattached tabs in GetTabById() I think there will be more fallout if we did that than just this instance :). > - Are correctly able to dispatch the oncreated event before the tab is added to > the tab strip If we get to that point, I'll be happy to fix this. The reason I'm using it is so it is consistent with how this code behaves in other places in the file. It will be easier to grep/fix later if it is consistently using the same pattern than not. > that this will behave incorrectly. But since we use this logic elsewhere, it > would have to be a bulk update. Maybe add a TODO to some effect? WDYT? Indeed. Bulk update will be nice. I'm fine adding a TODO, but not sure what the action should be. Just adding it without explicit and clear action doesn't seem very useful to me. Open to ideas if you have any. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:282: Profile::FromBrowserContext(web_contents()->GetBrowserContext()), On 2016/12/02 20:17:11, Devlin wrote: > nit: this can take a browser context, so no need for Profile casting. Done. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:283: false, NULL, NULL, NULL, NULL)) { On 2016/12/02 20:17:11, Devlin wrote: > nit: nullptr in new code. Done. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:294: DispatchOnBeforeNavigate(); On 2016/12/02 20:17:11, Devlin wrote: > Does this not have the same possible race? Or why wouldn't we have dispatched > this once we dispatched the target created event? This guarantees that if we are going to send a commit, we must have sent the onBeforeNavigate. The TargetBlankIncognito test ends up properly dispatching the onCreatedNavigationTarget, but the onBeforeNavigate wasn't being send, since it arrived after the TabAdded notification. So this is a safeguard to ensure that if there is anything cached that we missed sending, we will ensure it is sent before the commit. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:417: EventRouter* event_router = EventRouter::Get(profile); On 2016/12/02 20:17:11, Devlin wrote: > nit: use browser context directly. Done. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:418: if (profile && event_router) On 2016/12/02 20:17:11, Devlin wrote: > can profile/context be null? (Or event router, for that matter?) If it can, I > think we'll crash on EventRouter::Get(). I don't know whether the context can be null, possibly in unit tests? Looking at other usage, it looks like lots of places use the returned pointer directly, so doesn't look like it should ever be null. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_navigation/web_navigation_api.h (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.h:75: void DispatchOnBeforeNavigate(); On 2016/12/02 20:17:11, Devlin wrote: > nit: Can we rename this to be DispatchPendingOnBeforeNavigate(), > DispatchCached..., Dispatch...IfPending, etc, to make it clear from call sites > that this conditionally does it only if we have one stored, rather than > unconditionally sending a new event? Acknowledged. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:92: EventRouter* event_router = EventRouter::Get(profile); On 2016/12/02 20:17:11, Devlin wrote: > Why do we need event router? Because of copy/paste :).
Description was changed from ========== 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 ========== to ========== 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 ==========
https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:280: if (ExtensionTabUtil::GetTabById( On 2016/12/02 21:40:45, nasko wrote: > On 2016/12/02 20:17:11, Devlin wrote: > > I don't love that this is how we check this. It makes me worried if we ever > > - Include unattached tabs in GetTabById() > > I think there will be more fallout if we did that than just this instance :). > > > - Are correctly able to dispatch the oncreated event before the tab is added > to > > the tab strip > > If we get to that point, I'll be happy to fix this. The reason I'm using it is > so it is consistent with how this code behaves in other places in the file. It > will be easier to grep/fix later if it is consistently using the same pattern > than not. > > > that this will behave incorrectly. But since we use this logic elsewhere, it > > would have to be a bulk update. Maybe add a TODO to some effect? WDYT? > > Indeed. Bulk update will be nice. I'm fine adding a TODO, but not sure what the > action should be. Just adding it without explicit and clear action doesn't seem > very useful to me. Open to ideas if you have any. // TODO(nasko|devlin): We have to do this check because chrome::Navigate() begins the navigation before the sending the TabAdded notification, and we use this as an indication of that. It would be best if we instead knew when the tab was created and immediately sent the created event instead of waiting for the later TabAdded notification, but this appears to work for now. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:294: DispatchOnBeforeNavigate(); On 2016/12/02 21:40:45, nasko wrote: > On 2016/12/02 20:17:11, Devlin wrote: > > Does this not have the same possible race? Or why wouldn't we have dispatched > > this once we dispatched the target created event? > > This guarantees that if we are going to send a commit, we must have sent the > onBeforeNavigate. The TargetBlankIncognito test ends up properly dispatching the > onCreatedNavigationTarget, but the onBeforeNavigate wasn't being send, since it > arrived after the TabAdded notification. So this is a safeguard to ensure that > if there is anything cached that we missed sending, we will ensure it is sent > before the commit. If the TabAdded notification is sent before DidStartNavigation, then we *should* send the event immediately on line 284 (of this patch set). If the if above (line 280) is returning false for the incognito test, it's probably because we pass "false" for "include_incognito" to GetTabById. We should be able to flip that to true, since we know there will only be a single tab with that id and don't care if it's incognito or not. https://codereview.chromium.org/2545133002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc (right): https://codereview.chromium.org/2545133002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:93: event->restrict_to_browser_context = profile; Just use browser context directly.
https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:280: if (ExtensionTabUtil::GetTabById( On 2016/12/02 21:58:31, Devlin wrote: > On 2016/12/02 21:40:45, nasko wrote: > > On 2016/12/02 20:17:11, Devlin wrote: > > > I don't love that this is how we check this. It makes me worried if we ever > > > - Include unattached tabs in GetTabById() > > > > I think there will be more fallout if we did that than just this instance :). > > > > > - Are correctly able to dispatch the oncreated event before the tab is added > > to > > > the tab strip > > > > If we get to that point, I'll be happy to fix this. The reason I'm using it is > > so it is consistent with how this code behaves in other places in the file. It > > will be easier to grep/fix later if it is consistently using the same pattern > > than not. > > > > > that this will behave incorrectly. But since we use this logic elsewhere, > it > > > would have to be a bulk update. Maybe add a TODO to some effect? WDYT? > > > > Indeed. Bulk update will be nice. I'm fine adding a TODO, but not sure what > the > > action should be. Just adding it without explicit and clear action doesn't > seem > > very useful to me. Open to ideas if you have any. > > // TODO(nasko|devlin): We have to do this check because chrome::Navigate() > begins the navigation before the sending the TabAdded notification, and we use > this as an indication of that. It would be best if we instead knew when the tab > was created and immediately sent the created event instead of waiting for the > later TabAdded notification, but this appears to work for now. Done. https://codereview.chromium.org/2545133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:294: DispatchOnBeforeNavigate(); On 2016/12/02 21:58:31, Devlin wrote: > On 2016/12/02 21:40:45, nasko wrote: > > On 2016/12/02 20:17:11, Devlin wrote: > > > Does this not have the same possible race? Or why wouldn't we have > dispatched > > > this once we dispatched the target created event? > > > > This guarantees that if we are going to send a commit, we must have sent the > > onBeforeNavigate. The TargetBlankIncognito test ends up properly dispatching > the > > onCreatedNavigationTarget, but the onBeforeNavigate wasn't being send, since > it > > arrived after the TabAdded notification. So this is a safeguard to ensure that > > if there is anything cached that we missed sending, we will ensure it is sent > > before the commit. > > If the TabAdded notification is sent before DidStartNavigation, then we *should* > send the event immediately on line 284 (of this patch set). If the if above > (line 280) is returning false for the incognito test, it's probably because we > pass "false" for "include_incognito" to GetTabById. We should be able to flip > that to true, since we know there will only be a single tab with that id and > don't care if it's incognito or not. The BrowserContext passed in should already be the incognito one, so the tab should be found just fine. It turns out that this happens for the very first tab in the incognito window, not for the latter. Unfortunately, if I move the dispatch to be part of the TabAdded handling, we are back to broken state. I know this is not ideal, but it might be the best we can do without fixing chrome::Navigate (which I already spent at least 2-3 weeks trying to disentangle). https://codereview.chromium.org/2545133002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc (right): https://codereview.chromium.org/2545133002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:93: event->restrict_to_browser_context = profile; On 2016/12/02 21:58:31, Devlin wrote: > Just use browser context directly. Done.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This looks fine, just a quick question - is this covered by any tests that currently fail in PlzNavigate mode? If so, do you need to update the filter file, or if not, does this need a new test? https://codereview.chromium.org/2545133002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:418: if (!pending_on_before_navigate_event_.get()) nit: .get() is not needed
> This looks fine, just a quick question - is this covered by any tests that > currently fail in PlzNavigate mode? If so, do you need to update the filter > file, or if not, does this need a new test? Good point. I've removed the fixed tests from the filter. https://codereview.chromium.org/2545133002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2545133002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:418: if (!pending_on_before_navigate_event_.get()) On 2016/12/03 00:03:46, alexmos wrote: > nit: .get() is not needed Done.
LGTM
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2545133002/#ps100001 (title: "Rebase on ToT.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480978220603870,
"parent_rev": "68413380642ff097a399a40e996f2a72722d7aba", "commit_rev":
"b86d362b4c167d2bdce8eff5ff2fb502ad7bbc85"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2d7f7dc70c5984a92ef96b45e4d4019296112d4d Cr-Commit-Position: refs/heads/master@{#436471} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
