Update Physical Web WebUI to show new results automatically

components/physical_web/webui/physical_web_base_message_handler.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/physical_web/webui/physical_web_base_message_handler.h"
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "components/physical_web/data_source/physical_web_data_source.h"
 #include "components/physical_web/webui/physical_web_ui_constants.h"
 
 namespace physical_web_ui {
 
-PhysicalWebBaseMessageHandler::PhysicalWebBaseMessageHandler() {}
+PhysicalWebBaseMessageHandler::PhysicalWebBaseMessageHandler()
+    : data_source_(nullptr) {}
 
-PhysicalWebBaseMessageHandler::~PhysicalWebBaseMessageHandler() = default;
+PhysicalWebBaseMessageHandler::~PhysicalWebBaseMessageHandler() {
+  if (data_source_)
+    data_source_->UnregisterListener(this);
+}
+
+void PhysicalWebBaseMessageHandler::OnFound(const GURL& url) {
+  HandleRequestNearbyURLs(nullptr);
+}
+
+void PhysicalWebBaseMessageHandler::OnLost(const GURL& url) {
+  // do nothing
+}
+
+void PhysicalWebBaseMessageHandler::OnDistanceChanged(
+    const GURL& url,
+    double distance_estimate) {
+  // do nothing
+}
 
 void PhysicalWebBaseMessageHandler::RegisterMessages() {
+  base::Callback<void(const base::ListValue* args)> RequestNearbyUrlsCb =
+      base::Bind(

[mmocny, 2017/03/22 16:03:50]

Looks like you switched to using base::Bind, which states is deprecated.  Did
you mean to use BindRepeating or BindOnce?

[Ran, 2017/03/22 19:10:47]

Done.

+          [](PhysicalWebBaseMessageHandler* current,
+             const base::ListValue* args) {
+            current->HandleRequestNearbyURLs(nullptr);
+            UMA_HISTOGRAM_EXACT_LINEAR("PhysicalWeb.TotalUrls.OnInitialDisplay",
+                                       (int)current->GetPhysicalWebDataSource()
+                                           ->GetMetadataList()
+                                           ->size(),
+                                       50);
+          },
+          this);

[mmocny, 2017/03/22 16:03:50]

It looks like you switched to use raw `this`, instead of
`base::Unretained(this)` -- was this intended?

[Ran, 2017/03/22 19:10:47]

It is intended. Can't bind base::Unretained(this) to a lambda function. (even
can't capture this, Bind can only convert a captureless lambda to callback)

+  RegisterMessageCallback(kRequestNearbyUrls, RequestNearbyUrlsCb);

[mmocny, 2017/03/22 16:03:50]

Could you explain what this is doing?

It looks like you wrapped the HandleRequestNearbyURLs function with a lambda
which calls the UMA.

It still looks like OnInitialDisplay will be called every time this message is
handled, unless I misunderstand, so I don't see how this is resolved?

[Ran, 2017/03/22 19:10:47]

I separate the UMA from the HandleRequestNearbyURLs function. The UMA will be
called once the page is loaded, other time it will call HandleRequestNearbyURLs,
which does not include the UMA.

Renamed it to PhysicalWebPageLoaded to avoid confusing.

+
   RegisterMessageCallback(
-      kRequestNearbyUrls,
-      base::BindRepeating(
-          &PhysicalWebBaseMessageHandler::HandleRequestNearbyURLs,
-          base::Unretained(this)));
-  RegisterMessageCallback(kPhysicalWebItemClicked,
+      kPhysicalWebItemClicked,
       base::BindRepeating(
           &PhysicalWebBaseMessageHandler::HandlePhysicalWebItemClicked,
           base::Unretained(this)));
+
+  data_source_ = GetPhysicalWebDataSource();
+  if (data_source_)
+    data_source_->RegisterListener(this);
 }
 
 void PhysicalWebBaseMessageHandler::HandleRequestNearbyURLs(
     const base::ListValue* args) {
   base::DictionaryValue results;
 
   std::unique_ptr<physical_web::MetadataList> metadata_list =
       GetPhysicalWebDataSource()->GetMetadataList();
 
+  // Append new metadata at the end of the list.
+  for (const auto& metadata_list_item : *metadata_list) {
+    const std::string& group_id = metadata_list_item.group_id;
+    if (metadata_map_.find(group_id) == metadata_map_.end()) {
+      ordered_group_ids_.push_back(group_id);
+      metadata_map_.emplace(group_id, metadata_list_item);
+    }
+  }
+
   auto metadata = base::MakeUnique<base::ListValue>();
   int index = 0;
-  for (const auto& metadata_list_item : *metadata_list) {
+  for (const auto& group_id : ordered_group_ids_) {

[mmocny, 2017/03/22 16:03:50]

Ran, as you can see, this is attempting to produce values one-per-groupid.

I wonder, is the group-id just not being set correctly?  Perhaps we should just
fix this code if we are seeing duplicates.

[Ran, 2017/03/22 19:10:47]

Done.

+    auto metadata_list_item = metadata_map_[group_id];
     auto metadata_item = base::MakeUnique<base::DictionaryValue>();
     metadata_item->SetString(physical_web_ui::kResolvedUrl,
                              metadata_list_item.resolved_url.spec());
     metadata_item->SetString(physical_web_ui::kPageInfoIcon,
                              metadata_list_item.icon_url.spec());
     metadata_item->SetString(physical_web_ui::kPageInfoTitle,
                              metadata_list_item.title);
     metadata_item->SetString(physical_web_ui::kPageInfoDescription,
                              metadata_list_item.description);
     // Add the item index so when an item is selected, the index can be recorded
     // in a UMA histogram.
     metadata_item->SetInteger(physical_web_ui::kIndex, index);
     metadata->Append(std::move(metadata_item));
     ++index;
   }
 
   results.Set(physical_web_ui::kMetadata, metadata.release());
 
-  UMA_HISTOGRAM_EXACT_LINEAR("PhysicalWeb.TotalUrls.OnInitialDisplay",
-                             (int)metadata_list->size(), 50);
-
   // Pass the list of Physical Web URL metadata to the WebUI. A jstemplate will
   // create a list view with an item for each URL.
   CallJavaScriptFunction(physical_web_ui::kReturnNearbyUrls, results);
 }
 
 void PhysicalWebBaseMessageHandler::HandlePhysicalWebItemClicked(
     const base::ListValue* args) {
   int index = 0;
   if (!args->GetInteger(0, &index)) {
     DLOG(ERROR) << "Invalid selection index";
     return;
   }
 
   // Record the index of the selected item.
   UMA_HISTOGRAM_EXACT_LINEAR("PhysicalWeb.WebUI.ListViewUrlPosition", index,
                              50);
 
   // Count the number of selections.
   base::RecordAction(
       base::UserMetricsAction("PhysicalWeb.WebUI.ListViewUrlSelected"));
 }
 
-} //  namespace physical_web_ui
+}  //  namespace physical_web_ui