[NTP::PhysicalWeb] Wire Physical Web provider to the data source.

components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc

Index: components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
diff --git a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
index 475e3c88e5c2c0cad3891747cd974bfc3f7f6e88..ca0f223d8e2a9c6cd2d90d173994085f8e9b3d18 100644
--- a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
+++ b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
@@ -4,6 +4,8 @@
 
 #include "components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h"
 
+#include <algorithm>
+#include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ -14,6 +16,9 @@
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/image/image.h"
+#include "url/gurl.h"
+
+using base::Value;

[Marc Treib, 2016/11/23 09:47:52]

The "base::" is spelled out for ListValue and DictionaryValue, please be
consistent and either do "using" for all or none.

[vitaliii, 2016/11/23 14:16:52]

Done.

 
 namespace ntp_snippets {
 
@@ -23,46 +28,23 @@ const size_t kMaxSuggestionsCount = 10;
 
 }  // namespace
 
-// TODO(vitaliii): remove when Physical Web C++ interface is provided.
-UrlInfo::UrlInfo() = default;
-UrlInfo::~UrlInfo() = default;
-UrlInfo::UrlInfo(const UrlInfo& other) = default;
-
 PhysicalWebPageSuggestionsProvider::PhysicalWebPageSuggestionsProvider(
     ContentSuggestionsProvider::Observer* observer,
-    CategoryFactory* category_factory)
+    CategoryFactory* category_factory,
+    PhysicalWebDataSource* physical_web_data_source)
     : ContentSuggestionsProvider(observer, category_factory),
       category_status_(CategoryStatus::AVAILABLE_LOADING),

[Marc Treib, 2016/11/23 09:47:52]

The status is immediately set to AVAILABLE in FetchPhysicalWebPages, might as
well do that right away and get rid of _LOADING?

[vitaliii, 2016/11/23 14:16:53]

Done.

       provided_category_(category_factory->FromKnownCategory(
-          KnownCategories::PHYSICAL_WEB_PAGES)) {
+          KnownCategories::PHYSICAL_WEB_PAGES)),
+      physical_web_data_source_(physical_web_data_source) {
   observer->OnCategoryStatusChanged(this, provided_category_, category_status_);
+  physical_web_data_source_->RegisterListener(this);
+  // TODO(vitaliii): Rewrite initial fetch once crbug.com/667754 is resolved.
+  FetchPhysicalWebPages();
 }
 
-PhysicalWebPageSuggestionsProvider::~PhysicalWebPageSuggestionsProvider() =
-    default;
-
-void PhysicalWebPageSuggestionsProvider::OnDisplayableUrlsChanged(
-    const std::vector<UrlInfo>& urls) {
-  NotifyStatusChanged(CategoryStatus::AVAILABLE);
-  std::vector<ContentSuggestion> suggestions;
-
-  for (const UrlInfo& url_info : urls) {
-    if (suggestions.size() >= kMaxSuggestionsCount) {
-      break;
-    }
-
-    ContentSuggestion suggestion(provided_category_, url_info.site_url.spec(),
-                                 url_info.site_url);
-
-    suggestion.set_title(base::UTF8ToUTF16(url_info.title));
-    suggestion.set_snippet_text(base::UTF8ToUTF16(url_info.description));
-    suggestion.set_publish_date(url_info.scan_time);
-    suggestion.set_publisher_name(base::UTF8ToUTF16(url_info.site_url.host()));
-    suggestions.push_back(std::move(suggestion));
-  }
-
-  observer()->OnNewSuggestions(this, provided_category_,
-                               std::move(suggestions));
+PhysicalWebPageSuggestionsProvider::~PhysicalWebPageSuggestionsProvider() {
+  physical_web_data_source_->UnregisterListener(this);
 }
 
 CategoryStatus PhysicalWebPageSuggestionsProvider::GetCategoryStatus(
@@ -72,10 +54,12 @@ CategoryStatus PhysicalWebPageSuggestionsProvider::GetCategoryStatus(
 
 CategoryInfo PhysicalWebPageSuggestionsProvider::GetCategoryInfo(
     Category category) {
-  // TODO(vitaliii): Use the proper strings once they've been agreed on.
+  // TODO(vitaliii): Use the proper string once it has been agreed on.
+  // TODO(vitaliii): Use a translateable string. (crbug.com/667764)
+  // TODO(vitaliii): Implement More action. (crbug.com/667759)
   return CategoryInfo(
       base::ASCIIToUTF16("Physical web pages"),
-      ContentSuggestionsCardLayout::MINIMAL_CARD,
+      ContentSuggestionsCardLayout::FULL_CARD,
       /*has_more_action=*/false,
       /*has_reload_action=*/false,
       /*has_view_all_action=*/false,
@@ -86,13 +70,13 @@ CategoryInfo PhysicalWebPageSuggestionsProvider::GetCategoryInfo(
 void PhysicalWebPageSuggestionsProvider::DismissSuggestion(
     const ContentSuggestion::ID& suggestion_id) {
   // TODO(vitaliii): Implement this and then
-  // ClearDismissedSuggestionsForDebugging.
+  // ClearDismissedSuggestionsForDebugging. (crbug.com/667766)
 }
 
 void PhysicalWebPageSuggestionsProvider::FetchSuggestionImage(
     const ContentSuggestion::ID& suggestion_id,
     const ImageFetchedCallback& callback) {
-  // TODO(vitaliii): Implement.
+  // TODO(vitaliii): Implement. (crbug.com/667765)
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback, gfx::Image()));
 }
@@ -138,7 +122,6 @@ void PhysicalWebPageSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
-// Updates the |category_status_| and notifies the |observer_|, if necessary.
 void PhysicalWebPageSuggestionsProvider::NotifyStatusChanged(
     CategoryStatus new_status) {
   if (category_status_ == new_status) {
@@ -148,4 +131,88 @@ void PhysicalWebPageSuggestionsProvider::NotifyStatusChanged(
   observer()->OnCategoryStatusChanged(this, provided_category_, new_status);
 }
 
+void PhysicalWebPageSuggestionsProvider::FetchPhysicalWebPages() {
+  NotifyStatusChanged(CategoryStatus::AVAILABLE);
+  std::unique_ptr<base::ListValue> page_values =
+      physical_web_data_source_->GetMetadata();
+
+  std::vector<const base::DictionaryValue*> page_dictionaries;
+  for (const std::unique_ptr<Value>& page_value : *page_values) {
+    const base::DictionaryValue* page_dictionary;
+    bool success = page_value->GetAsDictionary(&page_dictionary);
+    DCHECK(success);

[Marc Treib, 2016/11/23 09:47:52]

Are all the DCHECKs in this method really DCHECKs? Do we want to crash if we get
invalid data from physical web?
(IMO if there are such strong constraints on the data, then it should be
provided in a struct, not in a dictionary.)

[vitaliii, 2016/11/23 14:16:52]

We definitely can live without a title or a description, but I would like to
know when this happens.
What are the downsides of DCHECKs here? My assumption is that only developer
build may crash and before the feature is enabled by default, this means only
mine.

(IMO a struct is definitely better, but the first chance to express this will be
only on Monday and in case I am wrong - crbug.com/668141)

[Marc Treib, 2016/11/23 14:22:45]

Per the style guide, DCHECKs are treated as asserts - something that can *never*
happen.

LOG(DFATAL) might serve your needs? Or even just DLOG(ERROR).

[vitaliii, 2016/11/23 16:00:54]

Done.

+    page_dictionaries.push_back(page_dictionary);
+  }
+
+  std::sort(page_dictionaries.begin(), page_dictionaries.end(),
+            [](const base::DictionaryValue* left,
+               const base::DictionaryValue* right) {
+              double left_distance, right_distance;
+              bool success = left->GetDouble(kPhysicalWebDistanceEstimateKey,
+                                             &left_distance);
+              DCHECK(success);
+              success = right->GetDouble(kPhysicalWebDistanceEstimateKey,
+                                         &right_distance);
+              DCHECK(success);
+              return left_distance < right_distance;
+            });
+
+  std::vector<ContentSuggestion> suggestions;
+  for (const base::DictionaryValue* page_dictionary : page_dictionaries) {
+    suggestions.push_back(ConvertPhysicalWebPage(page_dictionary));
+    if (suggestions.size() == kMaxSuggestionsCount)
+      break;

[Marc Treib, 2016/11/23 09:47:52]

nit: braces now :)

[vitaliii, 2016/11/23 14:16:52]

Done.
This might have been written before we decided :)

+  }
+
+  observer()->OnNewSuggestions(this, provided_category_,
+                               std::move(suggestions));
+}
+
+ContentSuggestion PhysicalWebPageSuggestionsProvider::ConvertPhysicalWebPage(
+    const base::DictionaryValue* page) const {
+  bool success;
+  std::string scanned_url;
+  success = page->GetString(kPhysicalWebScannedUrlKey, &scanned_url);

[Marc Treib, 2016/11/23 09:47:52]

Oh wow, they have global constants without a namespace...

[vitaliii, 2016/11/23 14:16:52]

Acknowledged.

+  DCHECK(success);
+  int scan_timestamp;
+  success = page->GetInteger(kPhysicalWebScanTimestampKey, &scan_timestamp);
+  DCHECK(success);
+  std::string raw_resolved_url;
+  success = page->GetString(kPhysicalWebResolvedUrlKey, &raw_resolved_url);
+  DCHECK(success);
+  std::string title;
+  success = page->GetString(kPhysicalWebTitleKey, &title);
+  DCHECK(success);
+  std::string description;
+  success = page->GetString(kPhysicalWebDescriptionKey, &description);
+  DCHECK(success);
+
+  const GURL resolved_url(raw_resolved_url);
+  ContentSuggestion suggestion(provided_category_, scanned_url, resolved_url);
+  suggestion.set_title(base::UTF8ToUTF16(title));
+  // TODO(vitaliii): Set the time properly once the proper value is provided
+  // (see crbug.com/667722).
+  suggestion.set_publish_date(
+      base::Time::FromTimeT(static_cast<time_t>(scan_timestamp)));
+  DCHECK(base::IsStringUTF8(resolved_url.host()));

[Marc Treib, 2016/11/23 09:47:52]

Isn't this guaranteed anyway if the GURL parsed without errors?

[vitaliii, 2016/11/23 14:16:52]

Done.
You are right, "... the host, path, etc. will be guaranteed ASCII ...".

+  suggestion.set_publisher_name(base::UTF8ToUTF16(resolved_url.host()));
+  DCHECK(base::IsStringUTF8(description));
+  suggestion.set_snippet_text(base::UTF8ToUTF16(description));
+  return suggestion;
+}
+
+// PhysicalWebListener implementation.
+void PhysicalWebPageSuggestionsProvider::OnFound(const std::string& url) {
+  FetchPhysicalWebPages();
+}
+void PhysicalWebPageSuggestionsProvider::OnLost(const std::string& url) {
+  // TODO(vitaliii): Do not refetch, but just update the current state.
+  FetchPhysicalWebPages();
+}
+void PhysicalWebPageSuggestionsProvider::OnDistanceChanged(

[Marc Treib, 2016/11/23 09:47:52]

nit: empty lines between method implementations

[vitaliii, 2016/11/23 14:16:52]

Done.

+    const std::string& url,
+    double distance_estimate) {
+  FetchPhysicalWebPages();
+}
+
 }  // namespace ntp_snippets