Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2811123003:
[Content suggestions] Allow to specify URLs for favicons (Closed)
|
Created:
3 years, 8 months ago by jkrcal Modified:
3 years, 8 months ago Reviewers:
Marc Treib CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, gambard Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Content suggestions] Allow to override URLs for favicons
This CL enables content suggestions providers to specify the URL used
to fetch favicon of the suggestion.
To minimize code bloat, this is not needed. If url_with_favicon() is missing, url() is used by ContentSuggestionsService, instead.
BUG=676259
Review-Url: https://codereview.chromium.org/2811123003
Cr-Commit-Position: refs/heads/master@{#464314}
Committed: https://chromium.googlesource.com/chromium/src/+/cd011686c4e1fb0b82628b8c08fde2f8cc7b9bcb
Patch Set 1 #
Total comments: 5
Patch Set 2 : Marc's comment #
Total comments: 10
Patch Set 3 : Marc's comments #2 #Patch Set 4 : Marc's comments #3 #
Messages
Total messages: 29 (19 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
[Preliminary version, this does not compile]
Marc, could you PTAL before I go and add { return GURL();} to all the other
providers?
Once this is done, you should notify gambard@: Currently ReadingListSuggestionExtra has a special favicon_url field which can then go away. https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:102: virtual GURL GetSuggestionURLForFavicon( Hm, rather than routing this to the provider on-demand, would it be easier to add it as an extra field on ContentSuggestion? https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:173: return publisher_url.GetWithEmptyPath(); Why GetWithEmptyPath? Shouldn't the provider decide that? It might have an actual icon URL (right?)
vitaliii@chromium.org changed reviewers: + vitaliii@chromium.org
drive-by. https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:102: virtual GURL GetSuggestionURLForFavicon( On 2017/04/12 10:00:59, Marc Treib wrote: > Hm, rather than routing this to the provider on-demand, would it be easier to > add it as an extra field on ContentSuggestion? I think we *have* to use an extra field. The approach above will force all providers to store "id -> favicon_url" mapping for all suggestions they report, which is fairly unpleasant.
Description was changed from ========== [Content suggestions] Allow to override URLs for favicons This CL enables content suggestions providers to override the URL used to fetch favicon of the suggestion. To minimize code bloat, each provider can also return empty URL. In this case, the service will use ContentSuggestion::url() of the cached suggestion, instead. BUG=676259 ========== to ========== [Content suggestions] Allow to override URLs for favicons This CL enables content suggestions providers to override the URL used to fetch favicon of the suggestion. To minimize code bloat, each provider can also return empty URL. In this case, the service will use ContentSuggestion::url() of the cached suggestion, instead. BUG=676259 ==========
vitaliii@chromium.org changed reviewers: - vitaliii@chromium.org
Description was changed from ========== [Content suggestions] Allow to override URLs for favicons This CL enables content suggestions providers to override the URL used to fetch favicon of the suggestion. To minimize code bloat, each provider can also return empty URL. In this case, the service will use ContentSuggestion::url() of the cached suggestion, instead. BUG=676259 ========== to ========== [Content suggestions] Allow to override URLs for favicons This CL enables content suggestions providers to specify the URL used to fetch favicon of the suggestion. To minimize code bloat, this is not needed. If url_with_favicon() is missing, url() is used by ContentSuggestionsService, instead. BUG=676259 ==========
Thanks, PTAL again! (now it compiles so treat it as a final CL, please) https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:102: virtual GURL GetSuggestionURLForFavicon( On 2017/04/12 10:00:59, Marc Treib wrote: > Hm, rather than routing this to the provider on-demand, would it be easier to > add it as an extra field on ContentSuggestion? Okay, that's even easier! https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2811123003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:173: return publisher_url.GetWithEmptyPath(); On 2017/04/12 10:00:59, Marc Treib wrote: > Why GetWithEmptyPath? Shouldn't the provider decide that? It might have an > actual icon URL (right?) No, the URL given by the provider must be a page URL. The favicons are fetched via Google favicon server which only allows fetching favicons for a given _page_ URL. We use the top-level path on the domain to simplify life for the favicon service (without big sacrifice on quality, in this case). The same is used by the previous favicon server (called directly from the java code). We may reconsider this later...
The CQ bit was checked by jkrcal@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...
LGTM with nits https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:155: const GURL& publisher_url = s/publisher_url/url_with_favicon/ ? https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:157: ? position->url_with_favicon().GetWithEmptyPath() Hm, should the fallback logic live in ContentSuggestion directly? https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:172: void ContentSuggestionsService::OnGetFaviconFromCacheFinished( Orthogonal to this CL: We might want to extract the icon fetching logic into a separate class. Looks like the caching, fallback logic etc. could be encapsulated nicely. https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestion.cc (right): https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestion.cc:397: // If AMP URL is provided as the main URL, we set the non-AMP URL for favicon. Please describe the "why" rather than the "what". It'd also be nice to merge the two ifs, or maybe introduce a "bool use_amp".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:155: const GURL& publisher_url = On 2017/04/12 11:48:00, Marc Treib wrote: > s/publisher_url/url_with_favicon/ ? Done. Renamed to domain_with_favicon to make the trimming of the URL even more apparent. https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:157: ? position->url_with_favicon().GetWithEmptyPath() On 2017/04/12 11:48:00, Marc Treib wrote: > Hm, should the fallback logic live in ContentSuggestion directly? I do not think so, because it is CSS who is using the fetching API of LargeIconService so it is its responsibility to choose what URL trimming to use. https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:172: void ContentSuggestionsService::OnGetFaviconFromCacheFinished( On 2017/04/12 11:48:00, Marc Treib wrote: > Orthogonal to this CL: We might want to extract the icon fetching logic into a > separate class. Looks like the caching, fallback logic etc. could be > encapsulated nicely. Good point, added a CL. https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestion.cc (right): https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestion.cc:397: // If AMP URL is provided as the main URL, we set the non-AMP URL for favicon. On 2017/04/12 11:48:00, Marc Treib wrote: > Please describe the "why" rather than the "what". > It'd also be nice to merge the two ifs, or maybe introduce a "bool use_amp". Done.
The CQ bit was checked by jkrcal@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/2811123003/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:157: ? position->url_with_favicon().GetWithEmptyPath() On 2017/04/12 12:44:19, jkrcal wrote: > On 2017/04/12 11:48:00, Marc Treib wrote: > > Hm, should the fallback logic live in ContentSuggestion directly? > > I do not think so, because it is CSS who is using the fetching API of > LargeIconService so it is its responsibility to choose what URL trimming to use. I didn't mean the trimming, just the "url_with_favicon.is_valid() ? url_with_favicon : url"
Thanks, again :) https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2811123003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:157: ? position->url_with_favicon().GetWithEmptyPath() On 2017/04/12 12:50:18, Marc Treib wrote: > On 2017/04/12 12:44:19, jkrcal wrote: > > On 2017/04/12 11:48:00, Marc Treib wrote: > > > Hm, should the fallback logic live in ContentSuggestion directly? > > > > I do not think so, because it is CSS who is using the fetching API of > > LargeIconService so it is its responsibility to choose what URL trimming to > use. > > I didn't mean the trimming, just the "url_with_favicon.is_valid() ? > url_with_favicon : url" Ah, fair enough. Done.
The CQ bit was checked by jkrcal@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.
The CQ bit was checked by jkrcal@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/2811123003/#ps60001 (title: "Marc's comments #3")
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": 60001, "attempt_start_ts": 1492061159845940,
"parent_rev": "68c508188787079d0400260ee4dff6ef6814c40c", "commit_rev":
"cd011686c4e1fb0b82628b8c08fde2f8cc7b9bcb"}
Message was sent while issue was closed.
Description was changed from ========== [Content suggestions] Allow to override URLs for favicons This CL enables content suggestions providers to specify the URL used to fetch favicon of the suggestion. To minimize code bloat, this is not needed. If url_with_favicon() is missing, url() is used by ContentSuggestionsService, instead. BUG=676259 ========== to ========== [Content suggestions] Allow to override URLs for favicons This CL enables content suggestions providers to specify the URL used to fetch favicon of the suggestion. To minimize code bloat, this is not needed. If url_with_favicon() is missing, url() is used by ContentSuggestionsService, instead. BUG=676259 Review-Url: https://codereview.chromium.org/2811123003 Cr-Commit-Position: refs/heads/master@{#464314} Committed: https://chromium.googlesource.com/chromium/src/+/cd011686c4e1fb0b82628b8c08fd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/cd011686c4e1fb0b82628b8c08fd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
