Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760463005:
Fix handling of external protocols with PlzNavigate. (Closed)
DescriptionFix handling of external protocols with PlzNavigate.
This fixes
UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout
ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork
externalnav.UrlOverridingTest#testNavigationFromTimer
Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures.
BUG=699387
Review-Url: https://codereview.chromium.org/2760463005
Cr-Commit-Position: refs/heads/master@{#459285}
Committed: https://chromium.googlesource.com/chromium/src/+/cc2df16e35d1016a66a1b457b4070cc50a54e045
Patch Set 1 : fix #Patch Set 2 : simpler fix #Patch Set 3 : fix #Patch Set 4 : without PlzNavigate #Patch Set 5 : fix #Patch Set 6 : clean fix with PlzNavigate #Patch Set 7 : without PlzNavigate #
Total comments: 6
Patch Set 8 : review comments #
Total comments: 2
Messages
Total messages: 69 (59 generated)
Description was changed from ========== Fix handling of external protocls with PlzNavigate BUG= ========== to ========== Fix handling of external protocls with PlzNavigate BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jam@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 checked by jam@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...
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:10003) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Fix handling of external protocls with PlzNavigate BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix handling of external protocols with PlzNavigate BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jam@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 jam@chromium.org
Patchset #2 (id:70001) has been deleted
The CQ bit was checked by jam@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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Description was changed from ========== Fix handling of external protocols with PlzNavigate BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix handling of external protocols with PlzNavigate BUG= ==========
Description was changed from ========== Fix handling of external protocols with PlzNavigate BUG= ========== to ========== Fix handling of external protocols with PlzNavigate BUG=659089 ==========
Description was changed from ========== Fix handling of external protocols with PlzNavigate BUG=659089 ========== to ========== Fix handling of external protocols with PlzNavigate BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix handling of external protocols with PlzNavigate BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix handling of external protocols with PlzNavigate BUG=659089 ==========
The CQ bit was checked by jam@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...
Description was changed from ========== Fix handling of external protocols with PlzNavigate BUG=659089 ========== to ========== Fix handling of external protocols with PlzNavigate BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jam@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: This issue passed the CQ dry run.
jam@chromium.org changed reviewers: + wychen@chromium.org
Thanks for tracking down the difference in handled protocol logic :) I think I see what's causing UrlOverridingTest#testNavigationWithFallbackURL, but it's different than this sho it should be fixed separately to unblock these two tests.
On 2017/03/23 07:00:26, jam wrote: > Thanks for tracking down the difference in handled protocol logic :) > > I think I see what's causing UrlOverridingTest#testNavigationWithFallbackURL, > but it's different than this sho it should be fixed separately to unblock these > two tests. actually, hold off on this review. I need to understand this better.
The CQ bit was checked by jam@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Fix handling of external protocols with PlzNavigate BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes org.chromium.chrome.browser.externalnav.UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout org.chromium.chrome.browser.toolbar.ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork org.chromium.chrome.browser.externalnav.UrlOverridingTest#testNavigationFromTimer BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix handling of external protocols with PlzNavigate. This fixes org.chromium.chrome.browser.externalnav.UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout org.chromium.chrome.browser.toolbar.ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork org.chromium.chrome.browser.externalnav.UrlOverridingTest#testNavigationFromTimer BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jam@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 checked by jam@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer BUG=659089 ==========
Description was changed from ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer BUG=659089 ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 ==========
jam@chromium.org changed reviewers: + brettw@chromium.org, dcheng@chromium.org, thakis@chromium.org - wychen@chromium.org
Nico: WebKit/Source/platform Daniel: rest of WebKit Brett: outside of WebKit
blink lgtm https://codereview.chromium.org/2760463005/diff/190001/content/common/navigat... File content/common/navigation_params.cc (right): https://codereview.chromium.org/2760463005/diff/190001/content/common/navigat... content/common/navigation_params.cc:33: break; Nit: maybe slightly shorter to just return false directly? https://codereview.chromium.org/2760463005/diff/190001/content/public/common/... File content/public/common/content_client.h (right): https://codereview.chromium.org/2760463005/diff/190001/content/public/common/... content/public/common/content_client.h:114: // Registers a URL scheme to be treated as a noAccess scheme. This means Nit: indent https://codereview.chromium.org/2760463005/diff/190001/content/public/common/... content/public/common/content_client.h:116: // any other URL scheme. Nit: probably more accurate to say that pages with this URL scheme always have an opaque origin. (the spec used to call these unique origins, but the terminology changed)
Description was changed from ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jam@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...
https://codereview.chromium.org/2760463005/diff/190001/content/common/navigat... File content/common/navigation_params.cc (right): https://codereview.chromium.org/2760463005/diff/190001/content/common/navigat... content/common/navigation_params.cc:33: break; On 2017/03/23 21:07:15, dcheng wrote: > Nit: maybe slightly shorter to just return false directly? rewrote this to make it simpler https://codereview.chromium.org/2760463005/diff/190001/content/public/common/... File content/public/common/content_client.h (right): https://codereview.chromium.org/2760463005/diff/190001/content/public/common/... content/public/common/content_client.h:114: // Registers a URL scheme to be treated as a noAccess scheme. This means On 2017/03/23 21:07:15, dcheng wrote: > Nit: indent Done. https://codereview.chromium.org/2760463005/diff/190001/content/public/common/... content/public/common/content_client.h:116: // any other URL scheme. On 2017/03/23 21:07:15, dcheng wrote: > Nit: probably more accurate to say that pages with this URL scheme always have > an opaque origin. > > (the spec used to call these unique origins, but the terminology changed) Done.
Description was changed from ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 ==========
//url LGTM https://codereview.chromium.org/2760463005/diff/210001/url/url_util.cc File url/url_util.cc (right): https://codereview.chromium.org/2760463005/diff/210001/url/url_util.cc#newcode85 url/url_util.cc:85: kAboutScheme, Indenting.
kbr@chromium.org changed reviewers: + kbr@chromium.org
lgtm
https://codereview.chromium.org/2760463005/diff/210001/url/url_util.cc File url/url_util.cc (right): https://codereview.chromium.org/2760463005/diff/210001/url/url_util.cc#newcode85 url/url_util.cc:85: kAboutScheme, On 2017/03/23 22:02:13, brettw (plz ping after 24h) wrote: > Indenting. I'll do this in a followup if you don't mind, just because of CQ time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2760463005/#ps210001 (title: "review comments")
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": 210001, "attempt_start_ts": 1490312757264620,
"parent_rev": "fcf25e5abf9f679d8717049e2d48f6d38bdd24ae", "commit_rev":
"cc2df16e35d1016a66a1b457b4070cc50a54e045"}
Description was changed from ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 ==========
Message was sent while issue was closed.
Description was changed from ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 Review-Url: https://codereview.chromium.org/2760463005 Cr-Commit-Position: refs/heads/master@{#459285} Committed: https://chromium.googlesource.com/chromium/src/+/cc2df16e35d1016a66a1b457b407... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:210001) as https://chromium.googlesource.com/chromium/src/+/cc2df16e35d1016a66a1b457b407...
Message was sent while issue was closed.
Description was changed from ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=659089 Review-Url: https://codereview.chromium.org/2760463005 Cr-Commit-Position: refs/heads/master@{#459285} Committed: https://chromium.googlesource.com/chromium/src/+/cc2df16e35d1016a66a1b457b407... ========== to ========== Fix handling of external protocols with PlzNavigate. This fixes UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout ToolbarTest#testNTPNavigatesToErrorPageOnDisconnectedNetwork externalnav.UrlOverridingTest#testNavigationFromTimer Once is_external_protocol was set correctly, the second bug that happened was because with PlzNavigate we were making requests for chrome-native schemes. This caused NavigationHandleImpl::WillStartRequest to be called, which added navigation throttles and that ended up calling the Android navigation interceptor. It's not called without PlzNavigate, which led to these tests failures. BUG=699387 Review-Url: https://codereview.chromium.org/2760463005 Cr-Commit-Position: refs/heads/master@{#459285} Committed: https://chromium.googlesource.com/chromium/src/+/cc2df16e35d1016a66a1b457b407... ========== |
