Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1787633002:
Use whitelist large icon for corresponding most visited suggestions. (Closed)
DescriptionUse whitelist large icon for corresponding most visited suggestions.
If we get a most visited suggestion that is also a whitelist entry
point, use the icon from whitelist when creating he suggestions. This
is done in order to have consistently good large icon image on the NTP.
The reasoning for this comes from the fact that a lot of websites do
not have a large icon which would mean that while the user is browsing
and a site transitions from an entry point to a most visited suggestion
we will lose the icon image and will replace it with a color. That is
why we need to do this check and keep the flow consistent.
BUG=586097
Committed: https://crrev.com/9c0dda60b98364be39e3656c6c9e9031ee91bf92
Cr-Commit-Position: refs/heads/master@{#382601}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : Rebased against previous CL #
Total comments: 7
Patch Set 5 : #Patch Set 6 : Rebase #Patch Set 7 : Another rebase #Patch Set 8 : Using host + path #
Total comments: 4
Patch Set 9 : #
Messages
Total messages: 25 (5 generated)
Description was changed from ========== Use whitelist large icon for corresponding most visited suggestions. If we get a most visited suggestion that is also a whitelist entry point, use the icon from whitelist when creating he suggestions. This is done in order to have consistently good large icon image on the NTP. The reasoning for this comes from the fact that a lot of websites do not have a large icon which would mean that while the user is browsing and a site transitions from an entry point to a most visited suggestion we will lose the icon image and will replace it with a color. That is why we need to do this check and keep the flow consistent. BUG=586097 ========== to ========== Use whitelist large icon for corresponding most visited suggestions. If we get a most visited suggestion that is also a whitelist entry point, use the icon from whitelist when creating he suggestions. This is done in order to have consistently good large icon image on the NTP. The reasoning for this comes from the fact that a lot of websites do not have a large icon which would mean that while the user is browsing and a site transitions from an entry point to a most visited suggestion we will lose the icon image and will replace it with a color. That is why we need to do this check and keep the flow consistent. BUG=586097 ==========
atanasova@chromium.org changed reviewers: + bauerb@chromium.org, treib@chromium.org
https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:502: if (whitelist->entry_point().host() == url.host()) Hmmm, I'm not sure about this. Different URLs on the same host can have different icons (example: www.heise.de vs www.heise.de/ct). https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.h:109: MostVisitedSource source); nit: Put this ctor (with only the common parameters) first. (If it is even used anymore? Otherwise just delete :) )
https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:502: if (whitelist->entry_point().host() == url.host()) On 2016/03/11 11:42:02, Marc Treib wrote: > Hmmm, I'm not sure about this. Different URLs on the same host can have > different icons (example: http://www.heise.de vs http://www.heise.de/ct). We currently do not display more than 1 website from the same host. That is why I though that following the logic here would be good, but I see your point. I have changed it to "GetContent" instead of "host()". I was trying to think of edge cases where this would be a problem, but I think that if a user accesses a different URL, they either have to enter it manually (so NTP has to effect) or visiting it often enough for it to replace the entry point. So I think using the GetContent should get us the results we want. https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.h:109: MostVisitedSource source); On 2016/03/11 11:42:02, Marc Treib wrote: > nit: Put this ctor (with only the common parameters) first. (If it is even used > anymore? Otherwise just delete :) ) It is used for PopularSites suggestions.
lgtm https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:502: if (whitelist->entry_point().host() == url.host()) On 2016/03/11 13:58:18, atanasova wrote: > On 2016/03/11 11:42:02, Marc Treib wrote: > > Hmmm, I'm not sure about this. Different URLs on the same host can have > > different icons (example: http://www.heise.de vs http://www.heise.de/ct). > > We currently do not display more than 1 website from the same host. That is why > I though that following the logic here would be good, but I see your point. I > have changed it to "GetContent" instead of "host()". I was trying to think of > edge cases where this would be a problem, but I think that if a user accesses a > different URL, they either have to enter it manually (so NTP has to effect) or > visiting it often enough for it to replace the entry point. So I think using the > GetContent should get us the results we want. Are you sure about the "1 website per host"? I'm fairly sure both TopSites and MostLikely can give more than 1 result per host. Any particular reason for not just comparing the URLs? (Not that it will make much of a difference, just wondering) https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.h:109: MostVisitedSource source); On 2016/03/11 13:58:18, atanasova wrote: > On 2016/03/11 11:42:02, Marc Treib wrote: > > nit: Put this ctor (with only the common parameters) first. (If it is even > used > > anymore? Otherwise just delete :) ) > > It is used for PopularSites suggestions. Acknowledged. https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:529: new Suggestion(visited.title, visited.url.spec(), TOP_SITES, nit (pre-existing): ".spec()" isn't needed, Suggestions has a ctor that takes a GURL. https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:563: SUGGESTIONS_SERVICE, GetWhitelistLargeIconPath(GURL(suggestion.url())), You could do "GURL url(suggestion.url());" above and use that here and in the previous line, then the string->GURL conversion only needs to happen once. https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:93: // Will be empty for entries that do not have a whitelist associated with nit: "that don't have an associated whitelist", then it should fit on one line :)
https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:502: if (whitelist->entry_point().host() == url.host()) On 2016/03/11 14:21:01, Marc Treib wrote: > On 2016/03/11 13:58:18, atanasova wrote: > > On 2016/03/11 11:42:02, Marc Treib wrote: > > > Hmmm, I'm not sure about this. Different URLs on the same host can have > > > different icons (example: http://www.heise.de vs http://www.heise.de/ct). > > > > We currently do not display more than 1 website from the same host. That is > why > > I though that following the logic here would be good, but I see your point. I > > have changed it to "GetContent" instead of "host()". I was trying to think of > > edge cases where this would be a problem, but I think that if a user accesses > a > > different URL, they either have to enter it manually (so NTP has to effect) or > > visiting it often enough for it to replace the entry point. So I think using > the > > GetContent should get us the results we want. > > Are you sure about the "1 website per host"? I'm fairly sure both TopSites and > MostLikely can give more than 1 result per host. > > Any particular reason for not just comparing the URLs? (Not that it will make > much of a difference, just wondering) So we are doing this "host" check when adding whitelist or popular sites suggestions. If we already had a host suggested from MostVisited, we are not adding it. The check is not performed inside the suggestions that come from MostVisited or PopularSites. So you do indeed can end up with 2 websites with the same host name coming from MostVisites or PopularSites. I think that the idea of comparing hosts when it comes to PopularSites and whitelists comes from not wanting to spam the user with suggestions that are not extactly what they are looking for. If they already have a website with that host coming from MostVisited, there is likely no need to add it again in a different form from PopularSites. https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:529: new Suggestion(visited.title, visited.url.spec(), TOP_SITES, On 2016/03/11 14:21:01, Marc Treib wrote: > nit (pre-existing): ".spec()" isn't needed, Suggestions has a ctor that takes a > GURL. Done. https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:563: SUGGESTIONS_SERVICE, GetWhitelistLargeIconPath(GURL(suggestion.url())), On 2016/03/11 14:21:01, Marc Treib wrote: > You could do "GURL url(suggestion.url());" above and use that here and in the > previous line, then the string->GURL conversion only needs to happen once. Done. https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1787633002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:93: // Will be empty for entries that do not have a whitelist associated with On 2016/03/11 14:21:01, Marc Treib wrote: > nit: "that don't have an associated whitelist", then it should fit on one line > :) Done.
https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:502: if (whitelist->entry_point().host() == url.host()) On 2016/03/11 15:59:25, atanasova wrote: > On 2016/03/11 14:21:01, Marc Treib wrote: > > On 2016/03/11 13:58:18, atanasova wrote: > > > On 2016/03/11 11:42:02, Marc Treib wrote: > > > > Hmmm, I'm not sure about this. Different URLs on the same host can have > > > > different icons (example: http://www.heise.de vs http://www.heise.de/ct). > > > > > > We currently do not display more than 1 website from the same host. That is > > why > > > I though that following the logic here would be good, but I see your point. > I > > > have changed it to "GetContent" instead of "host()". I was trying to think > of > > > edge cases where this would be a problem, but I think that if a user > accesses > > a > > > different URL, they either have to enter it manually (so NTP has to effect) > or > > > visiting it often enough for it to replace the entry point. So I think using > > the > > > GetContent should get us the results we want. > > > > Are you sure about the "1 website per host"? I'm fairly sure both TopSites and > > MostLikely can give more than 1 result per host. > > > > Any particular reason for not just comparing the URLs? (Not that it will make > > much of a difference, just wondering) > > So we are doing this "host" check when adding whitelist or popular sites > suggestions. If we already had a host suggested from MostVisited, we are not > adding it. The check is not performed inside the suggestions that come from > MostVisited or PopularSites. So you do indeed can end up with 2 websites with > the same host name coming from MostVisites or PopularSites. I think that the > idea of comparing hosts when it comes to PopularSites and whitelists comes from > not wanting to spam the user with suggestions that are not extactly what they > are looking for. If they already have a website with that host coming from > MostVisited, there is likely no need to add it again in a different form from > PopularSites. Yup exactly, for PopSites, we don't want to spam the user with almost-the-same suggestions. (And PopSites will always be only a host anyway.) For whitelists, I guess the same applies, though they can have paths after the host. For replacing the icon, we should be more careful IMO. Better to occasionally end up without an icon, than with a wrong icon.
https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:502: if (whitelist->entry_point().host() == url.host()) On 2016/03/11 16:11:00, Marc Treib wrote: > On 2016/03/11 15:59:25, atanasova wrote: > > On 2016/03/11 14:21:01, Marc Treib wrote: > > > On 2016/03/11 13:58:18, atanasova wrote: > > > > On 2016/03/11 11:42:02, Marc Treib wrote: > > > > > Hmmm, I'm not sure about this. Different URLs on the same host can have > > > > > different icons (example: http://www.heise.de vs > http://www.heise.de/ct). > > > > > > > > We currently do not display more than 1 website from the same host. That > is > > > why > > > > I though that following the logic here would be good, but I see your > point. > > I > > > > have changed it to "GetContent" instead of "host()". I was trying to think > > of > > > > edge cases where this would be a problem, but I think that if a user > > accesses > > > a > > > > different URL, they either have to enter it manually (so NTP has to > effect) > > or > > > > visiting it often enough for it to replace the entry point. So I think > using > > > the > > > > GetContent should get us the results we want. > > > > > > Are you sure about the "1 website per host"? I'm fairly sure both TopSites > and > > > MostLikely can give more than 1 result per host. > > > > > > Any particular reason for not just comparing the URLs? (Not that it will > make > > > much of a difference, just wondering) > > > > So we are doing this "host" check when adding whitelist or popular sites > > suggestions. If we already had a host suggested from MostVisited, we are not > > adding it. The check is not performed inside the suggestions that come from > > MostVisited or PopularSites. So you do indeed can end up with 2 websites with > > the same host name coming from MostVisites or PopularSites. I think that the > > idea of comparing hosts when it comes to PopularSites and whitelists comes > from > > not wanting to spam the user with suggestions that are not extactly what they > > are looking for. If they already have a website with that host coming from > > MostVisited, there is likely no need to add it again in a different form from > > PopularSites. > > Yup exactly, for PopSites, we don't want to spam the user with almost-the-same > suggestions. (And PopSites will always be only a host anyway.) For whitelists, I > guess the same applies, though they can have paths after the host. > For replacing the icon, we should be more careful IMO. Better to occasionally > end up without an icon, than with a wrong icon. Agreed. The next patch has the change where "GetContents" is used to acknowledge this.
@Bernhard, friendly ping :)
https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:466: if (whitelist->entry_point().GetContent() == url.GetContent()) Hm... content is everything after the scheme, so including query parameters and fragment identifier, but _not_ the scheme... Is that what we want? (I mean, the question of when two URLs are the "same" is surprisingly complicated, so we might have to compromise in some form anyway, but I'd like to at least do that consciously.) https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:119: // If there is a whitelist entry point for the URL return the large icon path Nit: comma after URL, and a period at the end.
https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:466: if (whitelist->entry_point().GetContent() == url.GetContent()) On 2016/03/17 15:49:00, Bernhard Bauer wrote: > Hm... content is everything after the scheme, so including query parameters and > fragment identifier, but _not_ the scheme... Is that what we want? > > (I mean, the question of when two URLs are the "same" is surprisingly > complicated, so we might have to compromise in some form anyway, but I'd like to > at least do that consciously.) Marc and I discussed in the previous comments that using host() will not be appropriate, since we want to use the whitelist icon when it is the same url. I personally think that using everything after the scheme will be enough to distinguish the URLs. Since this is a feature that is going to be used by children who might not be able to fully type out addresses, I think that different schemes in URLs will still lead to the same source. However, I am open to suggestions to how we can improve this and we can definitely discuss how exact do we want to be vs. trying to "anticipate" what the child meant. https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:119: // If there is a whitelist entry point for the URL return the large icon path On 2016/03/17 15:49:00, Bernhard Bauer wrote: > Nit: comma after URL, and a period at the end. Done.
https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:466: if (whitelist->entry_point().GetContent() == url.GetContent()) On 2016/03/17 16:02:08, atanasova wrote: > On 2016/03/17 15:49:00, Bernhard Bauer wrote: > > Hm... content is everything after the scheme, so including query parameters > and > > fragment identifier, but _not_ the scheme... Is that what we want? > > > > (I mean, the question of when two URLs are the "same" is surprisingly > > complicated, so we might have to compromise in some form anyway, but I'd like > to > > at least do that consciously.) > > Marc and I discussed in the previous comments that using host() will not be > appropriate, since we want to use the whitelist icon when it is the same url. I > personally think that using everything after the scheme will be enough to > distinguish the URLs. Since this is a feature that is going to be used by > children who might not be able to fully type out addresses, I think that > different schemes in URLs will still lead to the same source. > However, I am open to suggestions to how we can improve this and we can > definitely discuss how exact do we want to be vs. trying to "anticipate" what > the child meant. I think Bernhard's suggesting to use less than the full GetContent() - we're probably not interested in query params.
https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:466: if (whitelist->entry_point().GetContent() == url.GetContent()) On 2016/03/17 16:29:02, Marc Treib wrote: > On 2016/03/17 16:02:08, atanasova wrote: > > On 2016/03/17 15:49:00, Bernhard Bauer wrote: > > > Hm... content is everything after the scheme, so including query parameters > > and > > > fragment identifier, but _not_ the scheme... Is that what we want? > > > > > > (I mean, the question of when two URLs are the "same" is surprisingly > > > complicated, so we might have to compromise in some form anyway, but I'd > like > > to > > > at least do that consciously.) > > > > Marc and I discussed in the previous comments that using host() will not be > > appropriate, since we want to use the whitelist icon when it is the same url. > I > > personally think that using everything after the scheme will be enough to > > distinguish the URLs. Since this is a feature that is going to be used by > > children who might not be able to fully type out addresses, I think that > > different schemes in URLs will still lead to the same source. > > However, I am open to suggestions to how we can improve this and we can > > definitely discuss how exact do we want to be vs. trying to "anticipate" what > > the child meant. > > I think Bernhard's suggesting to use less than the full GetContent() - we're > probably not interested in query params. Exactly. And in particular, I'd like to explicitly list the things we are looking at for equality purposes here, so for example host and path?
https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:466: if (whitelist->entry_point().GetContent() == url.GetContent()) On 2016/03/17 17:44:30, Bernhard Bauer wrote: > On 2016/03/17 16:29:02, Marc Treib wrote: > > On 2016/03/17 16:02:08, atanasova wrote: > > > On 2016/03/17 15:49:00, Bernhard Bauer wrote: > > > > Hm... content is everything after the scheme, so including query > parameters > > > and > > > > fragment identifier, but _not_ the scheme... Is that what we want? > > > > > > > > (I mean, the question of when two URLs are the "same" is surprisingly > > > > complicated, so we might have to compromise in some form anyway, but I'd > > like > > > to > > > > at least do that consciously.) > > > > > > Marc and I discussed in the previous comments that using host() will not be > > > appropriate, since we want to use the whitelist icon when it is the same > url. > > I > > > personally think that using everything after the scheme will be enough to > > > distinguish the URLs. Since this is a feature that is going to be used by > > > children who might not be able to fully type out addresses, I think that > > > different schemes in URLs will still lead to the same source. > > > However, I am open to suggestions to how we can improve this and we can > > > definitely discuss how exact do we want to be vs. trying to "anticipate" > what > > > the child meant. > > > > I think Bernhard's suggesting to use less than the full GetContent() - we're > > probably not interested in query params. > > Exactly. And in particular, I'd like to explicitly list the things we are > looking at for equality purposes here, so for example host and path? Changed to use host+path as discussed here and in the email thread.
https://codereview.chromium.org/1787633002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:161: bool URLsEquals(const GURL& url1, const GURL& url2) { nit: AreURLsEquivalent or something? We're not actually checking for equality. https://codereview.chromium.org/1787633002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:164: return truncated_url1 == truncated_url2; I'd just compare host() and path() separately, that way you don't have to construct new strings. There's also GURL::Replacements but that might be overkill here.
https://codereview.chromium.org/1787633002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1787633002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:161: bool URLsEquals(const GURL& url1, const GURL& url2) { On 2016/03/22 12:25:38, Marc Treib wrote: > nit: AreURLsEquivalent or something? We're not actually checking for equality. Done. https://codereview.chromium.org/1787633002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:164: return truncated_url1 == truncated_url2; On 2016/03/22 12:25:38, Marc Treib wrote: > I'd just compare host() and path() separately, that way you don't have to > construct new strings. > > There's also GURL::Replacements but that might be overkill here. Done.
lgtm
lgtm
The CQ bit was checked by atanasova@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1787633002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1787633002/160001
Message was sent while issue was closed.
Description was changed from ========== Use whitelist large icon for corresponding most visited suggestions. If we get a most visited suggestion that is also a whitelist entry point, use the icon from whitelist when creating he suggestions. This is done in order to have consistently good large icon image on the NTP. The reasoning for this comes from the fact that a lot of websites do not have a large icon which would mean that while the user is browsing and a site transitions from an entry point to a most visited suggestion we will lose the icon image and will replace it with a color. That is why we need to do this check and keep the flow consistent. BUG=586097 ========== to ========== Use whitelist large icon for corresponding most visited suggestions. If we get a most visited suggestion that is also a whitelist entry point, use the icon from whitelist when creating he suggestions. This is done in order to have consistently good large icon image on the NTP. The reasoning for this comes from the fact that a lot of websites do not have a large icon which would mean that while the user is browsing and a site transitions from an entry point to a most visited suggestion we will lose the icon image and will replace it with a color. That is why we need to do this check and keep the flow consistent. BUG=586097 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Use whitelist large icon for corresponding most visited suggestions. If we get a most visited suggestion that is also a whitelist entry point, use the icon from whitelist when creating he suggestions. This is done in order to have consistently good large icon image on the NTP. The reasoning for this comes from the fact that a lot of websites do not have a large icon which would mean that while the user is browsing and a site transitions from an entry point to a most visited suggestion we will lose the icon image and will replace it with a color. That is why we need to do this check and keep the flow consistent. BUG=586097 ========== to ========== Use whitelist large icon for corresponding most visited suggestions. If we get a most visited suggestion that is also a whitelist entry point, use the icon from whitelist when creating he suggestions. This is done in order to have consistently good large icon image on the NTP. The reasoning for this comes from the fact that a lot of websites do not have a large icon which would mean that while the user is browsing and a site transitions from an entry point to a most visited suggestion we will lose the icon image and will replace it with a color. That is why we need to do this check and keep the flow consistent. BUG=586097 Committed: https://crrev.com/9c0dda60b98364be39e3656c6c9e9031ee91bf92 Cr-Commit-Position: refs/heads/master@{#382601} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9c0dda60b98364be39e3656c6c9e9031ee91bf92 Cr-Commit-Position: refs/heads/master@{#382601} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
