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

components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc

 // Copyright 2016 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/physical_web_pages/physical_web_page_suggestions_provider.h"
 
+#include <algorithm>
+#include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #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 {
 
 namespace {
 
 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(
     Category category) {
   return category_status_;
 }
 
 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,
       /*show_if_empty=*/false,
       l10n_util::GetStringUTF16(IDS_NTP_SUGGESTIONS_SECTION_EMPTY));
 }
 
 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()));
 }
 
 void PhysicalWebPageSuggestionsProvider::Fetch(
     const Category& category,
     const std::set<std::string>& known_suggestion_ids,
     const FetchDoneCallback& callback) {
   LOG(DFATAL)
       << "PhysicalWebPageSuggestionsProvider has no |Fetch| functionality!";
@@ skipping 25 matching lines @@
 }
 
 void PhysicalWebPageSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
     Category category) {
   // TODO(vitaliii): Implement when dismissed suggestions are supported.
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
-// Updates the |category_status_| and notifies the |observer_|, if necessary.
 void PhysicalWebPageSuggestionsProvider::NotifyStatusChanged(
     CategoryStatus new_status) {
   if (category_status_ == new_status) {
     return;
   }
   category_status_ = new_status;
   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