Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2829963005:
[Remote suggestions] Get favicon URLs from archive (Closed)
Description[Remote suggestions] Get favicon URLs from archive
Fetching favicons via content suggestions service is currently only
enabled for remote suggestions. The general API for getting the URL is
via content_suggestion.h which does not work for archived suggestions
(i.e. by Fetch More).
This CL introduces a quick fix to loop in remote suggestions provider
for the favicon URLs of suggestions not known to
ContentSuggestionsService. A cleaner fix requires more thinking /
refactoring.
This is a M59 merge candidate.
BUG=714025
Review-Url: https://codereview.chromium.org/2829963005
Cr-Commit-Position: refs/heads/master@{#466653}
Committed: https://chromium.googlesource.com/chromium/src/+/8083bc5ed7a1989652884bec28bf775dd198b447
Patch Set 1 #Patch Set 2 : Fix the unit-test #
Total comments: 6
Patch Set 3 : Tim's comments #
Total comments: 7
Patch Set 4 : Comments #2 #
Total comments: 2
Patch Set 5 : Comments #3 #Messages
Total messages: 31 (17 generated)
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...
jkrcal@chromium.org changed reviewers: + tschumann@chromium.org, vitaliii@chromium.org
Tim, could you PTAL? cc to Vitalii who influenced the earlier decision to skip the call to suggestions provider (and have the url in content_suggestion.h).
On 2017/04/21 07:46:09, jkrcal wrote: > Tim, could you PTAL? > > cc to Vitalii who influenced the earlier decision to skip the call to > suggestions provider (and have the url in content_suggestion.h). Jan, can you update the bug or CL description with the precise error scenario that this CL fixes but that cannot be fixed with storing the URL in content_suggestions.h ? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
I expanded the bug description (and fixed a unit-test compilation problem).
thanks! https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:172: if (position != suggestions->end()) { nit: this is a bit tricky to follow (especially with my other comment): I'd be in favor of moving this into a helper method (you can write it next to this method to have easier navigation when reading the code). like GURL domain_with_favicon = GetFaviconDomain(suggestion_id); You can then write the selection using return statements which is easier to follow: if (position != suggestions->end()) { return position->url_with_favicon(); } if (...) { ...; } return GURL(); https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:176: KnownCategories::ARTICLES)) { unknown remote suggestions should also be tackled here. We should user providers_by_category and compare against static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider). Not sure if that's nicer though. On the other hand, it guarantees we're asking the right provider (and the provider implementation acutally checks the category...).
oh yeah, and please add a TODO that this hack should get removed when we find a proper solution for FetchMore with a single, long-lived surface (aka ChromeHome tab). --Tim On Fri, Apr 21, 2017 at 10:27 AM, <tschumann@chromium.org> wrote: > thanks! > > > https://codereview.chromium.org/2829963005/diff/20001/ > components/ntp_snippets/content_suggestions_service.cc > File components/ntp_snippets/content_suggestions_service.cc (right): > > https://codereview.chromium.org/2829963005/diff/20001/ > components/ntp_snippets/content_suggestions_service.cc#newcode172 > components/ntp_snippets/content_suggestions_service.cc:172: if (position > != suggestions->end()) { > nit: this is a bit tricky to follow (especially with my other comment): > I'd be in favor of moving this into a helper method (you can write it > next to this method to have easier navigation when reading the code). > like > > GURL domain_with_favicon = GetFaviconDomain(suggestion_id); > > You can then write the selection using return statements which is easier > to follow: > if (position != suggestions->end()) { > return position->url_with_favicon(); > } > > if (...) { > ...; > } > return GURL(); > > https://codereview.chromium.org/2829963005/diff/20001/ > components/ntp_snippets/content_suggestions_service.cc#newcode176 > components/ntp_snippets/content_suggestions_service.cc:176: > KnownCategories::ARTICLES)) { > unknown remote suggestions should also be tackled here. > We should user providers_by_category and compare against > static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider). > Not sure if that's nicer though. On the other hand, it guarantees we're > asking the right provider (and the provider implementation acutally > checks the category...). > > https://codereview.chromium.org/2829963005/ > Tim Schumann Software Engineer tschumann@google.com Google Germany GmbH Erika-Mann-Str. 33 Munich, 80636 Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, Tim! Could you PTAL, again? There already is a TODO to remove this hack. Did you mean something different? https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:172: if (position != suggestions->end()) { On 2017/04/21 08:27:55, tschumann wrote: > nit: this is a bit tricky to follow (especially with my other comment): > I'd be in favor of moving this into a helper method (you can write it next to > this method to have easier navigation when reading the code). > like > > GURL domain_with_favicon = GetFaviconDomain(suggestion_id); > > You can then write the selection using return statements which is easier to > follow: > if (position != suggestions->end()) { > return position->url_with_favicon(); > } > > if (...) { > ...; > } > return GURL(); Done. https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:176: KnownCategories::ARTICLES)) { On 2017/04/21 08:27:54, tschumann wrote: > unknown remote suggestions should also be tackled here. > We should user providers_by_category and compare against > static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider). Not sure > if that's nicer though. On the other hand, it guarantees we're asking the right > provider (and the provider implementation acutally checks the category...). Due to privacy, we call the new code path from java only for ARTICLES in M59. So this is overgeneralizing. I hope we find a nicer way around for M60 (for which I hope we unblock privacy).
https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:164: const GURL& domain_with_favicon = GetFaviconDomain(suggestion_id); Since we started using "domain" in the naming, should we rename the url_with_favicon in other places (e.g. ContentSuggestion)? https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:353: return suggestion->url().GetWithEmptyPath(); This duplicates logic in ContentSuggestion. Should we move it into a static function there, e.g. GURL GetFaviconDomain(const GURL& suggestion_url, const GURL& suggestion_amp_url)?
https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:176: KnownCategories::ARTICLES)) { On 2017/04/21 10:57:00, jkrcal wrote: > On 2017/04/21 08:27:54, tschumann wrote: > > unknown remote suggestions should also be tackled here. > > We should user providers_by_category and compare against > > static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider). Not > sure > > if that's nicer though. On the other hand, it guarantees we're asking the > right > > provider (and the provider implementation acutally checks the category...). > > Due to privacy, we call the new code path from java only for ARTICLES in M59. So > this is overgeneralizing. I hope we find a nicer way around for M60 (for which I > hope we unblock privacy). I would prefer we only have a single place where we decide for which articles to fetch those. That decision should be done on a higher level and down here we should just properly handle Favicon requests. https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:164: const GURL& domain_with_favicon = GetFaviconDomain(suggestion_id); On 2017/04/21 11:09:11, vitaliii wrote: > Since we started using "domain" in the naming, should we rename the > url_with_favicon in other places (e.g. ContentSuggestion)? please not in this CL ;-) https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:186: std::vector<ContentSuggestion>* suggestions = nit: can we make this a const& instead?
Thanks, PTAL, again! https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:176: KnownCategories::ARTICLES)) { On 2017/04/21 11:23:58, tschumann wrote: > On 2017/04/21 10:57:00, jkrcal wrote: > > On 2017/04/21 08:27:54, tschumann wrote: > > > unknown remote suggestions should also be tackled here. > > > We should user providers_by_category and compare against > > > static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider). Not > > sure > > > if that's nicer though. On the other hand, it guarantees we're asking the > > right > > > provider (and the provider implementation acutally checks the category...). > > > > Due to privacy, we call the new code path from java only for ARTICLES in M59. > So > > this is overgeneralizing. I hope we find a nicer way around for M60 (for which > I > > hope we unblock privacy). > > I would prefer we only have a single place where we decide for which articles to > fetch those. > That decision should be done on a higher level and down here we should just > properly handle Favicon requests. Okay, done. https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:164: const GURL& domain_with_favicon = GetFaviconDomain(suggestion_id); On 2017/04/21 11:23:58, tschumann wrote: > On 2017/04/21 11:09:11, vitaliii wrote: > > Since we started using "domain" in the naming, should we rename the > > url_with_favicon in other places (e.g. ContentSuggestion)? > > please not in this CL ;-) Acknowledged. https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:186: std::vector<ContentSuggestion>* suggestions = On 2017/04/21 11:23:58, tschumann wrote: > nit: can we make this a const& instead? Done. https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:353: return suggestion->url().GetWithEmptyPath(); On 2017/04/21 11:09:11, vitaliii wrote: > This duplicates logic in ContentSuggestion. > Should we move it into a static function there, e.g. > GURL GetFaviconDomain(const GURL& suggestion_url, const GURL& > suggestion_amp_url)? Done (similarly).
On 2017/04/21 13:30:55, jkrcal wrote: > Thanks, PTAL, again! > > https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... > File components/ntp_snippets/content_suggestions_service.cc (right): > > https://codereview.chromium.org/2829963005/diff/20001/components/ntp_snippets... > components/ntp_snippets/content_suggestions_service.cc:176: > KnownCategories::ARTICLES)) { > On 2017/04/21 11:23:58, tschumann wrote: > > On 2017/04/21 10:57:00, jkrcal wrote: > > > On 2017/04/21 08:27:54, tschumann wrote: > > > > unknown remote suggestions should also be tackled here. > > > > We should user providers_by_category and compare against > > > > static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider). Not > > > sure > > > > if that's nicer though. On the other hand, it guarantees we're asking the > > > right > > > > provider (and the provider implementation acutally checks the > category...). > > > > > > Due to privacy, we call the new code path from java only for ARTICLES in > M59. > > So > > > this is overgeneralizing. I hope we find a nicer way around for M60 (for > which > > I > > > hope we unblock privacy). > > > > I would prefer we only have a single place where we decide for which articles > to > > fetch those. > > That decision should be done on a higher level and down here we should just > > properly handle Favicon requests. > > Okay, done. > > https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... > File components/ntp_snippets/content_suggestions_service.cc (right): > > https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... > components/ntp_snippets/content_suggestions_service.cc:164: const GURL& > domain_with_favicon = GetFaviconDomain(suggestion_id); > On 2017/04/21 11:23:58, tschumann wrote: > > On 2017/04/21 11:09:11, vitaliii wrote: > > > Since we started using "domain" in the naming, should we rename the > > > url_with_favicon in other places (e.g. ContentSuggestion)? > > > > please not in this CL ;-) > > Acknowledged. > > https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... > components/ntp_snippets/content_suggestions_service.cc:186: > std::vector<ContentSuggestion>* suggestions = > On 2017/04/21 11:23:58, tschumann wrote: > > nit: can we make this a const& instead? > > Done. > > https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... > File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): > > https://codereview.chromium.org/2829963005/diff/40001/components/ntp_snippets... > components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:353: return > suggestion->url().GetWithEmptyPath(); > On 2017/04/21 11:09:11, vitaliii wrote: > > This duplicates logic in ContentSuggestion. > > Should we move it into a static function there, e.g. > > GURL GetFaviconDomain(const GURL& suggestion_url, const GURL& > > suggestion_amp_url)? > > Done (similarly). Friendly ping, Tim!
lgtm https://codereview.chromium.org/2829963005/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:201: static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider_)) { thinking of my comment from Friday, I don't think the cast is actually necessary (== should do the same cast as the LHS is the base class). I honestly don't feel strongly. If you think the cast adds clarity, feel free to leave it in. Otherwise feel free to remove.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2829963005/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2829963005/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:201: static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider_)) { On 2017/04/24 09:29:56, tschumann wrote: > thinking of my comment from Friday, I don't think the cast is actually necessary > (== should do the same cast as the LHS is the base class). > > I honestly don't feel strongly. If you think the cast adds clarity, feel free to > leave it in. Otherwise feel free to remove. Good point, I slightly prefer the version without the case.
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_chromium_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 jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2829963005/#ps80001 (title: "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": 80001, "attempt_start_ts": 1493044621907890,
"parent_rev": "6a0288a5e8d47ab32160cdabd48c7c8faf871060", "commit_rev":
"8083bc5ed7a1989652884bec28bf775dd198b447"}
Message was sent while issue was closed.
Description was changed from ========== [Remote suggestions] Get favicon URLs from archive Fetching favicons via content suggestions service is currently only enabled for remote suggestions. The general API for getting the URL is via content_suggestion.h which does not work for archived suggestions (i.e. by Fetch More). This CL introduces a quick fix to loop in remote suggestions provider for the favicon URLs of suggestions not known to ContentSuggestionsService. A cleaner fix requires more thinking / refactoring. This is a M59 merge candidate. BUG=714025 ========== to ========== [Remote suggestions] Get favicon URLs from archive Fetching favicons via content suggestions service is currently only enabled for remote suggestions. The general API for getting the URL is via content_suggestion.h which does not work for archived suggestions (i.e. by Fetch More). This CL introduces a quick fix to loop in remote suggestions provider for the favicon URLs of suggestions not known to ContentSuggestionsService. A cleaner fix requires more thinking / refactoring. This is a M59 merge candidate. BUG=714025 Review-Url: https://codereview.chromium.org/2829963005 Cr-Commit-Position: refs/heads/master@{#466653} Committed: https://chromium.googlesource.com/chromium/src/+/8083bc5ed7a1989652884bec28bf... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8083bc5ed7a1989652884bec28bf... |
