Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1921553004:
Add favicon and publisher name to snippet cards (Closed)
DescriptionAdd favicon and publisher name to snippet cards
BUG=596410
Committed: https://crrev.com/f7360bf8adcd2455a07dbf39f41f33b6bc194899
Cr-Commit-Position: refs/heads/master@{#391493}
Patch Set 1 #Patch Set 2 : #
Total comments: 66
Patch Set 3 : Code review comments. Will address test issues in another update. #
Total comments: 20
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #
Total comments: 19
Patch Set 6 : #
Total comments: 12
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #Patch Set 9 : Fix junit test #Patch Set 10 : Change unit test to use non-C++ 11 STL functions #Messages
Total messages: 43 (11 generated)
maybelle@google.com changed reviewers: + bauerb@chromium.org, maybelle@google.com, treib@chromium.org
https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:7: #include "base/memory/scoped_ptr.h" Not needed anymore (bad merge?) https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:15: const char kSiteTitle[] = "sourceName"; nit: I'd move these two down to the other sourcecorpus stuff. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:73: if (value->GetAsDictionary(&dict_value)) { nit: "if (!Get...) continue;", to remove one level of indentation? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:76: if (dict_value->GetString(kCorpusId, &corpus_id_str)) { You probably also want to handle the "else" here, i.e. the corpus ID is missing or not a string? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:80: DLOG(WARNING) << "Invalid article url " << corpus_id.spec(); I think if a GURL is invalid, then it's spec will be empty. I'd use corpus_id_str here. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:87: if (dict_value->GetDictionary(kPublisherData, &publisher_data)) { Also here: Do we want to warn also if the publisher data dict is missing? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:91: << corpus_id.spec(); nit: misaligned https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:101: DLOG(WARNING) << "Invalid AMP url " << amp_url.spec(); nit: DLOG_IF https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:113: // 3) A source that has url and AMP url, or url only 3) ...so, just URL (which is required anyway), and we don't care about AMP? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:114: if (snippet->sources_.size() > 0) { nit: !.empty() https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:117: SnippetSource current_source = snippet->sources_[i]; const SnippetSource& ? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:125: best_source_found = current_source; Here we can break directly, no? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:149: dict->SetString(kUrl, url_.spec()); Hm. So the URL is now effectively stored twice, but the other per-source items aren't. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:61: // Optional data So far all of the fields, except for the URL, have been optional. Are we changing that? IIRC, we usually don't get expiry_date from ChromeReader for example? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:99: const std::vector<SnippetSource> get_sources() const { return sources_; } Return by ref? Also just "sources", no "get". https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:101: bool is_valid_snippet() const { nit: just "is_valid" please (though IMO "valid" might not be the right term - isn't a snippet without publisher still valid for example?) https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:120: GURL amp_url_; A few fields are now effectively stored twice. Can we get rid of them here, and just store a "best source index" instead? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:122: std::vector<SnippetSource> sources_; Or in fact: Do we actually need to store all the sources? Don't we just need the best one? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:149: // data we need to display it properly I dislike this very much - IMO doing stuff differently in Release vs Developer builds is a Very Bad Thing. Is there any particular reason for this?
https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:188: private void upadateFavicon(final SnippetArticle snippet) { Nit: updateFavicon :) Also, the final is unnecessary. https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:218: URL url = null; I think it's more common to use java.net.URI when you don't actually want to load the URL from the network (at least not via Java in this case). https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:226: String origin = null; I wouldn't call this origin if it's just protocol and host (technically, an origin includes the port as well). Also, could we extract this into a method that returns the URL to use for the favicon? That might help with dealing with some of the error cases here. https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:237: Drawable domainFavicon = new BitmapDrawable( I think you could extract this into a shared method. Maybe one that takes a Drawable and sets it on the |mPublisherTextView|, and one that takes a Bitmap and then calls the former method with a BitmapDrawable? Then you could also reuse the former method for drawDefaultFavicon(). https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:69: DLOG(WARNING) << "No sources found for article " << url_str; You could `return snippet` here to avoid having to indent the rest of the method. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:118: if (!best_source_found.publisher_name.empty()) { I feel like this would be best done with a comparison function, and then just picking the maximum element. If you wanted to get real fancy, you might be able to do that with std::max_element, in fact. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:295: std::vector<std::string> source_urls; I think you could use an initializer list here to construct the vector in one go? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:300: amp_urls.push_back(std::string("")); The empty string literal is unnecessary.
https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:188: private void upadateFavicon(final SnippetArticle snippet) { On 2016/04/27 13:22:05, Bernhard Bauer wrote: > Nit: updateFavicon :) > > Also, the final is unnecessary. Done. https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:218: URL url = null; On 2016/04/27 13:22:05, Bernhard Bauer wrote: > I think it's more common to use java.net.URI when you don't actually want to > load the URL from the network (at least not via Java in this case). Done. https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:226: String origin = null; On 2016/04/27 13:22:05, Bernhard Bauer wrote: > I wouldn't call this origin if it's just protocol and host (technically, an > origin includes the port as well). Also, could we extract this into a method > that returns the URL to use for the favicon? That might help with dealing with > some of the error cases here. Not sure I understand the second part. We first try to get the cached favicon of the exact article URL. If that fails, we do this thing where we get the origin (okay, minus the port) and try to get the favicon for that. Not sure what extracting that into a method helps with? https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:237: Drawable domainFavicon = new BitmapDrawable( On 2016/04/27 13:22:05, Bernhard Bauer wrote: > I think you could extract this into a shared method. Maybe one that takes a > Drawable and sets it on the |mPublisherTextView|, and one that takes a Bitmap > and then calls the former method with a BitmapDrawable? Then you could also > reuse the former method for drawDefaultFavicon(). Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:7: #include "base/memory/scoped_ptr.h" On 2016/04/27 07:03:09, Marc Treib wrote: > Not needed anymore (bad merge?) Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:15: const char kSiteTitle[] = "sourceName"; On 2016/04/27 07:03:09, Marc Treib wrote: > nit: I'd move these two down to the other sourcecorpus stuff. Done. Removed sourceLogoUrl since we don't use it. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:69: DLOG(WARNING) << "No sources found for article " << url_str; On 2016/04/27 13:22:05, Bernhard Bauer wrote: > You could `return snippet` here to avoid having to indent the rest of the > method. Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:73: if (value->GetAsDictionary(&dict_value)) { On 2016/04/27 07:03:09, Marc Treib wrote: > nit: "if (!Get...) continue;", to remove one level of indentation? Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:76: if (dict_value->GetString(kCorpusId, &corpus_id_str)) { On 2016/04/27 07:03:09, Marc Treib wrote: > You probably also want to handle the "else" here, i.e. the corpus ID is missing > or not a string? Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:80: DLOG(WARNING) << "Invalid article url " << corpus_id.spec(); On 2016/04/27 07:03:09, Marc Treib wrote: > I think if a GURL is invalid, then it's spec will be empty. I'd use > corpus_id_str here. Eep, good catch. Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:87: if (dict_value->GetDictionary(kPublisherData, &publisher_data)) { On 2016/04/27 07:03:09, Marc Treib wrote: > Also here: Do we want to warn also if the publisher data dict is missing? Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:91: << corpus_id.spec(); On 2016/04/27 07:03:09, Marc Treib wrote: > nit: misaligned Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:101: DLOG(WARNING) << "Invalid AMP url " << amp_url.spec(); On 2016/04/27 07:03:09, Marc Treib wrote: > nit: DLOG_IF Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:113: // 3) A source that has url and AMP url, or url only On 2016/04/27 07:03:09, Marc Treib wrote: > 3) ...so, just URL (which is required anyway), and we don't care about AMP? yeah, clarified a bit in the comments. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:114: if (snippet->sources_.size() > 0) { On 2016/04/27 07:03:09, Marc Treib wrote: > nit: !.empty() Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:117: SnippetSource current_source = snippet->sources_[i]; On 2016/04/27 07:03:09, Marc Treib wrote: > const SnippetSource& ? Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:118: if (!best_source_found.publisher_name.empty()) { On 2016/04/27 13:22:05, Bernhard Bauer wrote: > I feel like this would be best done with a comparison function, and then just > picking the maximum element. If you wanted to get real fancy, you might be able > to do that with std::max_element, in fact. Pshaw, you kids with your fancy al-gow-rhythms. Given the discussion today re: asking ChromeReader to just give us a definitive source, I will wait on updating the CR to do this as it's possible I will be updating the CR shortly to do something completely different... https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:125: best_source_found = current_source; On 2016/04/27 07:03:09, Marc Treib wrote: > Here we can break directly, no? Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:149: dict->SetString(kUrl, url_.spec()); On 2016/04/27 07:03:09, Marc Treib wrote: > Hm. So the URL is now effectively stored twice, but the other per-source items > aren't. yeah, mainly because the original url that we get from ChromeReader could be different from all the source_urls. It's useful for deduping and debugging. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:61: // Optional data On 2016/04/27 07:03:09, Marc Treib wrote: > So far all of the fields, except for the URL, have been optional. Are we > changing that? IIRC, we usually don't get expiry_date from ChromeReader for > example? Removed favicon actually, since we don't get that data anymore from ChromeReader. But to answer the general question: I count as 'optional' fields that we don't need to display the snippet to the user. In your example, expiry date is not optional (since we need it to know when to stop showing it to the user). Also, we calculate that each time so it should never be blank. Given the insistence that we only get data from ChromeReader that is "perfect", I'm taking the route of erring on the side of only showing the user something that is complete. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:99: const std::vector<SnippetSource> get_sources() const { return sources_; } On 2016/04/27 07:03:09, Marc Treib wrote: > Return by ref? Also just "sources", no "get". Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:101: bool is_valid_snippet() const { On 2016/04/27 07:03:09, Marc Treib wrote: > nit: just "is_valid" please (though IMO "valid" might not be the right term - > isn't a snippet without publisher still valid for example?) Changed to complete(). Better..? https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:120: GURL amp_url_; On 2016/04/27 07:03:09, Marc Treib wrote: > A few fields are now effectively stored twice. Can we get rid of them here, and > just store a "best source index" instead? Done. I kept url_ because we do have a url from ChromeReader that's separate from the source info, it's just that we prefer the source info data. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:122: std::vector<SnippetSource> sources_; On 2016/04/27 07:03:09, Marc Treib wrote: > Or in fact: Do we actually need to store all the sources? Don't we just need the > best one? I do it so that we can effectively de-dupe later as new snippets come in (see crbug/603884). We can't guarantee that the sources we receive from ChromeReader will always be in the same order (or that we have the same exact sources even -- it's possible that in a later fetch a new source came in for the same article) so we won't always choose the same best source. De-duping not in this CL yet, planning to submit a separate one for that. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:149: // data we need to display it properly On 2016/04/27 07:03:09, Marc Treib wrote: > I dislike this very much - IMO doing stuff differently in Release vs Developer > builds is a Very Bad Thing. Is there any particular reason for this? I had a long internal debate about this. Basically what I want to achieve is that we only show "perfect" snippets to users, but I also want us (devs) to know right away if something's not right with the data so we can talk to the ChromeReader guys about it. I thought about adding a flag, but we'd need to have everyone turn it on. Or we could also just add everything for now, but add the if(snippet->valid()) check before we launch to stable. Thoughts?
https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:61: // Optional data On 2016/04/27 16:45:09, May wrote: > On 2016/04/27 07:03:09, Marc Treib wrote: > > So far all of the fields, except for the URL, have been optional. Are we > > changing that? IIRC, we usually don't get expiry_date from ChromeReader for > > example? > > Removed favicon actually, since we don't get that data anymore from > ChromeReader. But to answer the general question: I count as 'optional' fields > that we don't need to display the snippet to the user. In your example, expiry > date is not optional (since we need it to know when to stop showing it to the > user). Also, we calculate that each time so it should never be blank. Given the > insistence that we only get data from ChromeReader that is "perfect", I'm taking > the route of erring on the side of only showing the user something that is > complete. Okay, makes sense, thanks! https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:101: bool is_valid_snippet() const { On 2016/04/27 16:45:09, May wrote: > On 2016/04/27 07:03:09, Marc Treib wrote: > > nit: just "is_valid" please (though IMO "valid" might not be the right term - > > isn't a snippet without publisher still valid for example?) > > Changed to complete(). Better..? Yes, thank you! https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:120: GURL amp_url_; On 2016/04/27 16:45:08, May wrote: > On 2016/04/27 07:03:09, Marc Treib wrote: > > A few fields are now effectively stored twice. Can we get rid of them here, > and > > just store a "best source index" instead? > > Done. I kept url_ because we do have a url from ChromeReader that's separate > from the source info, it's just that we prefer the source info data. Alright, fair enough. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:122: std::vector<SnippetSource> sources_; On 2016/04/27 16:45:08, May wrote: > On 2016/04/27 07:03:09, Marc Treib wrote: > > Or in fact: Do we actually need to store all the sources? Don't we just need > the > > best one? > > I do it so that we can effectively de-dupe later as new snippets come in (see > crbug/603884). We can't guarantee that the sources we receive from ChromeReader > will always be in the same order (or that we have the same exact sources even -- > it's possible that in a later fetch a new source came in for the same article) > so we won't always choose the same best source. > > De-duping not in this CL yet, planning to submit a separate one for that. Acknowledged. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:149: // data we need to display it properly On 2016/04/27 16:45:09, May wrote: > On 2016/04/27 07:03:09, Marc Treib wrote: > > I dislike this very much - IMO doing stuff differently in Release vs Developer > > builds is a Very Bad Thing. Is there any particular reason for this? > > I had a long internal debate about this. Basically what I want to achieve is > that we only show "perfect" snippets to users, but I also want us (devs) to know > right away if something's not right with the data so we can talk to the > ChromeReader guys about it. I thought about adding a flag, but we'd need to have > everyone turn it on. IMO a cmdline flag for devs would be better, yes. Or maybe just log messages if we discard incomplete snippets would be enough? Then the UI would never have to handle incomplete snippets. In any case, we'd also like to know if this happens on users' devices, so I think a UMA histogram to record this would be useful. > Or we could also just add everything for now, but add the if(snippet->valid()) > check before we launch to stable. Hm, given how short-term our launch decisions tend to be, that means it'd have to be controlled via Finch. I don't think that's worth the effort. https://codereview.chromium.org/1921553004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1921553004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:126: ? snippet.best_source().url.spec() Wait, if we don't have an AMP URL, then shouldn't the amp_urls entry stay empty? https://codereview.chromium.org/1921553004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1921553004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:45: entry->SetString("url", snippet.url().spec()); Add amp_url? https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:73: if (value->GetAsDictionary(&dict_value)) { if (!value->GetAsDictionary..) { LOG(); continue; } ? https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:78: if (!corpus_id.is_valid()) { You could move this out of here, and then remove the "else" below. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:106: << amp_url.spec(); amp_url_str https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:124: SnippetSource& best_source_found = snippet->sources_[0]; I'd move this into the loop, then it can be const: const SnippetSource& best_source = snippet->sources_[best_source_index]; (and you don't have to update both index and source below). https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:126: SnippetSource current_source = snippet->sources_[i]; const SnippetSource& ? https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:130: best_source_index = i - 1; Shouldn't best_source_index already have the correct value here?! https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:52: void set_url(const GURL& url) { url_ = url; } I think set_url isn't needed anymore now?
https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:226: String origin = null; On 2016/04/27 16:45:08, May wrote: > On 2016/04/27 13:22:05, Bernhard Bauer wrote: > > I wouldn't call this origin if it's just protocol and host (technically, an > > origin includes the port as well). Also, could we extract this into a method > > that returns the URL to use for the favicon? That might help with dealing with > > some of the error cases here. > > Not sure I understand the second part. We first try to get the cached favicon of > the exact article URL. If that fails, we do this thing where we get the origin > (okay, minus the port) and try to get the favicon for that. Not sure what > extracting that into a method helps with? What I meant was extracting the part that removes the path from the URL into a method, because that has multiple ways to fail (so you could just return null, and have the caller handle that in a single place). But really, some of the error checking here is unnecessary: In line 225, |uri| can't be null (if parsing the URI failed, we'd have returned), and because of that, the origin-ish will also not be empty in line 227. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:295: std::vector<std::string> source_urls; On 2016/04/27 13:22:05, Bernhard Bauer wrote: > I think you could use an initializer list here to construct the vector in one > go? Ping :) https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:300: amp_urls.push_back(std::string("")); On 2016/04/27 13:22:05, Bernhard Bauer wrote: > The empty string literal is unnecessary. Ping :) https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:94: void add_source(SnippetSource& source) { sources_.push_back(source); } Const reference
Also updated tests since I've changed the logic to still store the original url from ChromeReader instead of overwriting it with the source_corpus_id. https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1921553004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:226: String origin = null; On 2016/04/28 14:19:20, Bernhard Bauer wrote: > On 2016/04/27 16:45:08, May wrote: > > On 2016/04/27 13:22:05, Bernhard Bauer wrote: > > > I wouldn't call this origin if it's just protocol and host (technically, an > > > origin includes the port as well). Also, could we extract this into a method > > > that returns the URL to use for the favicon? That might help with dealing > with > > > some of the error cases here. > > > > Not sure I understand the second part. We first try to get the cached favicon > of > > the exact article URL. If that fails, we do this thing where we get the origin > > (okay, minus the port) and try to get the favicon for that. Not sure what > > extracting that into a method helps with? > > What I meant was extracting the part that removes the path from the URL into a > method, because that has multiple ways to fail (so you could just return null, > and have the caller handle that in a single place). > > But really, some of the error checking here is unnecessary: In line 225, |uri| > can't be null (if parsing the URI failed, we'd have returned), and because of > that, the origin-ish will also not be empty in line 227. True, I removed the checks and now there's just one possible error. Hooray? Re: name for that origin-ish string, couldn't decide on a name for the string so just did it in place. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:149: // data we need to display it properly On 2016/04/28 08:50:13, Marc Treib wrote: > On 2016/04/27 16:45:09, May wrote: > > On 2016/04/27 07:03:09, Marc Treib wrote: > > > I dislike this very much - IMO doing stuff differently in Release vs > Developer > > > builds is a Very Bad Thing. Is there any particular reason for this? > > > > I had a long internal debate about this. Basically what I want to achieve is > > that we only show "perfect" snippets to users, but I also want us (devs) to > know > > right away if something's not right with the data so we can talk to the > > ChromeReader guys about it. I thought about adding a flag, but we'd need to > have > > everyone turn it on. > > IMO a cmdline flag for devs would be better, yes. Or maybe just log messages if > we discard incomplete snippets would be enough? Then the UI would never have to > handle incomplete snippets. > Ok, I'll do a cmd line flag. I really want it to be obvious, and I think warning messages in the log are too subtle (we'd never see it). > In any case, we'd also like to know if this happens on users' devices, so I > think a UMA histogram to record this would be useful. Yep, good point. Done. > > > Or we could also just add everything for now, but add the if(snippet->valid()) > > check before we launch to stable. > > Hm, given how short-term our launch decisions tend to be, that means it'd have > to be controlled via Finch. I don't think that's worth the effort. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:295: std::vector<std::string> source_urls; On 2016/04/27 13:22:05, Bernhard Bauer wrote: > I think you could use an initializer list here to construct the vector in one > go? Done. https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:300: amp_urls.push_back(std::string("")); On 2016/04/27 13:22:05, Bernhard Bauer wrote: > The empty string literal is unnecessary. Done. https://codereview.chromium.org/1921553004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1921553004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:126: ? snippet.best_source().url.spec() On 2016/04/28 08:50:13, Marc Treib wrote: > Wait, if we don't have an AMP URL, then shouldn't the amp_urls entry stay empty? Ah, yeah it should. Good catch. https://codereview.chromium.org/1921553004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1921553004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:45: entry->SetString("url", snippet.url().spec()); On 2016/04/28 08:50:14, Marc Treib wrote: > Add amp_url? Done. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:73: if (value->GetAsDictionary(&dict_value)) { On 2016/04/28 08:50:14, Marc Treib wrote: > if (!value->GetAsDictionary..) { LOG(); continue; } > ? Done. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:78: if (!corpus_id.is_valid()) { On 2016/04/28 08:50:14, Marc Treib wrote: > You could move this out of here, and then remove the "else" below. Done. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:106: << amp_url.spec(); On 2016/04/28 08:50:14, Marc Treib wrote: > amp_url_str Done. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:124: SnippetSource& best_source_found = snippet->sources_[0]; On 2016/04/28 08:50:14, Marc Treib wrote: > I'd move this into the loop, then it can be const: > const SnippetSource& best_source = snippet->sources_[best_source_index]; > (and you don't have to update both index and source below). Simplified logic. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:126: SnippetSource current_source = snippet->sources_[i]; On 2016/04/28 08:50:14, Marc Treib wrote: > const SnippetSource& > ? Done. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:130: best_source_index = i - 1; On 2016/04/28 08:50:14, Marc Treib wrote: > Shouldn't best_source_index already have the correct value here?! Good point. Simplified logic. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:52: void set_url(const GURL& url) { url_ = url; } On 2016/04/28 08:50:14, Marc Treib wrote: > I think set_url isn't needed anymore now? Done. https://codereview.chromium.org/1921553004/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:94: void add_source(SnippetSource& source) { sources_.push_back(source); } On 2016/04/28 14:19:20, Bernhard Bauer wrote: > Const reference Done.
Thanks! Mostly LG, one more comment. https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:114: // url for the article. This will also be used to find the favicon for the nitty nit: I'd use "URL" in comments. Also comments should end with a "." https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:127: urls.push_back(snippet.best_source().url.spec()); Hm, I think this is problematic. We currently use the URL as an identifier for the snippet, so I think this will break e.g. dismissing snippets and (soon) fetching images. I think we need to pass both the "main/identifier URL" and the "display/link URL" (or introduce some other identifier).
LGTM w/ nits: https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:149: // data we need to display it properly On 2016/04/28 18:01:52, May wrote: > On 2016/04/28 08:50:13, Marc Treib wrote: > > On 2016/04/27 16:45:09, May wrote: > > > On 2016/04/27 07:03:09, Marc Treib wrote: > > > > I dislike this very much - IMO doing stuff differently in Release vs > > Developer > > > > builds is a Very Bad Thing. Is there any particular reason for this? > > > > > > I had a long internal debate about this. Basically what I want to achieve is > > > that we only show "perfect" snippets to users, but I also want us (devs) to > > know > > > right away if something's not right with the data so we can talk to the > > > ChromeReader guys about it. I thought about adding a flag, but we'd need to > > have > > > everyone turn it on. > > > > IMO a cmdline flag for devs would be better, yes. Or maybe just log messages > if > > we discard incomplete snippets would be enough? Then the UI would never have > to > > handle incomplete snippets. > > > > Ok, I'll do a cmd line flag. I really want it to be obvious, and I think warning > messages in the log are too subtle (we'd never see it). Just as a suggestion (and to add my opinion about the color of the bikeshed): We now have a UI surface with chrome://snippets-internals to show information for developers, and we're going to have the raw response from ChromeReader there. Really, the only thing missing would be highlighting which ones of the snippets failed to validate. > > In any case, we'd also like to know if this happens on users' devices, so I > > think a UMA histogram to record this would be useful. > > Yep, good point. Done. > > > > > > Or we could also just add everything for now, but add the > if(snippet->valid()) > > > check before we launch to stable. > > > > Hm, given how short-term our launch decisions tend to be, that means it'd have > > to be controlled via Finch. I don't think that's worth the effort. > > https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:114: // url for the article. This will also be used to find the favicon for the Nit: "URL" https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:115: // article Nit: period at the end please. https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:117: // url for the amp version of the article if it exists. This will be used as Nit: "AMP"
Also fixed a bug where we were discarding all snippets due to setting the default publish/expiry time too late. https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:114: // url for the article. This will also be used to find the favicon for the On 2016/04/29 09:19:29, Marc Treib wrote: > nitty nit: I'd use "URL" in comments. Also comments should end with a "." Done. https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:115: // article On 2016/04/29 09:26:34, Bernhard Bauer wrote: > Nit: period at the end please. Done. https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:117: // url for the amp version of the article if it exists. This will be used as On 2016/04/29 09:26:34, Bernhard Bauer wrote: > Nit: "AMP" Done. https://codereview.chromium.org/1921553004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:127: urls.push_back(snippet.best_source().url.spec()); On 2016/04/29 09:19:29, Marc Treib wrote: > Hm, I think this is problematic. We currently use the URL as an identifier for > the snippet, so I think this will break e.g. dismissing snippets and (soon) > fetching images. I think we need to pass both the "main/identifier URL" and the > "display/link URL" (or introduce some other identifier). Hmm, good point. Changed DiscardSnippet to use the best_source().url also as a comparator to address the dismissing issue. I imagine the code for deduping snippets will be something like adding a comparator to NTPSnippet so it should be trivial to also address this there. I'll make a note on the bug though so it doesn't fall through the cracks.
maybelle@chromium.org changed reviewers: + jwd@chromium.org
+jwd for metrics OWNERS LGTM, PTAL!
https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:153: // Fill in default publish/expiry dates where required. This will now also run when we're loading snippets from prefs, which we IMO don't want - those should always already have the dates. I think the problem is just with the "incomplete" block that you introduced just below, right? Then I'd move that block into LoadFromListValue, just below where the "default publish/expiry" block used to be. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:316: return snippet->url() == url || snippet->best_source().url; This will just check if |snippet->best_source().url| evaluates to true, you're missing a "== url". I'm surprised this even compiles - can GURL be auto-converted to bool or int?!
https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:153: // Fill in default publish/expiry dates where required. On 2016/04/29 12:11:20, Marc Treib wrote: > This will now also run when we're loading snippets from prefs, which we IMO > don't want - those should always already have the dates. > I think the problem is just with the "incomplete" block that you introduced just > below, right? Then I'd move that block into LoadFromListValue, just below where > the "default publish/expiry" block used to be. We should never store snippets without a publish/expiry date, right? So this should be no-op for those. But I can change it back if you feel strongly about it. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:316: return snippet->url() == url || snippet->best_source().url; On 2016/04/29 12:11:20, Marc Treib wrote: > This will just check if |snippet->best_source().url| evaluates to true, you're > missing a "== url". I'm surprised this even compiles - can GURL be > auto-converted to bool or int?! Ack -- fixed. And apparently it can be turned into a bool/int, because it definitely compiles!
https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:80: if (dict_value->GetString(kCorpusId, &corpus_id_str)) { nit: no braces required https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:85: // We must at least have a valid source URL nit: period after comment, also below https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:124: const SnippetSource& best_source = snippet->sources_[i]; This should be just "source" now, no? It's not the best source (so far), it's just the one we are currently looking at. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:128: // We already have the best possible source "This is the best possible source, stop looking." ? https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:136: return nullptr; log? https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:157: for (const SnippetSource source : sources_) { const SnippetSource& Maybe SnippetSource should be uncopyable, to prevent this kind of thing? Do we actually need to copy them anywhere? https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:153: // Fill in default publish/expiry dates where required. On 2016/04/29 12:19:09, May wrote: > On 2016/04/29 12:11:20, Marc Treib wrote: > > This will now also run when we're loading snippets from prefs, which we IMO > > don't want - those should always already have the dates. > > I think the problem is just with the "incomplete" block that you introduced > just > > below, right? Then I'd move that block into LoadFromListValue, just below > where > > the "default publish/expiry" block used to be. > > We should never store snippets without a publish/expiry date, right? So this > should be no-op for those. But I can change it back if you feel strongly about > it. Yes, please. It "should" be a no-op in those cases, but it has the potential to hide some other bug. I'd like to run this code only in the place where it's actually required, and expected to do something.
https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:149: // data we need to display it properly On 2016/04/29 09:26:34, Bernhard Bauer wrote: > On 2016/04/28 18:01:52, May wrote: > > On 2016/04/28 08:50:13, Marc Treib wrote: > > > On 2016/04/27 16:45:09, May wrote: > > > > On 2016/04/27 07:03:09, Marc Treib wrote: > > > > > I dislike this very much - IMO doing stuff differently in Release vs > > > Developer > > > > > builds is a Very Bad Thing. Is there any particular reason for this? > > > > > > > > I had a long internal debate about this. Basically what I want to achieve > is > > > > that we only show "perfect" snippets to users, but I also want us (devs) > to > > > know > > > > right away if something's not right with the data so we can talk to the > > > > ChromeReader guys about it. I thought about adding a flag, but we'd need > to > > > have > > > > everyone turn it on. > > > > > > IMO a cmdline flag for devs would be better, yes. Or maybe just log messages > > if > > > we discard incomplete snippets would be enough? Then the UI would never have > > to > > > handle incomplete snippets. > > > > > > > Ok, I'll do a cmd line flag. I really want it to be obvious, and I think > warning > > messages in the log are too subtle (we'd never see it). > > Just as a suggestion (and to add my opinion about the color of the bikeshed): We > now have a UI surface with chrome://snippets-internals to show information for > developers, and we're going to have the raw response from ChromeReader there. > Really, the only thing missing would be highlighting which ones of the snippets > failed to validate. > > > > In any case, we'd also like to know if this happens on users' devices, so I > > > think a UMA histogram to record this would be useful. > > > > Yep, good point. Done. > > > > > > > > > Or we could also just add everything for now, but add the > > if(snippet->valid()) > > > > check before we launch to stable. > > > > > > Hm, given how short-term our launch decisions tend to be, that means it'd > have > > > to be controlled via Finch. I don't think that's worth the effort. > > > > > Yeah, that would be a nice additional feature though would require a bit more restructuring since this CL just deletes the incomplete snippets. I'll file a bug. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:80: if (dict_value->GetString(kCorpusId, &corpus_id_str)) { On 2016/04/29 12:33:32, Marc Treib wrote: > nit: no braces required Done. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:85: // We must at least have a valid source URL On 2016/04/29 12:33:32, Marc Treib wrote: > nit: period after comment, also below Done. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:124: const SnippetSource& best_source = snippet->sources_[i]; On 2016/04/29 12:33:32, Marc Treib wrote: > This should be just "source" now, no? It's not the best source (so far), it's > just the one we are currently looking at. Done. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:128: // We already have the best possible source On 2016/04/29 12:33:32, Marc Treib wrote: > "This is the best possible source, stop looking." ? Done. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:136: return nullptr; On 2016/04/29 12:33:32, Marc Treib wrote: > log? Done. https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:157: for (const SnippetSource source : sources_) { On 2016/04/29 12:33:32, Marc Treib wrote: > const SnippetSource& > > Maybe SnippetSource should be uncopyable, to prevent this kind of thing? Do we > actually need to copy them anywhere? Changed to SnippetSource&. I can't think of when we'd need to copy SnippetSource. It probably can be changed to disallow copy, but would require changing the |sources_| vector to do pointers instead I guess. Worth it? https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:153: // Fill in default publish/expiry dates where required. On 2016/04/29 12:33:32, Marc Treib wrote: > On 2016/04/29 12:19:09, May wrote: > > On 2016/04/29 12:11:20, Marc Treib wrote: > > > This will now also run when we're loading snippets from prefs, which we IMO > > > don't want - those should always already have the dates. > > > I think the problem is just with the "incomplete" block that you introduced > > just > > > below, right? Then I'd move that block into LoadFromListValue, just below > > where > > > the "default publish/expiry" block used to be. > > > > We should never store snippets without a publish/expiry date, right? So this > > should be no-op for those. But I can change it back if you feel strongly about > > it. > > Yes, please. It "should" be a no-op in those cases, but it has the potential to > hide some other bug. I'd like to run this code only in the place where it's > actually required, and expected to do something. Sure, moved the snippet discarding logic into LoadFromListValue
LGTM with two more comments below. Thanks for putting up with all my comments! https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.cc (right): https://codereview.chromium.org/1921553004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.cc:157: for (const SnippetSource source : sources_) { On 2016/04/29 17:00:15, May wrote: > On 2016/04/29 12:33:32, Marc Treib wrote: > > const SnippetSource& > > > > Maybe SnippetSource should be uncopyable, to prevent this kind of thing? Do we > > actually need to copy them anywhere? > > Changed to SnippetSource&. > > I can't think of when we'd need to copy SnippetSource. It probably can be > changed to disallow copy, but would require changing the |sources_| vector to do > pointers instead I guess. Worth it? Nope, it can remain a vector of instances, thanks to the magic of C++11 move semantics :) Anyway, that can wait for another CL. https://codereview.chromium.org/1921553004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1921553004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:46: private static final String PUBLISHER_FORMAT_STRING = "%s - %s"; We might want to make this a proper resource string, for i18n? I'm thinking of RTL in particular. But that can wait for another CL. https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:429: [this](const std::unique_ptr<NTPSnippet>& snippet) { No need to capture |this| here. https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:434: if (num_snippets_discarded > 0) I think we want to record this even if it's zero, so that we can compute the percentage of cases where something goes wrong?
https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:434: if (num_snippets_discarded > 0) On 2016/04/30 13:51:19, Marc Treib wrote: > I think we want to record this even if it's zero, so that we can compute the > percentage of cases where something goes wrong? I'd suggest keeping this histogram as it is, and adding a 2nd histogram that's a boolean histogram and records if snippets were discarded or not. You can then compute ratios from the dashboard side. I suggest this so you can more easily do analysis on only the data when things were discarded. https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:435: UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.IncompleteSnippets", How many unique num_snippets_discarded do you expect to see in the full population? If it's too many, the aggregated data will be unmanageable. https://codereview.chromium.org/1921553004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1921553004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:32781: +<histogram name="NewTabPage.Snippets.IncompleteSnippets"> Nit: can you add a unit, probably snippets.
https://codereview.chromium.org/1921553004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1921553004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:46: private static final String PUBLISHER_FORMAT_STRING = "%s - %s"; On 2016/04/30 13:51:19, Marc Treib wrote: > We might want to make this a proper resource string, for i18n? I'm thinking of > RTL in particular. But that can wait for another CL. Hmm, good point. Filed crbug/608680 to track this (and a related bug/improvement to this string) https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:429: [this](const std::unique_ptr<NTPSnippet>& snippet) { On 2016/04/30 13:51:19, Marc Treib wrote: > No need to capture |this| here. Done. https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:434: if (num_snippets_discarded > 0) On 2016/05/02 16:00:28, Jesse Doherty wrote: > On 2016/04/30 13:51:19, Marc Treib wrote: > > I think we want to record this even if it's zero, so that we can compute the > > percentage of cases where something goes wrong? > > I'd suggest keeping this histogram as it is, and adding a 2nd histogram that's a > boolean histogram and records if snippets were discarded or not. You can then > compute ratios from the dashboard side. > > I suggest this so you can more easily do analysis on only the data when things > were discarded. Done. https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:435: UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.IncompleteSnippets", On 2016/05/02 16:00:28, Jesse Doherty wrote: > How many unique num_snippets_discarded do you expect to see in the full > population? If it's too many, the aggregated data will be unmanageable. Up to kMaxSnippetCount (=10 at the moment) can be discarded. Discarding any snippets should be a rare event, we expect to not discard any most of the time but would like to know when it does happen. https://codereview.chromium.org/1921553004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1921553004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:32781: +<histogram name="NewTabPage.Snippets.IncompleteSnippets"> On 2016/05/02 16:00:28, Jesse Doherty wrote: > Nit: can you add a unit, probably snippets. Done.
LGTM for histograms https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:435: UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.IncompleteSnippets", On 2016/05/03 17:11:00, May wrote: > On 2016/05/02 16:00:28, Jesse Doherty wrote: > > How many unique num_snippets_discarded do you expect to see in the full > > population? If it's too many, the aggregated data will be unmanageable. > > Up to kMaxSnippetCount (=10 at the moment) can be discarded. Discarding any > snippets should be a rare event, we expect to not discard any most of the time > but would like to know when it does happen. Sounds fine, as long as you're ok with the overhead from the sparse histogram (I'm assuming you are).
https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:436: UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.SnippetsDiscardedAfterFetch", I wouldn't use "Discarded" here, since that term is also user for "discarded by the user". Any particular reason for having a separate boolean histogram, instead of just unconditionally recording .IncompleteSnippets? https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:438: if (num_snippets_discarded > 0) nit: braces if the body doesn't fit on one line
https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:436: UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.SnippetsDiscardedAfterFetch", On 2016/05/04 07:42:43, Marc Treib wrote: > I wouldn't use "Discarded" here, since that term is also user for "discarded by > the user". > Any particular reason for having a separate boolean histogram, instead of just > unconditionally recording .IncompleteSnippets? Recommended by jwd@ to just have a separate boolean for this to use as the denominator for figuring out % of snippet fetches that have issues Changed name to IncompleteSnippetsAfterFetch, and changed the other one to NumIncompleteSnippets to clarify that it's a count. https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:438: if (num_snippets_discarded > 0) On 2016/05/04 07:42:43, Marc Treib wrote: > nit: braces if the body doesn't fit on one line Done.
lgtm https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:436: UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.SnippetsDiscardedAfterFetch", On 2016/05/04 09:00:06, May wrote: > On 2016/05/04 07:42:43, Marc Treib wrote: > > I wouldn't use "Discarded" here, since that term is also user for "discarded > by > > the user". > > Any particular reason for having a separate boolean histogram, instead of just > > unconditionally recording .IncompleteSnippets? > > Recommended by jwd@ to just have a separate boolean for this to use as the > denominator for figuring out % of snippet fetches that have issues Couldn't we do that anyway by comparing the number in the "0" bucket to the total number of samples? Anyway, I don't have strong feelings, I was just wondering. > Changed name to IncompleteSnippetsAfterFetch, and changed the other one to > NumIncompleteSnippets to clarify that it's a count. SGTM, thanks!
https://codereview.chromium.org/1921553004/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1921553004/diff/120001/chrome/browser/android... chrome/browser/android/ntp/ntp_snippets_bridge.cc:118: // the url to direct the user to on tap. Nit: URL please :) https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:436: UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.SnippetsDiscardedAfterFetch", On 2016/05/04 07:42:43, Marc Treib wrote: > I wouldn't use "Discarded" here, since that term is also user for "discarded by > the user". Suggestion for consistency: Use "dismissed" for snippets swiped away by the user, and "discarded" for the incomplete ones thrown away by us? > Any particular reason for having a separate boolean histogram, instead of just > unconditionally recording .IncompleteSnippets?
https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:436: UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.SnippetsDiscardedAfterFetch", On 2016/05/04 09:06:54, Bernhard Bauer wrote: > On 2016/05/04 07:42:43, Marc Treib wrote: > > I wouldn't use "Discarded" here, since that term is also user for "discarded > by > > the user". > > Suggestion for consistency: Use "dismissed" for snippets swiped away by the > user, and "discarded" for the incomplete ones thrown away by us? > > > Any particular reason for having a separate boolean histogram, instead of just > > unconditionally recording .IncompleteSnippets? > Ah indeed, I mixed up "dismissed" and "discarded". Still, I like "Incomplete" more since it's less ambiguous. Also, the bike shed should be sky blue.
https://codereview.chromium.org/1921553004/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/1921553004/diff/120001/chrome/browser/android... chrome/browser/android/ntp/ntp_snippets_bridge.cc:118: // the url to direct the user to on tap. On 2016/05/04 09:06:54, Bernhard Bauer wrote: > Nit: URL please :) Done. https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1921553004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:436: UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.SnippetsDiscardedAfterFetch", On 2016/05/04 09:08:29, Marc Treib wrote: > On 2016/05/04 09:06:54, Bernhard Bauer wrote: > > On 2016/05/04 07:42:43, Marc Treib wrote: > > > I wouldn't use "Discarded" here, since that term is also user for "discarded > > by > > > the user". > > > > Suggestion for consistency: Use "dismissed" for snippets swiped away by the > > user, and "discarded" for the incomplete ones thrown away by us? > > > > > Any particular reason for having a separate boolean histogram, instead of > just > > > unconditionally recording .IncompleteSnippets? > > > > Ah indeed, I mixed up "dismissed" and "discarded". Still, I like "Incomplete" > more since it's less ambiguous. > Also, the bike shed should be sky blue. Stop painting my bike shed!! I don't even use a bike!! Also, executive decision to use "incomplete" as much as grammatically possible for this scenario, and "dismissed" for the other ones.
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1921553004/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921553004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921553004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1921553004/#ps160001 (title: "Fix junit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921553004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921553004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maybelle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1921553004/#ps180001 (title: "Change unit test to use non-C++ 11 STL functions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921553004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921553004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add favicon and publisher name to snippet cards BUG=596410 ========== to ========== Add favicon and publisher name to snippet cards BUG=596410 Committed: https://crrev.com/f7360bf8adcd2455a07dbf39f41f33b6bc194899 Cr-Commit-Position: refs/heads/master@{#391493} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f7360bf8adcd2455a07dbf39f41f33b6bc194899 Cr-Commit-Position: refs/heads/master@{#391493} |
