Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2765213003:
PlzNavigate: treat intent:// as external scheme (Closed)
DescriptionPlzNavigate: treat intent:// as external scheme
This fixes the following tests, which fails with PlzNavigate enabled:
UrlOverridingTest#testNavigationFromTimer
UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout
BUG=699388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #
Depends on Patchset: Messages
Total messages: 17 (12 generated)
Description was changed from ========== Treat intent:// as external scheme This fixes the following tests, which fails with PlzNavigate enabled: UrlOverridingTest#testNavigationFromTimer UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout BUG=699388 ========== to ========== Treat intent:// as external scheme This fixes the following tests, which fails with PlzNavigate enabled: UrlOverridingTest#testNavigationFromTimer UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout BUG=699388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by wychen@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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by wychen@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...)
wychen@chromium.org changed reviewers: + clamy@chromium.org, jam@chromium.org
PTAL. The dependency on PlzNavigate-enabling CL is only for trybots.
Description was changed from ========== Treat intent:// as external scheme This fixes the following tests, which fails with PlzNavigate enabled: UrlOverridingTest#testNavigationFromTimer UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout BUG=699388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Treat intent:// as external scheme This fixes the following tests, which fails with PlzNavigate enabled: UrlOverridingTest#testNavigationFromTimer UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout BUG=699388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: Treat intent:// as external scheme This fixes the following tests, which fails with PlzNavigate enabled: UrlOverridingTest#testNavigationFromTimer UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout BUG=699388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: treat intent:// as external scheme This fixes the following tests, which fails with PlzNavigate enabled: UrlOverridingTest#testNavigationFromTimer UrlOverridingTest#testNavigationFromXHRCallbackAndLongTimeout BUG=699388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Thanks for tracking this down! Per chat, I think we need to properly fix is_external_protocol calculation so that plznavigate and non-plznavigate codepaths match. that means calling GetContentClient()->browser()->IsHandledURL() and ensuring it returns the same value as net::URLRequestJobFactoryImpl::IsHandledURL()
On 2017/03/22 23:24:33, jam wrote: > Thanks for tracking this down! > > Per chat, I think we need to properly fix is_external_protocol calculation so > that plznavigate and non-plznavigate codepaths match. that means calling > GetContentClient()->browser()->IsHandledURL() and ensuring it returns the same > value as net::URLRequestJobFactoryImpl::IsHandledURL() Right now, the failure only happens on Android because NavigationHandle::IsExternal is called just by the interception code which goes to shouldOverrideUrl
On 2017/03/22 23:25:24, jam wrote: > On 2017/03/22 23:24:33, jam wrote: > > Thanks for tracking this down! > > > > Per chat, I think we need to properly fix is_external_protocol calculation so > > that plznavigate and non-plznavigate codepaths match. that means calling > > GetContentClient()->browser()->IsHandledURL() and ensuring it returns the same > > value as net::URLRequestJobFactoryImpl::IsHandledURL() > > Right now, the failure only happens on Android because > NavigationHandle::IsExternal is called just by the interception code which goes > to shouldOverrideUrl You are right. We need to list supported scheme instead of listing external schemes. I just tried tel: and mailto:, and they have similar issues with intent://.
Message was sent while issue was closed.
Fixed by https://codereview.chromium.org/2760463005/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
