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) {
+  ReturnNearbyURLs(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)> PhysicalWebPageLoadedCb =
+      base::BindRepeating(
+          [](PhysicalWebBaseMessageHandler* current,
+             const base::ListValue* args) {
+            current->ReturnNearbyURLs(nullptr);
+            UMA_HISTOGRAM_EXACT_LINEAR("PhysicalWeb.TotalUrls.OnInitialDisplay",
+                                       (int)current->GetPhysicalWebDataSource()
+                                           ->GetMetadataList()
+                                           ->size(),
+                                       50);
+          },
+          this);
+  RegisterMessageCallback(kPhysicalWebPageLoaded, PhysicalWebPageLoadedCb);

[mmocny, 2017/03/24 17:15:12]

I think you should create a member function called HandlePhysicalWebPageLoaded
since I think you will now want to track Load and Reload differently, plus the
lambda is getting ugly.

Also, I think you should not call GetMetadataList()->size() here, you should use
the actual number of results that are shown, same as your other CL
https://codereview.chromium.org/2775653002

[Ran, 2017/03/24 18:45:19]

Done.

+
   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);

[mmocny, 2017/03/24 17:15:12]

Should we perhaps do this in the constructor instead?  This isn't related to
registering messages.

 I know we cannot push results to webview until after OnPageLoaded, so
alternatively we can also move this registration there.  However I think we may
already have to do some more work given that on page reloads we don't unregister
this listener, and probably already need a way to track when page is
loaded/unloaded to prevent OnFound causing an issue if timing is bad.

Also, if we subscribed in the constructor we could start a foreground scan + PWS
resolve in parallel while the webui page loads.  I don't expect this is a big
deal, but it may be a good idea for code-cleanliness anyway.

[Ran, 2017/03/24 18:45:19]

Can't call it in constructor as GetPhysicalWebDataSource is a pure virtual
function..

[mmocny, 2017/03/24 18:54:14]

Okay fair enough.  We could create a "start" method, but at that point I think
thats what RegisterMessages is.

Just to review: who named this "RegisterMessages"?  Is this standard naming
scheme or should we rename it?

[mattreynolds, 2017/03/27 21:46:59]

The name comes from the web_ui message handler which calls it RegisterMethods in
both the iOS and non-iOS implementations:

content/public/browser/web_ui_message_handler.h
ios/web/public/webui/web_ui_ios_message_handler.h

Both implementations have the same documentation suggesting it's fine to put
other initialization tasks in RegisterMethods:

  // This is where subclasses specify which messages they'd like to handle and
  // perform any additional initialization. At this point web_ui() will return
  // the associated WebUI[IOS] object.

 }
 
-void PhysicalWebBaseMessageHandler::HandleRequestNearbyURLs(
+void PhysicalWebBaseMessageHandler::ReturnNearbyURLs(
     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_) {
+    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