Add favicon and publisher name to snippet cards

components/ntp_snippets/ntp_snippets_service.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 127 matching lines @@
       return false;
 
     const base::DictionaryValue* content = nullptr;
     if (!dict->GetDictionary(kContentInfo, &content))
       return false;
     std::unique_ptr<NTPSnippet> snippet =
         NTPSnippet::CreateFromDictionary(*content);
     if (!snippet)
       return false;
 
-    snippets->push_back(std::move(snippet));
+// If we're running a release build, only add snippets that have all the
+// data we need to display it properly

[Marc Treib, 2016/04/27 07:03:09]

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?

[May, 2016/04/27 16:45:09]

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?

[Marc Treib, 2016/04/28 08:50:13]

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.
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.

[May, 2016/04/28 18:01:52]

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).
Yep, good point. Done.

[Bernhard Bauer, 2016/04/29 09:26:34]

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.

[May, 2016/04/29 17:00:14]

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.

+#if !DCHECK_IS_ON()
+    if (snippet->is_valid_snippet())
+#endif
+      snippets->push_back(std::move(snippet));
   }
   return true;
 }
 
 std::unique_ptr<base::ListValue> SnippetsToListValue(
     const NTPSnippetsService::NTPSnippetStorage& snippets) {
   std::unique_ptr<base::ListValue> list(new base::ListValue);
   for (const auto& snippet : snippets) {
     std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
     dict->Set(kContentInfo, snippet->ToDictionary());
@@ skipping 351 matching lines @@
     if (snippet->expiry_date() < next_expiry)
       next_expiry = snippet->expiry_date();
   }
   DCHECK_GT(next_expiry, expiry);
   expiry_timer_.Start(FROM_HERE, next_expiry - expiry,
                       base::Bind(&NTPSnippetsService::LoadingSnippetsFinished,
                                  base::Unretained(this)));
 }
 
 }  // namespace ntp_snippets