[NTP::PhysicalWeb] Filter our pages with same group_id.

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 <memory>
 #include <set>
 #include <string>
@@ skipping 19 matching lines @@
 using base::DictionaryValue;
 using base::ListValue;
 using base::Value;
 
 namespace ntp_snippets {
 
 namespace {
 
 const size_t kMaxSuggestionsCount = 10;
 
 std::string GetPageId(const DictionaryValue& page_dictionary) {

[Marc Treib, 2017/01/18 11:24:01]

Maybe make a GetGroupId function similar to this, so not all the call sites have
to do the error checking?

Also, unrelated to this CL, but weren't the physical web folks moving away from
the dictionary representation?

[vitaliii, 2017/01/18 11:48:16]

Done.

They do, but they may do this after BP.
https://codereview.chromium.org/2643453002/

   std::string raw_resolved_url;
   if (!page_dictionary.GetString(physical_web::kResolvedUrlKey,
                                  &raw_resolved_url)) {
     LOG(DFATAL) << physical_web::kResolvedUrlKey << " field is missing.";
   }
   return raw_resolved_url;
 }
 
+bool CompareByDistanceClosestFirst(const DictionaryValue* left,

[Marc Treib, 2017/01/18 11:24:01]

nit: "ClosestFirst" IMO isn't necessary to state, I'd assume that by default
(standard numerical ordering)

[vitaliii, 2017/01/18 11:48:15]

Done.

+                                   const DictionaryValue* right) {
+  double left_distance, right_distance;
+  bool success =
+      left->GetDouble(physical_web::kDistanceEstimateKey, &left_distance);
+  success =
+      right->GetDouble(physical_web::kDistanceEstimateKey, &right_distance) &&
+      success;
+  if (!success) {
+    LOG(DFATAL) << "Distance field is missing.";
+  }
+
+  // When there is no estimate, the value is <= 0, so we need to treat this case
+  // manually.

[Marc Treib, 2017/01/18 11:24:01]

nit: Treat how, exactly?
Or maybe actually just assign infinity to the corresponding double variable?

[vitaliii, 2017/01/18 11:48:16]

Do you how to get infinity for double?

I rewrote the comment, it may do now.

[Marc Treib, 2017/01/18 11:54:47]

std::numeric_limits<double>::infinity();
(or there's also some c-style constant for it, but I forget)

[vitaliii, 2017/01/18 14:02:27]

Acknowledged.

+  bool is_left_estimated = left_distance > 0;
+  bool is_right_estimated = right_distance > 0;
+
+  if (is_left_estimated != is_right_estimated)
+    return is_left_estimated;
+  return left_distance < right_distance;
+}
+
+void FilterOutByGroupId(
+    std::vector<const DictionaryValue*>* page_dictionaries) {
+  // |std::unique| only removes duplicates that immediately follow each other.
+  // Thus, first, we have to sort by group_id and distance and only then remove
+  // duplicates.
+  std::sort(page_dictionaries->begin(), page_dictionaries->end(),
+            [](const DictionaryValue* left, const DictionaryValue* right) {
+              std::string left_group_id, right_group_id;
+              bool success =
+                  left->GetString(physical_web::kGroupIdKey, &left_group_id);
+              success = right->GetString(physical_web::kGroupIdKey,
+                                         &right_group_id) &&
+                        success;
+              if (!success) {
+                LOG(DFATAL) << "Group id field is missing.";

[Marc Treib, 2017/01/18 11:24:01]

So the field must always be there, even if it's empty?

[vitaliii, 2017/01/18 11:48:16]

As experiments show, yes.

This won't be a problem after https://codereview.chromium.org/2643453002/. 

+              }
+
+              if (left_group_id != right_group_id) {
+                return left_group_id < right_group_id;
+              }
+
+              // We want closest pages first, so in case of same group_id we
+              // sort by distance.
+              return CompareByDistanceClosestFirst(left, right);
+            });
+
+  // Empty group_id must be treated as unique, so we do not apply std::unique to

[Marc Treib, 2017/01/18 11:24:01]

nit: *Each* empty group id must be...

[vitaliii, 2017/01/18 11:48:16]

Done.

+  // them at all.
+  std::vector<const DictionaryValue*>::iterator nonempty_group_id_begin =

[Marc Treib, 2017/01/18 11:24:01]

optional: You could use auto here

[vitaliii, 2017/01/18 11:48:16]

Done.

+      page_dictionaries->begin();
+  for (; nonempty_group_id_begin != page_dictionaries->end();

[Marc Treib, 2017/01/18 11:24:01]

Since you're already using std algorithms quite heavily: This could probably be
written as an std::find_if.

[vitaliii, 2017/01/18 11:48:16]

Done.

Good observation.

+       ++nonempty_group_id_begin) {
+    std::string group_id;
+
+    (*nonempty_group_id_begin)->GetString(physical_web::kGroupIdKey, &group_id);
+    if (!group_id.empty()) {
+      break;
+    }
+  }
+
+  std::vector<const DictionaryValue*>::iterator new_end = std::unique(

[Marc Treib, 2017/01/18 11:24:01]

optional: auto

[vitaliii, 2017/01/18 11:48:15]

Done.

+      nonempty_group_id_begin, page_dictionaries->end(),
+      [](const DictionaryValue* left, const DictionaryValue* right) {
+        std::string left_group_id, right_group_id;
+
+        left->GetString(physical_web::kGroupIdKey, &left_group_id);
+        right->GetString(physical_web::kGroupIdKey, &right_group_id);
+        return left_group_id == right_group_id;
+      });
+
+  page_dictionaries->erase(new_end, page_dictionaries->end());
+}
+
 }  // namespace
 
 PhysicalWebPageSuggestionsProvider::PhysicalWebPageSuggestionsProvider(
     ContentSuggestionsProvider::Observer* observer,
     physical_web::PhysicalWebDataSource* physical_web_data_source,
     PrefService* pref_service)
     : ContentSuggestionsProvider(observer),
       category_status_(CategoryStatus::AVAILABLE),
       provided_category_(
           Category::FromKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES)),
@@ skipping 156 matching lines @@
 
     if (old_dismissed_ids.count(page_id)) {
       new_dismissed_ids.insert(page_id);
     }
   }
 
   if (old_dismissed_ids.size() != new_dismissed_ids.size()) {
     StoreDismissedIDsToPrefs(new_dismissed_ids);
   }
 
+  FilterOutByGroupId(&page_dictionaries);
+
   std::sort(page_dictionaries.begin(), page_dictionaries.end(),
-            [](const DictionaryValue* left, const DictionaryValue* right) {
-              double left_distance, right_distance;
-              bool success = left->GetDouble(physical_web::kDistanceEstimateKey,
-                                             &left_distance);
-              success = right->GetDouble(physical_web::kDistanceEstimateKey,
-                                         &right_distance) &&
-                        success;
-              if (!success) {
-                LOG(DFATAL) << "Distance field is missing.";
-              }
-              return left_distance < right_distance;
-            });
+            CompareByDistanceClosestFirst);
 
   std::vector<ContentSuggestion> suggestions;
   for (const DictionaryValue* page_dictionary : page_dictionaries) {
     if (static_cast<int>(suggestions.size()) == max_quantity) {
       break;
     }
     suggestions.push_back(ConvertPhysicalWebPage(*page_dictionary));
   }
 
   return suggestions;
@@ skipping 59 matching lines @@
 }
 
 void PhysicalWebPageSuggestionsProvider::StoreDismissedIDsToPrefs(
     const std::set<std::string>& dismissed_ids) {
   prefs::StoreDismissedIDsToPrefs(pref_service_,
                                   prefs::kDismissedPhysicalWebPageSuggestions,
                                   dismissed_ids);
 }
 
 }  // namespace ntp_snippets