Pass Physical Web metadata through a struct

components/physical_web/data_source/physical_web_data_source.h

Index: components/physical_web/data_source/physical_web_data_source.h
diff --git a/components/physical_web/data_source/physical_web_data_source.h b/components/physical_web/data_source/physical_web_data_source.h
index c6310396ce1a25c4ac2a767975182c3def46f550..0a2fe174596e73fb632b7afca13e60f6f7240c58 100644
--- a/components/physical_web/data_source/physical_web_data_source.h
+++ b/components/physical_web/data_source/physical_web_data_source.h
@@ -6,6 +6,8 @@
 #define COMPONENTS_PHYSICAL_WEB_DATA_SOURCE_PHYSICAL_WEB_DATA_SOURCE_H_
 
 #include <memory>
+#include <string>
+#include <vector>
 
 namespace base {
 class ListValue;
@@ -16,6 +18,7 @@ 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[];
@@ -25,6 +28,23 @@ 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:
@@ -41,7 +61,14 @@ class PhysicalWebDataSource {
   // 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.