Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1868523002:
[NTP Snippets] Fetcher: only download snippets that have a thumbnail (Closed)
|
Created:
4 years, 8 months ago by Marc Treib Modified:
4 years, 8 months ago Reviewers:
May CC:
chromium-reviews, mcwilliams+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP Snippets] Fetcher: only download snippets that have a thumbnail
(by enforcing that all metadata is present)
BUG=599470
Committed: https://crrev.com/4b37fccfad398a82c5e1f03c9c00aa5c118edd0d
Cr-Commit-Position: refs/heads/master@{#385806}
Patch Set 1 #
Total comments: 4
Patch Set 2 : separate restricts #Messages
Total messages: 13 (3 generated)
treib@chromium.org changed reviewers: + maybelle@chromium.org
PTAL! (Also, note to my future self: This time, I have verified that this works, and we still get snippets! :D)
https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:44: " \"value\": \"ALL\"" Does ALL mean title, snippet, thumbnail, URL? Or would it also remove snippets that don't have a favicon or canonical publisher name. A lot of the data I've seen returned from ChromeReader don't have these two (though I think from Patrick's previous talks with Newsstand we can't use that data anyway).
https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:44: " \"value\": \"ALL\"" On 2016/04/06 13:39:20, May wrote: > Does ALL mean title, snippet, thumbnail, URL? Or would it also remove snippets > that don't have a favicon or canonical publisher name. A lot of the data I've > seen returned from ChromeReader don't have these two (though I think from > Patrick's previous talks with Newsstand we can't use that data anyway). ALL means title, snippet, and thumbnail. (Presumably URL is always there anyway). Those are all the possible values; I guess if new metadata restricts are added in the future, then those would also be applied. So maybe it would be better to explicitly specify title, snippet, and thumbnail - WDYT?
On 2016/04/06 13:52:54, Marc Treib wrote: > https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... > File components/ntp_snippets/ntp_snippets_fetcher.cc (right): > > https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... > components/ntp_snippets/ntp_snippets_fetcher.cc:44: " \"value\": \"ALL\"" > On 2016/04/06 13:39:20, May wrote: > > Does ALL mean title, snippet, thumbnail, URL? Or would it also remove snippets > > that don't have a favicon or canonical publisher name. A lot of the data I've > > seen returned from ChromeReader don't have these two (though I think from > > Patrick's previous talks with Newsstand we can't use that data anyway). > > ALL means title, snippet, and thumbnail. (Presumably URL is always there > anyway). Those are all the possible values; I guess if new metadata restricts > are added in the future, then those would also be applied. So maybe it would be > better to explicitly specify title, snippet, and thumbnail - WDYT? Ping! :)
https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:44: " \"value\": \"ALL\"" On 2016/04/06 13:52:54, Marc Treib wrote: > On 2016/04/06 13:39:20, May wrote: > > Does ALL mean title, snippet, thumbnail, URL? Or would it also remove snippets > > that don't have a favicon or canonical publisher name. A lot of the data I've > > seen returned from ChromeReader don't have these two (though I think from > > Patrick's previous talks with Newsstand we can't use that data anyway). > > ALL means title, snippet, and thumbnail. (Presumably URL is always there > anyway). Those are all the possible values; I guess if new metadata restricts > are added in the future, then those would also be applied. So maybe it would be > better to explicitly specify title, snippet, and thumbnail - WDYT? Yeah, I think so. It would insulate us better from changes that ChromeReader makes to their API.
https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1868523002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:44: " \"value\": \"ALL\"" On 2016/04/07 16:05:12, May wrote: > On 2016/04/06 13:52:54, Marc Treib wrote: > > On 2016/04/06 13:39:20, May wrote: > > > Does ALL mean title, snippet, thumbnail, URL? Or would it also remove > snippets > > > that don't have a favicon or canonical publisher name. A lot of the data > I've > > > seen returned from ChromeReader don't have these two (though I think from > > > Patrick's previous talks with Newsstand we can't use that data anyway). > > > > ALL means title, snippet, and thumbnail. (Presumably URL is always there > > anyway). Those are all the possible values; I guess if new metadata restricts > > are added in the future, then those would also be applied. So maybe it would > be > > better to explicitly specify title, snippet, and thumbnail - WDYT? > > Yeah, I think so. It would insulate us better from changes that ChromeReader > makes to their API. Done.
lgtm
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868523002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868523002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Fetcher: only download snippets that have a thumbnail (by enforcing that all metadata is present) BUG=599470 ========== to ========== [NTP Snippets] Fetcher: only download snippets that have a thumbnail (by enforcing that all metadata is present) BUG=599470 Committed: https://crrev.com/4b37fccfad398a82c5e1f03c9c00aa5c118edd0d Cr-Commit-Position: refs/heads/master@{#385806} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4b37fccfad398a82c5e1f03c9c00aa5c118edd0d Cr-Commit-Position: refs/heads/master@{#385806} |
