Clean up predicates that decide whether a URL is an NTP or not |
||||||||
Issue descriptionAs per discussion in https://codereview.chromium.org/2084953003/. There's various places in the Chromium codebase were it's verified whether a URL refers to the NTP. Unfortunately, this logic is complex because multiple variants of the NTP exist, with different URLs, and it also differs across different parts of the codebase, since NTP URLs get overwritten. This mess has been a source of bugs in the past and hence some cleanup would be beneficial. It's however not clear to me if it's possible without a major rewrite of the code (possibly by removing some existing variants of the NTP).
,
Jun 29 2016
Specifically, I was concerned about adding logic like this to ChromeContentBrowserClient::OverrideOpenURLParams: 1177 if (site_instance && 1178 site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme) && 1179 (site_instance->GetSiteURL().host() == 1180 chrome::kChromeSearchRemoteNtpHost || 1181 site_instance->GetSiteURL().host() == 1182 chrome::kChromeSearchLocalNtpHost) Call sites like this have to know not just that there's both a remote and a local NTP, but also that the SiteInstance's site URL is the thing to check against (since the committed URLs will sometimes look like a web URL instead). This is unusual, because the SiteInstance::GetSiteURL is often the wrong thing to look at. It's also surprising to me that search::IsInstantNTP and search::GetNewTabPageURL (which it partly depends on) are not sufficient for detecting NTPs, which is quite confusing. This all came up in https://codereview.chromium.org/2084953003/ because the switch to make the NTP no longer a WebUI page broke how link clicks on the NTP are classified ( issue 620296 ). That indicates that some code was relying on yet another approach for detecting the NTP (i.e., a WebUI object). I'd recommend giving this a higher priority to prevent even more bugs here.
,
Jul 1 2016
,
Jul 1 2016
,
Jul 11 2016
There's also search::IsNTPURL, which doesn't do what you'd think - it also considers the "Instant URL" (e.g. https://www.google.com/webhp?espv=2) an NTP URL, which IMO is quite unexpected.
,
Jul 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/387415b57a38770553fdce23a538cb069fa94ca9 commit 387415b57a38770553fdce23a538cb069fa94ca9 Author: treib <treib@chromium.org> Date: Mon Jul 11 12:35:36 2016 Introduce search::IsInstantNTPURL and clean up the comments for IsNTPURL and IsInstantNTP to match what the functions actually do. BUG=624410 Review-Url: https://codereview.chromium.org/2134133002 Cr-Commit-Position: refs/heads/master@{#404641} [modify] https://crrev.com/387415b57a38770553fdce23a538cb069fa94ca9/chrome/browser/search/search.cc [modify] https://crrev.com/387415b57a38770553fdce23a538cb069fa94ca9/chrome/browser/search/search.h
,
Nov 10 2017
,
Dec 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb3d859ad53f667a8cca5cb5d18ed09b13c51ef8 commit cb3d859ad53f667a8cca5cb5d18ed09b13c51ef8 Author: Marc Treib <treib@chromium.org> Date: Mon Dec 04 16:25:08 2017 search::IsInstantNTP: Prefer committed over visible URL The visible URL doesn't always correspond to the actual page that's loaded. For example, if the user types in a URL that ends up being a download, then the visible URL corresponds to the download, but the NTP is still there and should remain functional. Bug: 592273 , 624410 Change-Id: Ib2e0a7df8658e55030ee5e67959a0ebc486e495d Reviewed-on: https://chromium-review.googlesource.com/800931 Reviewed-by: Chris Pickel <sfiera@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#521359} [modify] https://crrev.com/cb3d859ad53f667a8cca5cb5d18ed09b13c51ef8/chrome/browser/search/search.cc [modify] https://crrev.com/cb3d859ad53f667a8cca5cb5d18ed09b13c51ef8/chrome/browser/search/search.h [modify] https://crrev.com/cb3d859ad53f667a8cca5cb5d18ed09b13c51ef8/chrome/browser/search/search_unittest.cc [modify] https://crrev.com/cb3d859ad53f667a8cca5cb5d18ed09b13c51ef8/chrome/browser/ui/search/local_ntp_browsertest.cc [modify] https://crrev.com/cb3d859ad53f667a8cca5cb5d18ed09b13c51ef8/chrome/browser/ui/search/search_ipc_router_policy_unittest.cc
,
Jan 11 2018
,
Jan 11 2018
,
Sep 16
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by mastiz@chromium.org
, Jun 29 2016