Pass Physical Web metadata through a struct

components/physical_web/data_source/physical_web_data_source.h

 // 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.
 
 #ifndef COMPONENTS_PHYSICAL_WEB_DATA_SOURCE_PHYSICAL_WEB_DATA_SOURCE_H_
 #define COMPONENTS_PHYSICAL_WEB_DATA_SOURCE_PHYSICAL_WEB_DATA_SOURCE_H_
 
 #include <memory>
+#include <string>
+#include <vector>
 
 namespace base {
 class ListValue;
 }
 
 namespace physical_web {
 
 class PhysicalWebListener;
 
 // Dictionary keys for reading Physical Web URL metadata.
+// TODO(cco3): Remove these when we are no longer dependent.
 extern const char kDescriptionKey[];
 extern const char kDistanceEstimateKey[];
 extern const char kGroupIdKey[];
 extern const char kIconUrlKey[];
 extern const char kResolvedUrlKey[];
 extern const char kScanTimestampKey[];
 extern const char kScannedUrlKey[];
 extern const char kTitleKey[];
 
+// Metadata struct for associating data with Physical Web URLs.
+struct Metadata {

[vitaliii, 2016/12/07 10:14:05]

Why not commenting each field?
E.g. guarantees (is everything always set?), units (for distance_estimate),
format (e.g. for urls), |group_id| I do not understand at all. 

In worst case, please at least point where to look in the Java code for all of
these.

[cco3, 2016/12/08 23:52:51]

Done.

+  Metadata();
+  Metadata(const Metadata& other);
+  ~Metadata();
+  std::string scanned_url;
+  std::string resolved_url;
+  std::string icon_url;

[vitaliii, 2016/12/07 10:14:04]

It would be nice to have these as GURL, but it seems that this will contradict
with Observer's methods.

[cco3, 2016/12/08 23:52:50]

Done.

+  std::string title;
+  std::string description;
+  std::string group_id;
+  double distance_estimate;
+  long scan_timestamp;

[vitaliii, 2016/12/07 10:14:04]

As far as I know, |long| in Java is 64 bits, but in C++ it is at least 32 bits.
What about int64_t or even uint64_t? However, going even further, no one is
going to use raw java timestamp, I believe this will be converted to base::Time,
why not doing this here already and have base::Time instead?

[cco3, 2016/12/08 23:52:50]

Done.

+};
+
+using MetadataList = std::vector<Metadata>;

[vitaliii, 2016/12/07 10:14:04]

Nit: This name somewhat collides with |ListValue| usage.
E.g. |GetMetadataListValue| takes quite some time to parse after one noticed
|MetadataList|.

[cco3, 2016/12/08 23:52:50]

I think it's fine to leave it.  The confusion will only be temporary.

[vitaliii, 2016/12/09 09:52:49]

Acknowledged.

+
 // Helper class for accessing Physical Web metadata and controlling the scanner.
 class PhysicalWebDataSource {

[mattreynolds, 2016/12/07 19:55:31]

Two more PhysicalWebDataSource implementations that need updating:

//src/components/physical_web/data_source/fake_physical_web_data_source.h
//src/ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h

[cco3, 2016/12/08 23:52:51]

Done.

  public:
   virtual ~PhysicalWebDataSource() {}
 
   // Starts scanning for Physical Web URLs. If |network_request_enabled| is
   // true, discovered URLs will be sent to a resolution service.
   virtual void StartDiscovery(bool network_request_enabled) = 0;
 
   // Stops scanning for Physical Web URLs and clears cached URL content.
   virtual void StopDiscovery() = 0;
 
   // Returns a list of resolved URLs and associated page metadata. If network
   // requests are disabled or if discovery is not active, the list will be
   // empty. The method can be called at any time to receive the current metadata
   // list.
-  virtual std::unique_ptr<base::ListValue> GetMetadata() = 0;
+  virtual std::unique_ptr<MetadataList> GetMetadata() = 0;
+
+  // Returns a list of resolved URLs and associated page metadata. If network
+  // requests are disabled or if discovery is not active, the list will be
+  // empty. The method can be called at any time to receive the current metadata
+  // list.
+  // TODO(cco3): Remove this when we are no longer dependent on it.
+  virtual std::unique_ptr<base::ListValue> GetMetadataListValue() = 0;

[vitaliii, 2016/12/07 10:14:04]

Explicitly say that this is deprecated and new users must use |GetMetadata|
instead.

[cco3, 2016/12/08 23:52:51]

Done.

 
   // Returns boolean |true| if network requests are disabled and there are one
   // or more discovered URLs that have not been sent to the resolution service.
   // The method can be called at any time to check for unresolved discoveries.
   // If discovery is inactive or network requests are enabled, it will always
   // return false.
   virtual bool HasUnresolvedDiscoveries() = 0;
 
   // Register for changes to Physical Web URLs and associated page metadata.
   virtual void RegisterListener(PhysicalWebListener* physical_web_listener) = 0;
 
   // Unregister for changes to Physical Web URLs and associated page metadata.
   virtual void UnregisterListener(
       PhysicalWebListener* physical_web_listener) = 0;
 };
 
 }  // namespace physical_web
 
 #endif  // COMPONENTS_PHYSICAL_WEB_DATA_SOURCE_PHYSICAL_WEB_DATA_SOURCE_H_