Replace deprecated calls to GetMetadata in Physical Web Zine provider

Replace deprecated calls to GetMetadata in Physical Web Zine provider

Use GetMetadataList instead of GetMetadata, which is now deprecated.

BUG=672012
Review-Url: https://codereview.chromium.org/2643453002
Cr-Commit-Position: refs/heads/master@{#445113}
Committed: https://chromium.googlesource.com/chromium/src/+/e11c34fb2c29ea47003a20391f0c7222b14b1e13

[mattreynolds, 2017-01-17 20:18:00]

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

[mattreynolds, 2017-01-17 20:18:00]

Hi Vitalii, PTAL

Let's move PhysicalWebPageSuggestionsProvider onto the new struct-based
PhysicalWebDataSource API.

[vitaliii, 2017-01-18 08:03:35]

Thank you for this change!

I will be OOO until the end of this week.
Please redirect this to treib@ or jkrcal@ when you address my comments.

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:38:
return page_metadata.resolved_url.spec();
This looks so much better now, thank you for the new data interface!

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:208:
return left.distance_estimate < right.distance_estimate;
Since now it is known that distance_estimate may be <= 0, could you compare them
such that <= 0 ones are last? Also adding a test would be nice.

You could achieve this by e.g.:
bool is_left_estimated = left.distance_estimate > 0;
bool is_right_estimated = right.distance_estimate > 0;
if (is_left_estimated != is_right_estimated) return is_left_estimated;
return left.distance_estimate < right.distance_estimate;

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h
(right):

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
I have another bug for this change (crbug.com/672012). Please add it to the
description.

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:20:
class GURL;
Please sort.

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:25:
}
}  // namespace physical_web

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:150:
physical_web_data_source()->SetMetadata(
I believe tests currently does not work, because SetMetadata does not seem to
impact GetMetadataList.

I guess you would like to get rid of the old interface completely, so it would
be nice to rewrite this to use SetMetadataList and all CreateDummy.* helpers to
return Metadata.

[vitaliii, 2017-01-18 11:50:51]

a small update to one of the comments.

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:208:
return left.distance_estimate < right.distance_estimate;
On 2017/01/18 08:03:34, vitaliii (OOO until 23rd) wrote:
> Since now it is known that distance_estimate may be <= 0, could you compare
them
> such that <= 0 ones are last? Also adding a test would be nice.
> 
> You could achieve this by e.g.:
> bool is_left_estimated = left.distance_estimate > 0;
> bool is_right_estimated = right.distance_estimate > 0;
> if (is_left_estimated != is_right_estimated) return is_left_estimated;
> return left.distance_estimate < right.distance_estimate;
> 

I am actually addressing this in https://codereview.chromium.org/2640773002/,
since you do not have to land yours before BP.
If it lands first, feel free to ignore the comment above.

[mattreynolds, 2017-01-18 22:22:29]

Description was changed from

==========
Replace deprecated calls to GetMetadata in Physical Web Zine provider

Use GetMetadataList instead of GetMetadata, which is now deprecated.

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

to

==========
Replace deprecated calls to GetMetadata in Physical Web Zine provider

Use GetMetadataList instead of GetMetadata, which is now deprecated.

BUG=672012
==========

[mattreynolds, 2017-01-18 22:26:13]

Thanks Vitalii!

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:38:
return page_metadata.resolved_url.spec();
On 2017/01/18 08:03:34, vitaliii (OOO until 23rd) wrote:
> This looks so much better now, thank you for the new data interface!

No problem!

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:208:
return left.distance_estimate < right.distance_estimate;
On 2017/01/18 11:50:50, vitaliii (OOO until 23rd) wrote:
> On 2017/01/18 08:03:34, vitaliii (OOO until 23rd) wrote:
> > Since now it is known that distance_estimate may be <= 0, could you compare
> them
> > such that <= 0 ones are last? Also adding a test would be nice.
> > 
> > You could achieve this by e.g.:
> > bool is_left_estimated = left.distance_estimate > 0;
> > bool is_right_estimated = right.distance_estimate > 0;
> > if (is_left_estimated != is_right_estimated) return is_left_estimated;
> > return left.distance_estimate < right.distance_estimate;
> > 
> 
> I am actually addressing this in https://codereview.chromium.org/2640773002/,
> since you do not have to land yours before BP.
> If it lands first, feel free to ignore the comment above.

Thanks for the heads up. Looks like yours has landed so I've rebased on top of
it.

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h
(right):

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/18 08:03:34, vitaliii (OOO until 23rd) wrote:
> I have another bug for this change (crbug.com/672012). Please add it to the
> description.

Done.

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:20:
class GURL;
On 2017/01/18 08:03:34, vitaliii (OOO until 23rd) wrote:
> Please sort.

Done.

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:25:
}
On 2017/01/18 08:03:34, vitaliii (OOO until 23rd) wrote:
> }  // namespace physical_web

Done.

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2643453002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:150:
physical_web_data_source()->SetMetadata(
On 2017/01/18 08:03:35, vitaliii (OOO until 23rd) wrote:
> I believe tests currently does not work, because SetMetadata does not seem to
> impact GetMetadataList.
> 
> I guess you would like to get rid of the old interface completely, so it would
> be nice to rewrite this to use SetMetadataList and all CreateDummy.* helpers
to
> return Metadata.

Oops, yes, tests were broken. I've now moved the tests over to
physical_web::MetadataList and will remove the unused parts of
FakePhysicalWebDataSource in a future CL.

[mattreynolds, 2017-01-18 22:26:49]

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

[mattreynolds, 2017-01-18 22:28:50]

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

[mattreynolds, 2017-01-18 22:28:50]

Hi Marc, could you take over this CL from Vitalii?  PTAL at
components/ntp_snippets

Olivier, PTAL at components/physical_web

[Marc Treib, 2017-01-19 10:50:04]

On 2017/01/18 22:28:50, mattreynolds wrote:
> Hi Marc, could you take over this CL from Vitalii?  PTAL at
> components/ntp_snippets
> 
> Olivier, PTAL at components/physical_web

Very nice! ntp_snippets LGTM

[Olivier, 2017-01-20 16:24:14]

components/physical_web LGTM

[mattreynolds, 2017-01-20 18:16:42]

The CQ bit was checked by mattreynolds@chromium.org

[commit-bot: I haz the power, 2017-01-20 18:17:07]

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-20 18:59:21]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484936202246140,
"parent_rev": "2c06df626151088d15a0e134dbd7c49350d0f217", "commit_rev":
"e11c34fb2c29ea47003a20391f0c7222b14b1e13"}

[commit-bot: I haz the power, 2017-01-20 18:59:56]

Description was changed from

==========
Replace deprecated calls to GetMetadata in Physical Web Zine provider

Use GetMetadataList instead of GetMetadata, which is now deprecated.

BUG=672012
==========

to

==========
Replace deprecated calls to GetMetadata in Physical Web Zine provider

Use GetMetadataList instead of GetMetadata, which is now deprecated.

BUG=672012

Review-Url: https://codereview.chromium.org/2643453002
Cr-Commit-Position: refs/heads/master@{#445113}
Committed:
https://chromium.googlesource.com/chromium/src/+/e11c34fb2c29ea47003a20391f0c...
==========

[commit-bot: I haz the power, 2017-01-20 18:59:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e11c34fb2c29ea47003a20391f0c...