Pass Physical Web metadata through a struct

Pass Physical Web metadata through a struct

Currently, we use DictionaryValues in the Physical Web data source.
The problem with this is that we cannot store store longs (timestamps).
This change introduces a metadata struct that can be requested from the
data source, deprecating the DictionaryValue option.

BUG=667722
Review-Url: https://codereview.chromium.org/2561493002
Cr-Commit-Position: refs/heads/master@{#443425}
Committed: https://chromium.googlesource.com/chromium/src/+/4417ce64c063776af45d1d897f15cdc0be361a2e

[cco3, 2016-12-07 00:55:52]

cco3@chromium.org changed reviewers:
+ mattreynolds@chromium.org

[Marc Treib, 2016-12-07 08:58:12]

treib@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-12-07 08:58:13]

+vitaliii FYI

[vitaliii, 2016-12-07 09:44:13]

On 2016/12/07 08:58:13, Marc Treib wrote:
> +vitaliii FYI

Thanks. This CL also covers crbug.com/668141.

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

vitaliii@chromium.org changed reviewers:
+ vitaliii@chromium.org

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

Drive-by comments.

Thanks! Happy to see this change (both timestamps and |struct| for physical web
page).

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:44:
dictionary_value->SetInteger(physical_web::kScanTimestampKey, scan_timestamp);
Probably add a comment that this does not work and do not set this field at all?

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/physical_web_data_source_android.h
(right):

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/physical_web_data_source_android.h:43: //
TODO(cco3): Remove when we no longer rely on this.
nit: explicitly say that this is deprecated and new users must use
|GetMatadataList| instead.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:32: struct
Metadata {
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.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:38: std::string
icon_url;
It would be nice to have these as GURL, but it seems that this will contradict
with Observer's methods.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:43: long
scan_timestamp;
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?

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:46: using
MetadataList = std::vector<Metadata>;
Nit: This name somewhat collides with |ListValue| usage.
E.g. |GetMetadataListValue| takes quite some time to parse after one noticed
|MetadataList|.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:71: virtual
std::unique_ptr<base::ListValue> GetMetadataListValue() = 0;
Explicitly say that this is deprecated and new users must use |GetMetadata|
instead.

[hollyfaith92, 2016-12-07 10:17:43]

"Great article".Struct is a keyword of structure,We can develop code using
structure.You should want to study the code and algorithm of the computer
science
correctly.https://maketheunitedstatesgreatagain.com/products/trump-usa-45-hat

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

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:39: auto
dictionary_value = new base::DictionaryValue();
This should be:

auto dictionary_value = base::MakeUnique<base::DictionaryValue>();

It should do the same thing, but the ListValue::Append method that takes a raw
pointer is deprecated.

https://codereview.chromium.org/2561493002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2561493002/diff/1/components/omnibox/browser/...
components/omnibox/browser/physical_web_provider.cc:76:
ConstructMatches(data_source->GetMetadataListValue().get());
There's another GetMetadata() caller that will need to be switched to
GetMetadataListValue():

PhysicalWebDOMHandler::HandleRequestNearbyURLs
//src/ios/chrome/browser/ui/webui/physical_web_ui.cc

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source.h (left):

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:29: class
PhysicalWebDataSource {
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]

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:39: auto
dictionary_value = new base::DictionaryValue();
On 2016/12/07 19:55:31, mattreynolds wrote:
> This should be:
> 
> auto dictionary_value = base::MakeUnique<base::DictionaryValue>();
> 
> It should do the same thing, but the ListValue::Append method that takes a raw
> pointer is deprecated.

Done.

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:44:
dictionary_value->SetInteger(physical_web::kScanTimestampKey, scan_timestamp);
On 2016/12/07 10:14:04, vitaliii wrote:
> Probably add a comment that this does not work and do not set this field at
all?

Done.

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/physical_web_data_source_android.h
(right):

https://codereview.chromium.org/2561493002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/physical_web_data_source_android.h:43: //
TODO(cco3): Remove when we no longer rely on this.
On 2016/12/07 10:14:04, vitaliii wrote:
> nit: explicitly say that this is deprecated and new users must use
> |GetMatadataList| instead.

Done.

https://codereview.chromium.org/2561493002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/physical_web_provider.cc (right):

https://codereview.chromium.org/2561493002/diff/1/components/omnibox/browser/...
components/omnibox/browser/physical_web_provider.cc:76:
ConstructMatches(data_source->GetMetadataListValue().get());
On 2016/12/07 19:55:31, mattreynolds wrote:
> There's another GetMetadata() caller that will need to be switched to
> GetMetadataListValue():
> 
> PhysicalWebDOMHandler::HandleRequestNearbyURLs
> //src/ios/chrome/browser/ui/webui/physical_web_ui.cc

Done.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source.h (left):

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:29: class
PhysicalWebDataSource {
On 2016/12/07 19:55:31, mattreynolds wrote:
> 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

Done.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:32: struct
Metadata {
On 2016/12/07 10:14:05, vitaliii wrote:
> 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.

Done.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:38: std::string
icon_url;
On 2016/12/07 10:14:04, vitaliii wrote:
> It would be nice to have these as GURL, but it seems that this will contradict
> with Observer's methods.

Done.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:43: long
scan_timestamp;
On 2016/12/07 10:14:04, vitaliii wrote:
> 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?

Done.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:46: using
MetadataList = std::vector<Metadata>;
On 2016/12/07 10:14:04, vitaliii wrote:
> Nit: This name somewhat collides with |ListValue| usage.
> E.g. |GetMetadataListValue| takes quite some time to parse after one noticed
> |MetadataList|.

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

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:71: virtual
std::unique_ptr<base::ListValue> GetMetadataListValue() = 0;
On 2016/12/07 10:14:04, vitaliii wrote:
> Explicitly say that this is deprecated and new users must use |GetMetadata|
> instead.

Done.

[mattreynolds, 2016-12-09 00:10:03]

https://codereview.chromium.org/2561493002/diff/20001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/20001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:34: struct
Metadata {
We should comment on which fields are optional. IIRC, at the PWS level the
scanned_url and resolved_url are required, and at least one of title or
description is required. On iOS we only set scanned_url, resolved_url, icon_url,
title, and description.

distance_estimate should probably be optional, maybe we should document it as
returning -1.0 when no distance estimate is available? (what do we do now?)

icon_url is definitely optional, group_id and scan_timestamp should be required
(but currently aren't).

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

More comments.

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/1/components/physical_web/dat...
components/physical_web/data_source/physical_web_data_source.h:46: using
MetadataList = std::vector<Metadata>;
On 2016/12/08 23:52:50, cco3 wrote:
> On 2016/12/07 10:14:04, vitaliii wrote:
> > Nit: This name somewhat collides with |ListValue| usage.
> > E.g. |GetMetadataListValue| takes quite some time to parse after one noticed
> > |MetadataList|.
> 
> I think it's fine to leave it.  The confusion will only be temporary.

Acknowledged.

https://codereview.chromium.org/2561493002/diff/40001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:66:
metadata.scan_timestamp = scan_timestamp;
I do not feel that this works as expected.
But even if it does it would be nice to make the conversion more explicit.
There is base::Time::FromJavaTime, which may be useful.

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:44: GURL
resolved_url;
Then PhysicalWebListener methods (e.g. OnFound) should take GURL as well.

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:52: // OPTIONAL
Could you elaborate on when one should not expect to get the description?

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:54: // An
identifier that associates multiple resolved URLs.  If two URLs have
I do not understand this. Could you give an example of two resolved URLs with
the same group id and why only one of them should be shown?

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:57: // OPTIONAL:
treat the item as its own unique group if this doesn't exist.
s/doesn't exist/empty

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:62: //
calculated.
An example when the distance is not estimated?

[cco3, 2016-12-10 00:25:23]

https://codereview.chromium.org/2561493002/diff/40001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:66:
metadata.scan_timestamp = scan_timestamp;
On 2016/12/09 09:52:49, vitaliii wrote:
> I do not feel that this works as expected.
> But even if it does it would be nice to make the conversion more explicit.
> There is base::Time::FromJavaTime, which may be useful. 

Yea, I didn't think it would either.  Thanks for finding this.

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:44: GURL
resolved_url;
On 2016/12/09 09:52:49, vitaliii wrote:
> Then PhysicalWebListener methods (e.g. OnFound) should take GURL as well.

Done.

Hm, good point.

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:52: // OPTIONAL
On 2016/12/09 09:52:49, vitaliii wrote:
> Could you elaborate on when one should not expect to get the description?

Done.

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:54: // An
identifier that associates multiple resolved URLs.  If two URLs have
On 2016/12/09 09:52:49, vitaliii wrote:
> I do not understand this. Could you give an example of two resolved URLs with
> the same group id and why only one of them should be shown?

Done.

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:57: // OPTIONAL:
treat the item as its own unique group if this doesn't exist.
On 2016/12/09 09:52:49, vitaliii wrote:
> s/doesn't exist/empty

Done.

https://codereview.chromium.org/2561493002/diff/40001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:62: //
calculated.
On 2016/12/09 09:52:49, vitaliii wrote:
> An example when the distance is not estimated?

Done.

[vitaliii, 2016-12-12 07:04:13]

LGTM with a nit. Thanks!

You still will have to wait for OWNER approval (treib@) for ntp_snippets folder.

https://codereview.chromium.org/2561493002/diff/40001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:66:
metadata.scan_timestamp = scan_timestamp;
On 2016/12/10 00:25:23, cco3 wrote:
> On 2016/12/09 09:52:49, vitaliii wrote:
> > I do not feel that this works as expected.
> > But even if it does it would be nice to make the conversion more explicit.
> > There is base::Time::FromJavaTime, which may be useful. 
> 
> Yea, I didn't think it would either.  Thanks for finding this.

Acknowledged.

https://codereview.chromium.org/2561493002/diff/60001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/60001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:67:
metadata_list_->push_back(std::move(metadata_item));
s/time/Time?

[cco3, 2016-12-12 19:33:36]

https://codereview.chromium.org/2561493002/diff/60001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/60001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:67:
metadata_list_->push_back(std::move(metadata_item));
On 2016/12/12 07:04:13, vitaliii wrote:
> s/time/Time?

Whoops, I was hiding some of the compile output and didn't see that this broke.

[cco3, 2016-12-12 19:34:01]

cco3@chromium.org changed reviewers:
+ nyquist@chromium.org

[cco3, 2016-12-12 19:34:02]

Hi Tommy, could you review the android changes here?  Thanks!

[mattreynolds, 2016-12-12 19:41:29]

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:12: #include
"url/gurl.h"
Also add "//url" to the data_source deps in
components/physical_web/data_source/BUILD.gn

https://codereview.chromium.org/2561493002/diff/80001/ios/chrome/browser/phys...
File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2561493002/diff/80001/ios/chrome/browser/phys...
ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h:34:
std::unique_ptr<base::ListValue> GetMetadataListValue() override;
(should be GetMetadata)

Also, IOSChromePhysicalWebDataSource needs an implementation of GetMetadataList
(I think it's fine for it to be empty for now)

https://codereview.chromium.org/2561493002/diff/80001/ios/chrome/browser/ui/w...
File ios/chrome/browser/ui/webui/physical_web_ui.cc (right):

https://codereview.chromium.org/2561493002/diff/80001/ios/chrome/browser/ui/w...
ios/chrome/browser/ui/webui/physical_web_ui.cc:83: ->GetMetadataListValue();
GetMetadata

[nyquist, 2016-12-13 18:13:25]

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:58:
physical_web::Metadata metadata;
Do we want to ensure that we didn't mess up any variable names here by writing a
test?

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:126:
(long)pw_collection.get());
Nit: missing space after cast?

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.h
(right):

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.h:69:
std::unique_ptr<physical_web::MetadataList> GetMetadataList() override;
The naming of these methods do not seem consistent. GetMetaDataList seems like
the new thing, but GetMetadata and GetMetadataListValue are both referring to
base::ListValue. Could we use the same method name for the same thing across
this?

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:11: #include
"base/time/time.h"
Nit: Should there be a newline before the //base include?

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:12: #include
"url/gurl.h"
On 2016/12/12 19:41:29, mattreynolds wrote:
> Also add "//url" to the data_source deps in
> components/physical_web/data_source/BUILD.gn

Since this is an exposed header with a new include, I guess this should be in
public_deps?

[cco3, 2016-12-16 19:17:53]

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:58:
physical_web::Metadata metadata;
On 2016/12/13 18:13:25, nyquist wrote:
> Do we want to ensure that we didn't mess up any variable names here by writing
a
> test?

Done.

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.h
(right):

https://codereview.chromium.org/2561493002/diff/80001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.h:69:
std::unique_ptr<physical_web::MetadataList> GetMetadataList() override;
On 2016/12/13 18:13:25, nyquist wrote:
> The naming of these methods do not seem consistent. GetMetaDataList seems like
> the new thing, but GetMetadata and GetMetadataListValue are both referring to
> base::ListValue. Could we use the same method name for the same thing across
> this?

Good catch.  The whole ListValue thing is temporary (hopefully will be gone in a
few weeks) and I went through too stages of naming it.

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:11: #include
"base/time/time.h"
On 2016/12/13 18:13:25, nyquist wrote:
> Nit: Should there be a newline before the //base include?

Done.

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:12: #include
"url/gurl.h"
On 2016/12/13 18:13:25, nyquist wrote:
> On 2016/12/12 19:41:29, mattreynolds wrote:
> > Also add "//url" to the data_source deps in
> > components/physical_web/data_source/BUILD.gn
> 
> Since this is an exposed header with a new include, I guess this should be in
> public_deps?

Done.

https://codereview.chromium.org/2561493002/diff/80001/components/physical_web...
components/physical_web/data_source/physical_web_data_source.h:12: #include
"url/gurl.h"
On 2016/12/12 19:41:29, mattreynolds wrote:
> Also add "//url" to the data_source deps in
> components/physical_web/data_source/BUILD.gn

Done.

[mattreynolds, 2016-12-16 20:27:44]

https://codereview.chromium.org/2561493002/diff/120001/chrome/browser/android...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2561493002/diff/120001/chrome/browser/android...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:114:
(long) pw_collection.get());
Use reinterpret_cast<long>

https://codereview.chromium.org/2561493002/diff/120001/chrome/browser/android...
File
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc
(right):

https://codereview.chromium.org/2561493002/diff/120001/chrome/browser/android...
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc:73:
(jlong) kScanTimestamp,
Use static_casts

https://codereview.chromium.org/2561493002/diff/120001/chrome/browser/android...
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc:82:
EXPECT_EQ((size_t) 1, metadata_list->size());
EXPECT_EQ(1U, metadata_list->size());

[mattreynolds, 2016-12-16 21:17:07]

lgtm

https://codereview.chromium.org/2561493002/diff/140001/chrome/browser/android...
File
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc
(right):

https://codereview.chromium.org/2561493002/diff/140001/chrome/browser/android...
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc:100:
(jlong) kScanTimestamp,
some more here

[cco3, 2016-12-21 20:02:07]

cco3@chromium.org changed reviewers:
+ jyquinn@chromium.org

[cco3, 2016-12-21 20:02:07]

Hi jyquinn@, would you be able to take a look at these iOS changes?

[Jackie Quinn, 2016-12-22 01:39:01]

jyquinn@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Jackie Quinn, 2016-12-22 01:39:02]

ios lgtm. I'm not an owner though, so cc'ing eugenebut@

[nyquist, 2017-01-05 07:42:28]

https://codereview.chromium.org/2561493002/diff/160001/chrome/browser/android...
File
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc
(right):

https://codereview.chromium.org/2561493002/diff/160001/chrome/browser/android...
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc:36:
PhysicalWebCollection* collection();
Naming for C++ starts with uppercase. See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
and the links to the Google C++ style guide.

https://codereview.chromium.org/2561493002/diff/160001/chrome/browser/android...
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc:45:
void PhysicalWebCollectionTest::SetUp() {
This is fine. I've also seen many times where we inline these test-functions.
I'm happy if you keep it as is as well though. I like that quite a bit to get a
good overview of the class first.

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
File components/physical_web/data_source/BUILD.gn (right):

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
components/physical_web/data_source/BUILD.gn:16: "//url",
This deps will also be required by packages that depend on this, since the
dependency is in the public header. Move this to a new public_deps entry
instead. In fact, a //base dependency is also now in the header, so just change
'deps =' on line 14 to 'public_deps ='

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
File components/physical_web/data_source/fake_physical_web_data_source.cc
(right):

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
components/physical_web/data_source/fake_physical_web_data_source.cc:77: return
base::MakeUnique<MetadataList>(*metadata_list_.get());
Does this copy all the underlying elements? Should this just std::move instead?

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:53: // OPTIONAL:
when the website has not specified a description, the PWS
Nit: Uppercase after : throughout these comments.

[cco3, 2017-01-05 23:52:56]

https://codereview.chromium.org/2561493002/diff/160001/chrome/browser/android...
File
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc
(right):

https://codereview.chromium.org/2561493002/diff/160001/chrome/browser/android...
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc:36:
PhysicalWebCollection* collection();
On 2017/01/05 07:42:28, nyquist (OOO back 1-3) wrote:
> Naming for C++ starts with uppercase. See
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> and the links to the Google C++ style guide.

Done.

https://codereview.chromium.org/2561493002/diff/160001/chrome/browser/android...
chrome/browser/android/physical_web/physical_web_data_source_android_unittest.cc:45:
void PhysicalWebCollectionTest::SetUp() {
On 2017/01/05 07:42:28, nyquist (OOO back 1-3) wrote:
> This is fine. I've also seen many times where we inline these test-functions.
> I'm happy if you keep it as is as well though. I like that quite a bit to get
a
> good overview of the class first.

Done.

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
File components/physical_web/data_source/BUILD.gn (right):

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
components/physical_web/data_source/BUILD.gn:16: "//url",
On 2017/01/05 07:42:28, nyquist (OOO back 1-3) wrote:
> This deps will also be required by packages that depend on this, since the
> dependency is in the public header. Move this to a new public_deps entry
> instead. In fact, a //base dependency is also now in the header, so just
change
> 'deps =' on line 14 to 'public_deps ='

Done.

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
File components/physical_web/data_source/fake_physical_web_data_source.cc
(right):

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
components/physical_web/data_source/fake_physical_web_data_source.cc:77: return
base::MakeUnique<MetadataList>(*metadata_list_.get());
On 2017/01/05 07:42:28, nyquist (OOO back 1-3) wrote:
> Does this copy all the underlying elements? Should this just std::move
instead?

This test datasource gets reused, so we do want to copy it.

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
File components/physical_web/data_source/physical_web_data_source.h (right):

https://codereview.chromium.org/2561493002/diff/160001/components/physical_we...
components/physical_web/data_source/physical_web_data_source.h:53: // OPTIONAL:
when the website has not specified a description, the PWS
On 2017/01/05 07:42:28, nyquist (OOO back 1-3) wrote:
> Nit: Uppercase after : throughout these comments.

Done.

[cco3, 2017-01-09 18:51:22]

Hi Tommy, does this look fine now?

[nyquist, 2017-01-11 18:52:53]

lgtm

[cco3, 2017-01-11 19:25:10]

The CQ bit was checked by cco3@chromium.org

[cco3, 2017-01-11 19:25:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
vitaliii@chromium.org, mattreynolds@chromium.org, jyquinn@chromium.org,
nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2561493002/#ps200001
(title: "Remove ios change")

[commit-bot: I haz the power, 2017-01-11 19:26:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 19:34:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:34:52]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[cco3, 2017-01-11 19:56:04]

Matt, could you take a look at these iOS changes?

[mattreynolds, 2017-01-11 20:54:51]

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h:40:
std::unique_ptr<MetadataList> GetMetadataList() override;
MetadataList needs the physical_web:: namespace

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.mm
(right):

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.mm:55:
std::unique_ptr<MetadataList>
physical_web:: (here and in the MakeUnique call below)

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.h (right):

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.h:63: -
(std::unique_ptr<base::ListValue>)metadataList;
physical_web::MetadataList

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:212: -
(std::unique_ptr<MetadataList>)metadataList {
physical_web:: (and in MakeUnique below)

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:230:
metadataRet->Append(std::move(metadataItem));
push_back

[cco3, 2017-01-11 22:51:02]

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h:40:
std::unique_ptr<MetadataList> GetMetadataList() override;
On 2017/01/11 20:54:50, mattreynolds wrote:
> MetadataList needs the physical_web:: namespace

Done.

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.mm
(right):

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.mm:55:
std::unique_ptr<MetadataList>
On 2017/01/11 20:54:50, mattreynolds wrote:
> physical_web:: (here and in the MakeUnique call below)

Done.

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.h (right):

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.h:63: -
(std::unique_ptr<base::ListValue>)metadataList;
On 2017/01/11 20:54:50, mattreynolds wrote:
> physical_web::MetadataList

Done.

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.mm (right):

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:212: -
(std::unique_ptr<MetadataList>)metadataList {
On 2017/01/11 20:54:50, mattreynolds wrote:
> physical_web:: (and in MakeUnique below)

Done.

https://codereview.chromium.org/2561493002/diff/230001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.mm:230:
metadataRet->Append(std::move(metadataItem));
On 2017/01/11 20:54:51, mattreynolds wrote:
> push_back

Done.

[mattreynolds, 2017-01-11 23:04:52]

https://codereview.chromium.org/2561493002/diff/250001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2561493002/diff/250001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h:40:
std::unique_ptr<physical_web::MetadataList> GetMetadataList() override;
This file needs #include
"components/physical_web/data_source/physical_web_data_source.h" (and probably
remove it from the .mm)

[cco3, 2017-01-11 23:32:41]

https://codereview.chromium.org/2561493002/diff/250001/ios/chrome/browser/phy...
File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h
(right):

https://codereview.chromium.org/2561493002/diff/250001/ios/chrome/browser/phy...
ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h:40:
std::unique_ptr<physical_web::MetadataList> GetMetadataList() override;
On 2017/01/11 23:04:52, mattreynolds wrote:
> This file needs #include
> "components/physical_web/data_source/physical_web_data_source.h" (and probably
> remove it from the .mm)

Done.

[mattreynolds, 2017-01-12 00:29:00]

(oops, I meant to post the previous comment to physical_web_scanner.h)

Once that's fixed, LGTM

[cco3, 2017-01-12 00:29:57]

cco3@chromium.org changed reviewers:
+ olivierrobin@chromium.org

[cco3, 2017-01-12 00:29:58]

Hi Olivier, would you be able to review these iOS changes?

[Olivier, 2017-01-12 08:47:46]

ios LGTM one nit

https://codereview.chromium.org/2561493002/diff/290001/ios/chrome/common/phys...
File ios/chrome/common/physical_web/physical_web_scanner.h (right):

https://codereview.chromium.org/2561493002/diff/290001/ios/chrome/common/phys...
ios/chrome/common/physical_web/physical_web_scanner.h:12: #include
"components/physical_web/data_source/physical_web_data_source.h"
Can you forward declare needed classes?

[cco3, 2017-01-12 23:22:06]

cco3@chromium.org changed reviewers:
+ rohitrao@chromium.org

[cco3, 2017-01-12 23:22:07]

Hi rohitrao@, would you be able to review these two unreviewed iOS files?

[rohitrao (ping after 24h), 2017-01-12 23:49:16]

ios/ LGTM

Is this something we could add unittests for, like we have on Android?

[cco3, 2017-01-12 23:50:38]

Thanks!

Possibly, but that will have to come in another change.

[cco3, 2017-01-12 23:50:45]

The CQ bit was checked by cco3@chromium.org

[cco3, 2017-01-12 23:50:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyquinn@chromium.org, vitaliii@chromium.org, nyquist@chromium.org,
olivierrobin@chromium.org, mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/2561493002/#ps310001
(title: "Forward declare MetadataList")

[commit-bot: I haz the power, 2017-01-12 23:51:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 01:09:40]

CQ is committing da patch.

Bot data: {"patchset_id": 310001, "attempt_start_ts": 1484265045418680,
"parent_rev": "c4bfb0e6139a447f0b4eeb31d34064b7ebf0fdcc", "commit_rev":
"4417ce64c063776af45d1d897f15cdc0be361a2e"}

[commit-bot: I haz the power, 2017-01-13 01:10:28]

Description was changed from

==========
Pass Physical Web metadata through a struct

Currently, we use DictionaryValues in the Physical Web data source.
The problem with this is that we cannot store store longs (timestamps).
This change introduces a metadata struct that can be requested from the
data source, deprecating the DictionaryValue option.

BUG=667722
==========

to

==========
Pass Physical Web metadata through a struct

Currently, we use DictionaryValues in the Physical Web data source.
The problem with this is that we cannot store store longs (timestamps).
This change introduces a metadata struct that can be requested from the
data source, deprecating the DictionaryValue option.

BUG=667722

Review-Url: https://codereview.chromium.org/2561493002
Cr-Commit-Position: refs/heads/master@{#443425}
Committed:
https://chromium.googlesource.com/chromium/src/+/4417ce64c063776af45d1d897f15...
==========

[commit-bot: I haz the power, 2017-01-13 01:10:30]

Committed patchset #17 (id:310001) as
https://chromium.googlesource.com/chromium/src/+/4417ce64c063776af45d1d897f15...