Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2084953003:
Fix clicks on NTP tiles not contributing to Most Visited tiles (Closed)
|
Created:
4 years, 6 months ago by mastiz Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix clicks on NTP tiles not contributing to Most Visited tiles
The code that computes the client-side list of Most Visited pages,
surfaced in the NTP, considers navigations of types TYPED or
AUTO_BOOKMARK (i.e. excludes LINK): see
https://cs.chromium.org/chromium/src/components/history/core/browser/history_backend.cc?l=339
The goal of this reinforcement is that a click on a tile increases the
likelihood of that page being listed the next time an NTP is opened.
This works fine for Android because clicks on NTP tiles produce
AUTO_BOOKMARK transitions.
However, for desktop, https://codereview.chromium.org/1908363002/ (M53)
caused a regression because it inadvertently changed the transition type
from AUTO_BOOKMARK to LINK (for TopSites tiles only, i.e. computed
locally). This means such clicks don't influence the ranking of tiles,
and therefore TYPED URLs dominate the list.
Note that the issue mostly affects non-signed-in users, since signed-in
users fetch a list from a remote service.
Manually tested:
Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK.
BUG=620296
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/a77db699719e4ecb07ce3a37fe4698ae92372111
Cr-Commit-Position: refs/heads/master@{#403132}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Moved logic to ContentBrowserClient. #
Total comments: 6
Patch Set 3 : Addressed comments. #
Total comments: 10
Patch Set 4 : Addressed nit. #
Total comments: 3
Patch Set 5 : Added TODO. #
Messages
Total messages: 32 (10 generated)
Description was changed from ========== Fix clicks on NTP tiles not contributing to Most Visited tiles The code that computes the client-side list of Most Visited pages, surfaced in the NTP, considers navigations of types TYPED or AUTO_BOOKMARK (i.e. excludes LINK): see https://cs.chromium.org/chromium/src/components/history/core/browser/history_... The goal of this reinforcement is that a click on a tile increases the likelihood of that page being listed the next time an NTP is opened. This works fine for Android because clicks on NTP tiles produce AUTO_BOOKMARK transitions. However, for desktop, https://codereview.chromium.org/1908363002/ (M53) caused a regression because it inadvertently changed the transition type from AUTO_BOOKMARK to LINK. This means such clicks don't influence the ranking of tiles, and therefore TYPED URLs dominate the list. Note that the issue mostly affects non-signed-in users, since signed-in users fetch a list from a remote service. Manually tested: Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK. BUG=620296 ========== to ========== Fix clicks on NTP tiles not contributing to Most Visited tiles The code that computes the client-side list of Most Visited pages, surfaced in the NTP, considers navigations of types TYPED or AUTO_BOOKMARK (i.e. excludes LINK): see https://cs.chromium.org/chromium/src/components/history/core/browser/history_... The goal of this reinforcement is that a click on a tile increases the likelihood of that page being listed the next time an NTP is opened. This works fine for Android because clicks on NTP tiles produce AUTO_BOOKMARK transitions. However, for desktop, https://codereview.chromium.org/1908363002/ (M53) caused a regression because it inadvertently changed the transition type from AUTO_BOOKMARK to LINK. This means such clicks don't influence the ranking of tiles, and therefore TYPED URLs dominate the list. Note that the issue mostly affects non-signed-in users, since signed-in users fetch a list from a remote service. Manually tested: Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK. BUG=620296 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
mastiz@chromium.org changed reviewers: + treib@chromium.org
Note that the desktop regression only affected TopSites tiles - MostLikely tiles always were LINK. https://codereview.chromium.org/2084953003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2084953003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:725: "chrome-search://remote-ntp/" && Oh man, we have too many NTP URLs. I didn't even know about this one. From the top of my head: - chrome://newtab (the "initial" newtab URL). - Unless you're in an incognito or guest profile, it gets rewritten to either - chrome-search://local-ntp/local-ntp.html, or - something like https://www.google.com/_/chrome/newtab (assuming Google is the DSE) - Apparently, at some point that gets rewritten again to this chrome-search://remote-ntp one. - chrome-native://newtab (on Android). There are various "is this an NTP URL" functions around, but I'm not sure any of them will work at this particular point in time. At the least, we should: - Use the existing constants (e.g. https://cs.chromium.org/chromium/src/chrome/common/url_constants.cc?rcl=0&l=742) instead of hardcoding strings. - Not compare full URL strings directly, but GURL.host() and GURL.scheme(). - Also handle the local NTP.
Description was changed from ========== Fix clicks on NTP tiles not contributing to Most Visited tiles The code that computes the client-side list of Most Visited pages, surfaced in the NTP, considers navigations of types TYPED or AUTO_BOOKMARK (i.e. excludes LINK): see https://cs.chromium.org/chromium/src/components/history/core/browser/history_... The goal of this reinforcement is that a click on a tile increases the likelihood of that page being listed the next time an NTP is opened. This works fine for Android because clicks on NTP tiles produce AUTO_BOOKMARK transitions. However, for desktop, https://codereview.chromium.org/1908363002/ (M53) caused a regression because it inadvertently changed the transition type from AUTO_BOOKMARK to LINK. This means such clicks don't influence the ranking of tiles, and therefore TYPED URLs dominate the list. Note that the issue mostly affects non-signed-in users, since signed-in users fetch a list from a remote service. Manually tested: Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK. BUG=620296 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix clicks on NTP tiles not contributing to Most Visited tiles The code that computes the client-side list of Most Visited pages, surfaced in the NTP, considers navigations of types TYPED or AUTO_BOOKMARK (i.e. excludes LINK): see https://cs.chromium.org/chromium/src/components/history/core/browser/history_... The goal of this reinforcement is that a click on a tile increases the likelihood of that page being listed the next time an NTP is opened. This works fine for Android because clicks on NTP tiles produce AUTO_BOOKMARK transitions. However, for desktop, https://codereview.chromium.org/1908363002/ (M53) caused a regression because it inadvertently changed the transition type from AUTO_BOOKMARK to LINK (for TopSites tiles only, i.e. computed locally). This means such clicks don't influence the ranking of tiles, and therefore TYPED URLs dominate the list. Note that the issue mostly affects non-signed-in users, since signed-in users fetch a list from a remote service. Manually tested: Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK. BUG=620296 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
mastiz@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please take a preliminary look. We need this fix in some form to avoid a regression before BP, or alternatively revert a big patch. Can you comment on the overall approach? Do we need to inject this string from higher layers? If yes, would that belong in NavigatorDelegate? Thx.
https://codereview.chromium.org/2084953003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2084953003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:708: // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for Looks like we already used to do this here, back when the NTP was a WebUI page. We can't really make the NTP WebUI, but maybe we can abstract this part to handle both WebUI and something from the ContentBrowserClient? We could have a function that returns an appropriate PageTransition override, for example. (That would let ChromeContentBrowserClient decide if it's an NTP or not, without content/ having to care.) Would we also want clear the referrer and treat it as a browser-initiated navigation?
https://codereview.chromium.org/2084953003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2084953003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:708: // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for On 2016/06/23 22:48:15, Charlie Reis wrote: > Looks like we already used to do this here, back when the NTP was a WebUI page. Yup! Ghosts of NTPs past... > We can't really make the NTP WebUI, but maybe we can abstract this part to > handle both WebUI and something from the ContentBrowserClient? We could have a > function that returns an appropriate PageTransition override, for example. > (That would let ChromeContentBrowserClient decide if it's an NTP or not, without > content/ having to care.) ContentBrowserClient looks like a good place for this logic. There's already "similar" methods there, like GetEffectiveURL, IsHandledURL etc. > Would we also want clear the referrer and treat it as a browser-initiated > navigation? That seems to be what happened before, so...probably? For reference, the old code was: chrome::NavigateParams params(this, url, ui::PAGE_TRANSITION_AUTO_BOOKMARK); params.referrer = content::Referrer(); params.source_contents = source_contents; params.disposition = disposition; params.is_renderer_initiated = false; params.initiating_profile = profile_; chrome::Navigate(¶ms);
On 2016/06/24 10:21:42, Marc Treib wrote: > https://codereview.chromium.org/2084953003/diff/1/content/browser/frame_host/... > File content/browser/frame_host/navigator_impl.cc (right): > > https://codereview.chromium.org/2084953003/diff/1/content/browser/frame_host/... > content/browser/frame_host/navigator_impl.cc:708: // link clicks (e.g., so the > new tab page can specify AUTO_BOOKMARK for > On 2016/06/23 22:48:15, Charlie Reis wrote: > > Looks like we already used to do this here, back when the NTP was a WebUI > page. > > Yup! Ghosts of NTPs past... > > > We can't really make the NTP WebUI, but maybe we can abstract this part to > > handle both WebUI and something from the ContentBrowserClient? We could have > a > > function that returns an appropriate PageTransition override, for example. > > (That would let ChromeContentBrowserClient decide if it's an NTP or not, > without > > content/ having to care.) > > ContentBrowserClient looks like a good place for this logic. There's already > "similar" methods there, like GetEffectiveURL, IsHandledURL etc. > > > Would we also want clear the referrer and treat it as a browser-initiated > > navigation? > > That seems to be what happened before, so...probably? > > For reference, the old code was: > > chrome::NavigateParams params(this, url, > ui::PAGE_TRANSITION_AUTO_BOOKMARK); > params.referrer = content::Referrer(); > params.source_contents = source_contents; > params.disposition = disposition; > params.is_renderer_initiated = false; > params.initiating_profile = profile_; > chrome::Navigate(¶ms); treib@: can you take another look? I didn't clear the referred because I have the impression it was already empty. I'll check again once the UI for this is working (currently returns 500s).
Mostly looks good! https://codereview.chromium.org/2084953003/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1164: chrome::kChromeSearchRemoteNtpHost && Also for the local NTP please! https://codereview.chromium.org/2084953003/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1168: // native implementations like Android's. nit: This isn't quite accurate - it'll also make e.g. navigations via the OneGoogleBar AUTO_BOOKMARK, which isn't consistent with Android because that just doesn't exist on Android. https://codereview.chromium.org/2084953003/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1169: params->transition = ui::PAGE_TRANSITION_AUTO_BOOKMARK; The old code also set renderer_initiated to false, which probably should be the case anyway? Re referrer, I'd either just clear it, or check that it's already empty - WDYT?
PTAL. https://codereview.chromium.org/2084953003/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1164: chrome::kChromeSearchRemoteNtpHost && On 2016/06/27 13:08:15, Marc Treib wrote: > Also for the local NTP please! Done. https://codereview.chromium.org/2084953003/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1168: // native implementations like Android's. On 2016/06/27 13:08:15, Marc Treib wrote: > nit: This isn't quite accurate - it'll also make e.g. navigations via the > OneGoogleBar AUTO_BOOKMARK, which isn't consistent with Android because that > just doesn't exist on Android. Done, PTAL. https://codereview.chromium.org/2084953003/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1169: params->transition = ui::PAGE_TRANSITION_AUTO_BOOKMARK; On 2016/06/27 13:08:15, Marc Treib wrote: > The old code also set renderer_initiated to false, which probably should be the > case anyway? > > Re referrer, I'd either just clear it, or check that it's already empty - WDYT? Done, went conservative and cleared the field out just in case.
LGTM, thanks!
creis@: PTAL, thx.
Looking better, thanks! https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1162: site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme) && There must be an existing way to check whether a tab is showing the NTP. (I don't think we should have subtle checks like this sprinkled around the codebase, in case they have bugs.) At first glance, I found search::IsInstantNTP. Would that or something similar work? https://codereview.chromium.org/2084953003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2084953003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:726: ¶ms); This seems a little unfortunate since we call right back out to the embedder with the params on the line below. I suppose the params immutable in that call, though, so perhaps we need this. https://codereview.chromium.org/2084953003/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2084953003/diff/40001/content/public/browser/... content/public/browser/content_browser_client.h:247: virtual void OverrideOpenURLParams(const SiteInstance* site_instance, nit: No const. http://www.chromium.org/developers/content-module/content-api "don't add the const identifier to interfaces. For interfaces implemented by the embedder, we can't make assumptions about what the embedder needs to implement it. For interfaces implemented by content, the implementation details doesn't have to be exposed."
PTAL. https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1162: site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme) && On 2016/06/28 00:52:05, Charlie Reis wrote: > There must be an existing way to check whether a tab is showing the NTP. (I > don't think we should have subtle checks like this sprinkled around the > codebase, in case they have bugs.) > > At first glance, I found search::IsInstantNTP. Would that or something similar > work? Unfortunately there is no such predicate in C++ code, which is easy to prove by searching for kChromeSearchRemoteNtpHost (only one relevant occurrence, ShouldUseProcessPerSiteForInstantURL, not reusable). Some cleanup is needed in this space but it's also non-trivial since exact requirements for the predicate might differ: in this particular codepath, we don't care about false positives, and it's also the case that URLs get overriden throughout the code. I would rather not address this cleanup now. WDYT? https://codereview.chromium.org/2084953003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2084953003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:726: ¶ms); On 2016/06/28 00:52:05, Charlie Reis wrote: > This seems a little unfortunate since we call right back out to the embedder > with the params on the line below. I suppose the params immutable in that call, > though, so perhaps we need this. We could make a modified copy of params within RequestOpenURL but it seems more convoluted to me: this alternative is rather readable and consistent with the equivalent logic for web-UI. https://codereview.chromium.org/2084953003/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2084953003/diff/40001/content/public/browser/... content/public/browser/content_browser_client.h:247: virtual void OverrideOpenURLParams(const SiteInstance* site_instance, On 2016/06/28 00:52:05, Charlie Reis wrote: > nit: No const. > > http://www.chromium.org/developers/content-module/content-api > "don't add the const identifier to interfaces. For interfaces implemented by the > embedder, we can't make assumptions about what the embedder needs to implement > it. For interfaces implemented by content, the implementation details doesn't > have to be exposed." Done.
https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1162: site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme) && On 2016/06/28 08:20:31, mastiz wrote: > On 2016/06/28 00:52:05, Charlie Reis wrote: > > There must be an existing way to check whether a tab is showing the NTP. (I > > don't think we should have subtle checks like this sprinkled around the > > codebase, in case they have bugs.) > > > > At first glance, I found search::IsInstantNTP. Would that or something > similar > > work? > > Unfortunately there is no such predicate in C++ code, which is easy to prove by > searching for kChromeSearchRemoteNtpHost (only one relevant occurrence, > ShouldUseProcessPerSiteForInstantURL, not reusable). > > Some cleanup is needed in this space but it's also non-trivial since exact > requirements for the predicate might differ: in this particular codepath, we > don't care about false positives, and it's also the case that URLs get overriden > throughout the code. > > I would rather not address this cleanup now. WDYT? Let me add my +1 here. You would indeed think that there's a canonical way to check "is this an NTP", but there isn't, due to how convoluted the whole thing is. E.g.: The initial URL is "chrome://newtab". That gets rewritten to something like "https://www.google.com/_/chrome/newtab", and then at some later point gets rewritten again to "chrome-search://remote-ntp". Then there's the local NTP which is different, the incognito NTP which stays at "chrome://newtab", etc etc. So, this whole mess definitely needs some serious cleanup, but this CL is not the place.
Ok. I'm not thrilled about proceeding as is, but I'm ok with it as long as there's no false positives in the check. Is there a way to test this CL? Seems like we want to prevent the bug from regressing again in the future. https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1162: site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme) && On 2016/06/28 08:57:07, Marc Treib wrote: > On 2016/06/28 08:20:31, mastiz wrote: > > On 2016/06/28 00:52:05, Charlie Reis wrote: > > > There must be an existing way to check whether a tab is showing the NTP. (I > > > don't think we should have subtle checks like this sprinkled around the > > > codebase, in case they have bugs.) > > > > > > At first glance, I found search::IsInstantNTP. Would that or something > > similar > > > work? > > > > Unfortunately there is no such predicate in C++ code, which is easy to prove > by > > searching for kChromeSearchRemoteNtpHost (only one relevant occurrence, > > ShouldUseProcessPerSiteForInstantURL, not reusable). > > > > Some cleanup is needed in this space but it's also non-trivial since exact > > requirements for the predicate might differ: in this particular codepath, we > > don't care about false positives, and it's also the case that URLs get > overriden > > throughout the code. > > > > I would rather not address this cleanup now. WDYT? > > Let me add my +1 here. You would indeed think that there's a canonical way to > check "is this an NTP", but there isn't, due to how convoluted the whole thing > is. E.g.: The initial URL is "chrome://newtab". That gets rewritten to something > like "https://www.google.com/_/chrome/newtab", and then at some later point gets > rewritten again to "chrome-search://remote-ntp". Then there's the local NTP > which is different, the incognito NTP which stays at "chrome://newtab", etc etc. > > So, this whole mess definitely needs some serious cleanup, but this CL is not > the place. Yes, it's unfortunate we don't have something we can use here. That said, the fact that it's so convoluted makes it even more important to have a canonical way to do it, rather than having lots of different places that all get it wrong in some way. Also, I strongly disagree that we don't care about false positives here, because there are security implications to treating a navigation as browser-initiated when it's actually renderer-initiated. That said, if we can be sure that we don't have false positives in this check, I'm ok with proceeding if you file a bug for abstracting this out and leave a TODO referencing the bug number. I would also highly recommend prioritizing that bug soon. https://codereview.chromium.org/2084953003/diff/60001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/60001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1166: chrome::kChromeSearchLocalNtpHost) && Sanity check: The NTP SiteInstance's Site URL is never https://google.com (e.g., when we load https://www.google.com/_/chrome/newtab), right? Hopefully it's kChromeSearchRemoteNtpHost in that case? (Not a big deal; that would be a false negative, which is just a functional bug like the one we're fixing with this CL. I don't see any opportunities for false positives here, but let me know if you do-- that would be a security bug where web pages could do things they aren't allowed to.)
On 2016/06/28 17:57:35, Charlie Reis wrote: > Ok. I'm not thrilled about proceeding as is, but I'm ok with it as long as > there's no false positives in the check. > > Is there a way to test this CL? Seems like we want to prevent the bug from > regressing again in the future. We unfortunately don't have a good test coverage, specially integration tests which would've caught past regressions. If you want me to, I'll add unit-tests at a class-level. However, I don't think they add any value.
https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1162: site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme) && On 2016/06/28 17:57:35, Charlie Reis wrote: > On 2016/06/28 08:57:07, Marc Treib wrote: > > On 2016/06/28 08:20:31, mastiz wrote: > > > On 2016/06/28 00:52:05, Charlie Reis wrote: > > > > There must be an existing way to check whether a tab is showing the NTP. > (I > > > > don't think we should have subtle checks like this sprinkled around the > > > > codebase, in case they have bugs.) > > > > > > > > At first glance, I found search::IsInstantNTP. Would that or something > > > similar > > > > work? > > > > > > Unfortunately there is no such predicate in C++ code, which is easy to prove > > by > > > searching for kChromeSearchRemoteNtpHost (only one relevant occurrence, > > > ShouldUseProcessPerSiteForInstantURL, not reusable). > > > > > > Some cleanup is needed in this space but it's also non-trivial since exact > > > requirements for the predicate might differ: in this particular codepath, we > > > don't care about false positives, and it's also the case that URLs get > > overriden > > > throughout the code. > > > > > > I would rather not address this cleanup now. WDYT? > > > > Let me add my +1 here. You would indeed think that there's a canonical way to > > check "is this an NTP", but there isn't, due to how convoluted the whole thing > > is. E.g.: The initial URL is "chrome://newtab". That gets rewritten to > something > > like "https://www.google.com/_/chrome/newtab", and then at some later point > gets > > rewritten again to "chrome-search://remote-ntp". Then there's the local NTP > > which is different, the incognito NTP which stays at "chrome://newtab", etc > etc. > > > > So, this whole mess definitely needs some serious cleanup, but this CL is not > > the place. > > Yes, it's unfortunate we don't have something we can use here. > > That said, the fact that it's so convoluted makes it even more important to have > a canonical way to do it, rather than having lots of different places that all > get it wrong in some way. Also, I strongly disagree that we don't care about > false positives here, because there are security implications to treating a > navigation as browser-initiated when it's actually renderer-initiated. > > That said, if we can be sure that we don't have false positives in this check, > I'm ok with proceeding if you file a bug for abstracting this out and leave a > TODO referencing the bug number. I would also highly recommend prioritizing > that bug soon. Added a TODO as suggested. Honestly, I don't think this is feasible without a major cleanup much beyond these predicates (i.e. avoid different codepaths for various versions of the NTP). The problem is not that the predicates themselves are convoluted, but all the code around. https://codereview.chromium.org/2084953003/diff/60001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/60001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1166: chrome::kChromeSearchLocalNtpHost) && On 2016/06/28 17:57:35, Charlie Reis wrote: > Sanity check: The NTP SiteInstance's Site URL is never https://google.com (e.g., > when we load https://www.google.com/_/chrome/newtab), right? Hopefully it's > kChromeSearchRemoteNtpHost in that case? > > (Not a big deal; that would be a false negative, which is just a functional bug > like the one we're fixing with this CL. I don't see any opportunities for false > positives here, but let me know if you do-- that would be a security bug where > web pages could do things they aren't allowed to.) I double-checked and for the case above, it is indeed kChromeSearchRemoteNtpHost, so it just works.
On 2016/06/29 14:19:36, mastiz wrote: > On 2016/06/28 17:57:35, Charlie Reis wrote: > > Ok. I'm not thrilled about proceeding as is, but I'm ok with it as long as > > there's no false positives in the check. > > > > Is there a way to test this CL? Seems like we want to prevent the bug from > > regressing again in the future. > > We unfortunately don't have a good test coverage, specially integration tests > which would've caught past regressions. > > If you want me to, I'll add unit-tests at a class-level. However, I don't think > they add any value. That seems like a severe problem. If there aren't tests for the NTP, then this bug (and probably others) are likely to regress again in a future change without anyone noticing. We have a lot of on-going refactoring projects in navigation (e.g., OOPIFs, PlzNavigate), and the only reliable way we can tell when something is broken after a refactor is via tests. I'll give my LGTM for the fix and the changes to content/ (especially since I'm a fan of r395312 and hope it won't get reverted), but I think it's highly important to add tests soon for the NTP behavior to prevent regressions like this. https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1162: site_instance->GetSiteURL().SchemeIs(chrome::kChromeSearchScheme) && On 2016/06/29 14:20:14, mastiz wrote: > On 2016/06/28 17:57:35, Charlie Reis wrote: > > On 2016/06/28 08:57:07, Marc Treib wrote: > > > On 2016/06/28 08:20:31, mastiz wrote: > > > > On 2016/06/28 00:52:05, Charlie Reis wrote: > > > > > There must be an existing way to check whether a tab is showing the NTP. > > > (I > > > > > don't think we should have subtle checks like this sprinkled around the > > > > > codebase, in case they have bugs.) > > > > > > > > > > At first glance, I found search::IsInstantNTP. Would that or something > > > > similar > > > > > work? > > > > > > > > Unfortunately there is no such predicate in C++ code, which is easy to > prove > > > by > > > > searching for kChromeSearchRemoteNtpHost (only one relevant occurrence, > > > > ShouldUseProcessPerSiteForInstantURL, not reusable). > > > > > > > > Some cleanup is needed in this space but it's also non-trivial since exact > > > > requirements for the predicate might differ: in this particular codepath, > we > > > > don't care about false positives, and it's also the case that URLs get > > > overriden > > > > throughout the code. > > > > > > > > I would rather not address this cleanup now. WDYT? > > > > > > Let me add my +1 here. You would indeed think that there's a canonical way > to > > > check "is this an NTP", but there isn't, due to how convoluted the whole > thing > > > is. E.g.: The initial URL is "chrome://newtab". That gets rewritten to > > something > > > like "https://www.google.com/_/chrome/newtab", and then at some later point > > gets > > > rewritten again to "chrome-search://remote-ntp". Then there's the local NTP > > > which is different, the incognito NTP which stays at "chrome://newtab", etc > > etc. > > > > > > So, this whole mess definitely needs some serious cleanup, but this CL is > not > > > the place. > > > > Yes, it's unfortunate we don't have something we can use here. > > > > That said, the fact that it's so convoluted makes it even more important to > have > > a canonical way to do it, rather than having lots of different places that all > > get it wrong in some way. Also, I strongly disagree that we don't care about > > false positives here, because there are security implications to treating a > > navigation as browser-initiated when it's actually renderer-initiated. > > > > That said, if we can be sure that we don't have false positives in this check, > > I'm ok with proceeding if you file a bug for abstracting this out and leave a > > TODO referencing the bug number. I would also highly recommend prioritizing > > that bug soon. > > Added a TODO as suggested. > > Honestly, I don't think this is feasible without a major cleanup much beyond > these predicates (i.e. avoid different codepaths for various versions of the > NTP). The problem is not that the predicates themselves are convoluted, but all > the code around. I posted a comment to the bug, since I think this is worth solving to prevent future problems. https://codereview.chromium.org/2084953003/diff/60001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2084953003/diff/60001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1166: chrome::kChromeSearchLocalNtpHost) && On 2016/06/29 14:20:14, mastiz wrote: > On 2016/06/28 17:57:35, Charlie Reis wrote: > > Sanity check: The NTP SiteInstance's Site URL is never https://google.com > (e.g., > > when we load https://www.google.com/_/chrome/newtab), right? Hopefully it's > > kChromeSearchRemoteNtpHost in that case? > > > > (Not a big deal; that would be a false negative, which is just a functional > bug > > like the one we're fixing with this CL. I don't see any opportunities for > false > > positives here, but let me know if you do-- that would be a security bug where > > web pages could do things they aren't allowed to.) > > I double-checked and for the case above, it is indeed > kChromeSearchRemoteNtpHost, so it just works. Thanks for checking.
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2084953003/#ps80001 (title: "Added TODO.")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix clicks on NTP tiles not contributing to Most Visited tiles The code that computes the client-side list of Most Visited pages, surfaced in the NTP, considers navigations of types TYPED or AUTO_BOOKMARK (i.e. excludes LINK): see https://cs.chromium.org/chromium/src/components/history/core/browser/history_... The goal of this reinforcement is that a click on a tile increases the likelihood of that page being listed the next time an NTP is opened. This works fine for Android because clicks on NTP tiles produce AUTO_BOOKMARK transitions. However, for desktop, https://codereview.chromium.org/1908363002/ (M53) caused a regression because it inadvertently changed the transition type from AUTO_BOOKMARK to LINK (for TopSites tiles only, i.e. computed locally). This means such clicks don't influence the ranking of tiles, and therefore TYPED URLs dominate the list. Note that the issue mostly affects non-signed-in users, since signed-in users fetch a list from a remote service. Manually tested: Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK. BUG=620296 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix clicks on NTP tiles not contributing to Most Visited tiles The code that computes the client-side list of Most Visited pages, surfaced in the NTP, considers navigations of types TYPED or AUTO_BOOKMARK (i.e. excludes LINK): see https://cs.chromium.org/chromium/src/components/history/core/browser/history_... The goal of this reinforcement is that a click on a tile increases the likelihood of that page being listed the next time an NTP is opened. This works fine for Android because clicks on NTP tiles produce AUTO_BOOKMARK transitions. However, for desktop, https://codereview.chromium.org/1908363002/ (M53) caused a regression because it inadvertently changed the transition type from AUTO_BOOKMARK to LINK (for TopSites tiles only, i.e. computed locally). This means such clicks don't influence the ranking of tiles, and therefore TYPED URLs dominate the list. Note that the issue mostly affects non-signed-in users, since signed-in users fetch a list from a remote service. Manually tested: Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK. BUG=620296 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix clicks on NTP tiles not contributing to Most Visited tiles The code that computes the client-side list of Most Visited pages, surfaced in the NTP, considers navigations of types TYPED or AUTO_BOOKMARK (i.e. excludes LINK): see https://cs.chromium.org/chromium/src/components/history/core/browser/history_... The goal of this reinforcement is that a click on a tile increases the likelihood of that page being listed the next time an NTP is opened. This works fine for Android because clicks on NTP tiles produce AUTO_BOOKMARK transitions. However, for desktop, https://codereview.chromium.org/1908363002/ (M53) caused a regression because it inadvertently changed the transition type from AUTO_BOOKMARK to LINK (for TopSites tiles only, i.e. computed locally). This means such clicks don't influence the ranking of tiles, and therefore TYPED URLs dominate the list. Note that the issue mostly affects non-signed-in users, since signed-in users fetch a list from a remote service. Manually tested: Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK. BUG=620296 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix clicks on NTP tiles not contributing to Most Visited tiles The code that computes the client-side list of Most Visited pages, surfaced in the NTP, considers navigations of types TYPED or AUTO_BOOKMARK (i.e. excludes LINK): see https://cs.chromium.org/chromium/src/components/history/core/browser/history_... The goal of this reinforcement is that a click on a tile increases the likelihood of that page being listed the next time an NTP is opened. This works fine for Android because clicks on NTP tiles produce AUTO_BOOKMARK transitions. However, for desktop, https://codereview.chromium.org/1908363002/ (M53) caused a regression because it inadvertently changed the transition type from AUTO_BOOKMARK to LINK (for TopSites tiles only, i.e. computed locally). This means such clicks don't influence the ranking of tiles, and therefore TYPED URLs dominate the list. Note that the issue mostly affects non-signed-in users, since signed-in users fetch a list from a remote service. Manually tested: Google remote NTP - transition type changed from LINK to AUTO_BOOKMARK. BUG=620296 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/a77db699719e4ecb07ce3a37fe4698ae92372111 Cr-Commit-Position: refs/heads/master@{#403132} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a77db699719e4ecb07ce3a37fe4698ae92372111 Cr-Commit-Position: refs/heads/master@{#403132} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
