Add ContentSuggestionsService to SnippetsInternals

chrome/browser/ui/webui/snippets_internals_message_handler.cc

Index: chrome/browser/ui/webui/snippets_internals_message_handler.cc
diff --git a/chrome/browser/ui/webui/snippets_internals_message_handler.cc b/chrome/browser/ui/webui/snippets_internals_message_handler.cc
index f6e7411baf0ca1ad6ad5cf19a64dda6b3303b449..707cb06c8524f81f6727146588bcd5556a4c31a6 100644
--- a/chrome/browser/ui/webui/snippets_internals_message_handler.cc
+++ b/chrome/browser/ui/webui/snippets_internals_message_handler.cc
@@ -18,12 +18,17 @@
 #include "base/strings/string_split.h"
 #include "base/values.h"
 #include "chrome/browser/android/chrome_feature_list.h"
+#include "chrome/browser/ntp_snippets/content_suggestions_service_factory.h"
 #include "chrome/browser/ntp_snippets/ntp_snippets_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/ntp_snippets/ntp_snippet.h"
 #include "components/ntp_snippets/switches.h"
 #include "content/public/browser/web_ui.h"
 
+using ntp_snippets::ContentSuggestion;
+using ntp_snippets::ContentSuggestionsCategory;
+using ntp_snippets::ContentSuggestionsCategoryStatus;
+
 namespace {
 
 std::unique_ptr<base::DictionaryValue> PrepareSnippet(
@@ -52,12 +57,68 @@ std::unique_ptr<base::DictionaryValue> PrepareSnippet(
   return entry;
 }
 
+std::unique_ptr<base::DictionaryValue> PrepareSuggestion(
+    const ntp_snippets::ContentSuggestion& suggestion,

[Marc Treib, 2016/07/12 13:04:52]

nit: ntp_snippets:: not needed, you have a "using" above

[Philipp Keck, 2016/07/12 13:45:02]

Done.

+    int index) {
+  std::unique_ptr<base::DictionaryValue> entry(new base::DictionaryValue);

[Bernhard Bauer, 2016/07/12 13:09:45]

You might be able to do `auto entry = base::MakeUnique<base::DictionaryValue>`?

[Philipp Keck, 2016/07/12 13:45:02]

Done.

+  entry->SetString("suggestionId", suggestion.id());
+  entry->SetString("url", suggestion.url().spec());
+  entry->SetString("ampUrl", suggestion.amp_url().spec());
+  entry->SetString("title", suggestion.title());
+  entry->SetString("snippetText", suggestion.snippet_text());
+  entry->SetString("publishDate",
+                   TimeFormatShortDateAndTime(suggestion.publish_date()));
+  entry->SetString("publisherName", suggestion.publisher_name());
+  entry->SetString("id", "content-suggestion-" + base::IntToString(index));
+  return entry;
+}
+
+std::string MapCategoryName(ContentSuggestionsCategory category) {
+  switch (category) {
+    case ContentSuggestionsCategory::ARTICLES:
+      return "Articles";
+    default:

[Bernhard Bauer, 2016/07/12 13:09:45]

If you remove the default case from the switch statement, the compiler will warn
(which is then turned into an error) if you forget a value, which might come in
handy when you add more categories.

[Philipp Keck, 2016/07/12 13:45:02]

Done. Oh great. I didn't know it would warn and that's exactly why I added the
default in the first place (even though it's currently dead code).

So I can remove it here, too, I assume:
https://codereview.chromium.org/2131943002/diff/200001/components/ntp_snippet...

And now there is a case for the "COUNT" dummy value...

[Bernhard Bauer, 2016/07/12 14:44:51]

Indeed!
Yup, that's one of the disadvantages, but reeallly, the COUNT value shouldn't be
part of the enum... :)

I think you'll have to move the old default handling code outside of the switch
now though (you can keep the section for COUNT empty), otherwise the compiler
will complain that you're missing a return statement.

[Philipp Keck, 2016/07/12 15:19:56]

Done. (other CL)
If the COUNT value wasn't there, how would you check whether a given "int" is a
valid value for the enum?

(Not sure if the empty COUNT section with the return statement after the switch
is more desirable than the current code -- but not having a COUNT section at all
would be nice.)

[Bernhard Bauer, 2016/07/12 15:50:31]

You could have an int (not an enum) that represents the maximum value (or the
maximum + 1, or something). I don't know of a way at the moment though to make
that work automatically, so my comment was more hypothetical. Sorry for not
making that clear!

[Philipp Keck, 2016/07/12 16:05:36]

You'd always have to remember to update that int, though, when you change the
contents of the enum. In my opinion, C++ should support sth like that out of the
box, at least with their "enum class" ...
The only solutions without compromises that I saw on the web use macros.

I'll leave the code here as is (not as seen here, but there:
https://codereview.chromium.org/2145563002/diff/40001/chrome/browser/ui/webui...)

[Bernhard Bauer, 2016/07/12 16:16:27]

Yes, precisely.

+      return "Unknown category " + base::IntToString(int(category));
+  }
+}
+
+std::string MapCategoryStatus(ContentSuggestionsCategoryStatus status) {
+  switch (status) {
+    case ContentSuggestionsCategoryStatus::INITIALIZING:
+      return "INITIALIZING";
+    case ContentSuggestionsCategoryStatus::AVAILABLE:
+      return "AVAILABLE";
+    case ContentSuggestionsCategoryStatus::AVAILABLE_LOADING:
+      return "AVAILABLE_LOADING";
+    case ContentSuggestionsCategoryStatus::NOT_PROVIDED:
+      return "NOT_PROVIDED";
+    case ContentSuggestionsCategoryStatus::ALL_SUGGESTIONS_EXPLICITLY_DISABLED:
+      return "ALL_SUGGESTIONS_EXPLICITLY_DISABLED";
+    case ContentSuggestionsCategoryStatus::CATEGORY_EXPLICITLY_DISABLED:
+      return "CATEGORY_EXPLICITLY_DISABLED";
+    case ContentSuggestionsCategoryStatus::SIGNED_OUT:
+      return "SIGNED_OUT";
+    case ContentSuggestionsCategoryStatus::SYNC_DISABLED:
+      return "SYNC_DISABLED";
+    case ContentSuggestionsCategoryStatus::PASSPHRASE_ENCRYPTION_ENABLED:
+      return "PASSPHRASE_ENCRYPTION_ENABLED";
+    case ContentSuggestionsCategoryStatus::HISTORY_SYNC_DISABLED:
+      return "HISTORY_SYNC_DISABLED";
+    case ContentSuggestionsCategoryStatus::LOADING_ERROR:
+      return "LOADING_ERROR";
+    default:

[Bernhard Bauer, 2016/07/12 13:09:45]

Same here.

[Philipp Keck, 2016/07/12 13:45:02]

Done.

+      return "Unknown status: " + base::IntToString(int(status));

[Bernhard Bauer, 2016/07/12 13:09:45]

Should this be a static_cast<int>? The style guide prefers those over C-style
casts, but then again this isn't really a cast at all... If static_cast<int>
isn't outright wrong here, I would go with that, I think, just for sake of being
consistent with the majority of Chrome code.

[Philipp Keck, 2016/07/12 13:45:02]

I delete this line anyway, but I use `int(...)` and `SomeEnumsName(someInt)`
elsewhere, so I could change those for consistency.

See also the discussion here:
https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets...

[Bernhard Bauer, 2016/07/12 14:44:51]

Hm. I do prefer static_cast<>, but I don't want to reopen that discussion
either, so I would go with consistency and either change the other places, or
leave things as they are. 

[Philipp Keck, 2016/07/12 15:19:56]

Done. Actually the style guide is quite clear on this, I changed it to
static_cast<> on the other CL (the C-style cast was deleted here anyway).

+  }
+}
+
 } // namespace
 
 SnippetsInternalsMessageHandler::SnippetsInternalsMessageHandler()
-    : observer_(this),
+    : ntp_snippets_service_observer_(this),
+      content_suggestions_service_observer_(this),
       dom_loaded_(false),
-      ntp_snippets_service_(nullptr) {}
+      ntp_snippets_service_(nullptr),
+      content_suggestions_service_(nullptr) {}
 
 SnippetsInternalsMessageHandler::~SnippetsInternalsMessageHandler() {}
 
@@ -77,11 +138,31 @@ void SnippetsInternalsMessageHandler::NTPSnippetsServiceLoaded() {
 void SnippetsInternalsMessageHandler::NTPSnippetsServiceDisabledReasonChanged(
     ntp_snippets::DisabledReason disabled_reason) {}
 
+void SnippetsInternalsMessageHandler::OnNewSuggestions() {
+  if (!dom_loaded_)
+    return;
+  SendContentSuggestions();
+}
+void SnippetsInternalsMessageHandler::OnCategoryStatusChanged(

[Marc Treib, 2016/07/12 13:04:53]

nit: newlines between methods please

[Philipp Keck, 2016/07/12 13:45:02]

Done.

+    ContentSuggestionsCategory category,
+    ContentSuggestionsCategoryStatus new_status) {
+  if (!dom_loaded_)
+    return;
+  SendContentSuggestions();
+}
+void SnippetsInternalsMessageHandler::ContentSuggestionsServiceShutdown() {}
+
 void SnippetsInternalsMessageHandler::RegisterMessages() {
   // additional initialization (web_ui() does not work from the constructor)
-  ntp_snippets_service_ = NTPSnippetsServiceFactory::GetInstance()->
-      GetForProfile(Profile::FromWebUI(web_ui()));
-  observer_.Add(ntp_snippets_service_);
+  Profile* profile = Profile::FromWebUI(web_ui());
+
+  ntp_snippets_service_ =
+      NTPSnippetsServiceFactory::GetInstance()->GetForProfile(profile);
+  ntp_snippets_service_observer_.Add(ntp_snippets_service_);
+
+  content_suggestions_service_ =
+      ContentSuggestionsServiceFactory::GetInstance()->GetForProfile(profile);
+  content_suggestions_service_observer_.Add(content_suggestions_service_);
 
   web_ui()->RegisterMessageCallback(
       "loaded",
@@ -100,6 +181,17 @@ void SnippetsInternalsMessageHandler::RegisterMessages() {
       "clearDiscarded",
       base::Bind(&SnippetsInternalsMessageHandler::HandleClearDiscarded,
                  base::Unretained(this)));
+
+  web_ui()->RegisterMessageCallback(
+      "clearCachedSuggestions",
+      base::Bind(&SnippetsInternalsMessageHandler::HandleClearCachedSuggestions,
+                 base::Unretained(this)));
+
+  web_ui()->RegisterMessageCallback(
+      "clearDiscardedSuggestions",
+      base::Bind(
+          &SnippetsInternalsMessageHandler::HandleClearDiscardedSuggestions,
+          base::Unretained(this)));
 }
 
 void SnippetsInternalsMessageHandler::HandleLoaded(
@@ -141,6 +233,20 @@ void SnippetsInternalsMessageHandler::HandleDownload(
   ntp_snippets_service_->FetchSnippetsFromHosts(hosts);
 }
 
+void SnippetsInternalsMessageHandler::HandleClearCachedSuggestions(
+    const base::ListValue* args) {
+  DCHECK_EQ(0u, args->GetSize());
+
+  content_suggestions_service_->ClearCachedSuggestionsForDebugging();
+}
+
+void SnippetsInternalsMessageHandler::HandleClearDiscardedSuggestions(
+    const base::ListValue* args) {
+  DCHECK_EQ(0u, args->GetSize());
+
+  content_suggestions_service_->ClearDiscardedSuggestionsForDebugging();
+}
+
 void SnippetsInternalsMessageHandler::SendInitialData() {
   SendHosts();
 
@@ -170,6 +276,7 @@ void SnippetsInternalsMessageHandler::SendInitialData() {
 
   SendSnippets();
   SendDiscardedSnippets();
+  SendContentSuggestions();
 }
 
 void SnippetsInternalsMessageHandler::SendSnippets() {
@@ -222,6 +329,36 @@ void SnippetsInternalsMessageHandler::SendHosts() {
       "chrome.SnippetsInternals.receiveHosts", result);
 }
 
+void SnippetsInternalsMessageHandler::SendContentSuggestions() {
+  std::unique_ptr<base::ListValue> categories_list(new base::ListValue);
+
+  int index = 0;
+  for (ContentSuggestionsCategory category :
+       content_suggestions_service_->GetCategories()) {
+    ContentSuggestionsCategoryStatus status =
+        content_suggestions_service_->GetCategoryStatus(category);
+    const std::vector<ContentSuggestion>& suggestions =
+        content_suggestions_service_->GetSuggestionsForCategory(category);
+
+    std::unique_ptr<base::ListValue> suggestions_list(new base::ListValue);
+    for (const ContentSuggestion& suggestion : suggestions) {
+      suggestions_list->Append(PrepareSuggestion(suggestion, index++));
+    }
+
+    std::unique_ptr<base::DictionaryValue> category_entry(
+        new base::DictionaryValue);
+    category_entry->SetString("name", MapCategoryName(category));
+    category_entry->SetString("status", MapCategoryStatus(status));
+    category_entry->Set("suggestions", std::move(suggestions_list));
+    categories_list->Append(std::move(category_entry));
+  }
+
+  base::DictionaryValue result;
+  result.Set("list", std::move(categories_list));
+  web_ui()->CallJavascriptFunctionUnsafe(
+      "chrome.SnippetsInternals.receiveContentSuggestions", result);
+}
+
 void SnippetsInternalsMessageHandler::SendBoolean(const std::string& name,
                                                   bool value) {
   SendString(name, value ? "True" : "False");