Use GURLs in Physical Web data source

Use GURLs in Physical Web data source

The Physical Web data source passes URLs to listeners, currently in the
form of strings.  This change passes them around as GURLs.

BUG=667722
Committed: https://crrev.com/343cfde27b2dc5779a5cff12f0d511b57e3b3deb
Cr-Commit-Position: refs/heads/master@{#438313}

[cco3, 2016-12-10 00:14:53]

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

[cco3, 2016-12-10 00:15:45]

On 2016/12/10 00:14:53, cco3 wrote:

I didn't run into anything ios specific...is there any test or anything I could
run on Linux to trigger the relevant breakages?

[vitaliii, 2016-12-12 06:56:38]

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

[vitaliii, 2016-12-12 06:56:38]

LGTM with a nit. Thanks!

You will have to wait for OWNER approval (Marc, could you have a look once
everything else is ready?).

As for iOS related files, there are some here
https://codereview.chromium.org/2527323003/

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

https://codereview.chromium.org/2565023002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:9:
#include <string>
Please leave this.
There are other places where the std::string is used.

[Marc Treib, 2016-12-12 10:20:25]

Thanks! LGTM after vitaliii's nit is fixed.

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

https://codereview.chromium.org/2565023002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:20:
#include "url/gurl.h"
optional: Could just forward-declare
class GURL;

[mattreynolds, 2016-12-12 18:24:29]

lgtm

[cco3, 2016-12-12 19:00:31]

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

https://codereview.chromium.org/2565023002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:9:
#include <string>
On 2016/12/12 06:56:38, vitaliii wrote:
> Please leave this.
> There are other places where the std::string is used.

Done.

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

https://codereview.chromium.org/2565023002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:20:
#include "url/gurl.h"
On 2016/12/12 10:20:25, Marc Treib wrote:
> optional: Could just forward-declare
> class GURL;

Done.

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

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

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

Hi Tommy, could you please review the android file?  Thanks!

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

The CQ bit was checked by cco3@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 19:03:56]

Dry run: 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, 2016-12-12 19:09:16]

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

[commit-bot: I haz the power, 2016-12-12 19:09:17]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[cco3, 2016-12-12 20:12:30]

The CQ bit was checked by cco3@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 20:13:37]

Dry run: 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, 2016-12-12 23:24:20]

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

[commit-bot: I haz the power, 2016-12-12 23:24:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nyquist, 2016-12-13 18:01:41]

lgtm

https://codereview.chromium.org/2565023002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2565023002/diff/40001/components/ntp_snippets...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:280:
InvalidateSuggestion(url.spec());
Nit: This seems to be the only caller of InvalidateSuggestion. Any reason why it
doesn't take a const GURL& and does the spec internally instead?

[cco3, 2016-12-13 19:53:41]

https://codereview.chromium.org/2565023002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2565023002/diff/40001/components/ntp_snippets...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:280:
InvalidateSuggestion(url.spec());
On 2016/12/13 18:01:40, nyquist wrote:
> Nit: This seems to be the only caller of InvalidateSuggestion. Any reason why
it
> doesn't take a const GURL& and does the spec internally instead?

I think a page_id is supposed to be a string rather than a URL, so it would make
sense that we pass a string.  Anyway, vitaliii can change this later if he wants
to.

[cco3, 2016-12-13 19:53:53]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-12-13 19:53:53]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
vitaliii@chromium.org, mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/2565023002/#ps40001
(title: "Register missing dependencies")

[commit-bot: I haz the power, 2016-12-13 19:54:34]

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, 2016-12-13 22:26:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481658833062960,
"parent_rev": "8e153f12469b578ad1c0cbbf55eb3baf447d73a8", "commit_rev":
"9488a25e9ef0b8e237bbcbae5c203f685d7a3346"}

[commit-bot: I haz the power, 2016-12-13 22:27:09]

Description was changed from

==========
Use GURLs in Physical Web data source

The Physical Web data source passes URLs to listeners, currently in the
form of strings.  This change passes them around as GURLs.

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

to

==========
Use GURLs in Physical Web data source

The Physical Web data source passes URLs to listeners, currently in the
form of strings.  This change passes them around as GURLs.

BUG=667722

Review-Url: https://codereview.chromium.org/2565023002
==========

[commit-bot: I haz the power, 2016-12-13 22:27:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-13 22:29:41]

Description was changed from

==========
Use GURLs in Physical Web data source

The Physical Web data source passes URLs to listeners, currently in the
form of strings.  This change passes them around as GURLs.

BUG=667722

Review-Url: https://codereview.chromium.org/2565023002
==========

to

==========
Use GURLs in Physical Web data source

The Physical Web data source passes URLs to listeners, currently in the
form of strings.  This change passes them around as GURLs.

BUG=667722

Committed: https://crrev.com/343cfde27b2dc5779a5cff12f0d511b57e3b3deb
Cr-Commit-Position: refs/heads/master@{#438313}
==========

[commit-bot: I haz the power, 2016-12-13 22:29:43]

Patchset 3 (id:??) landed as
https://crrev.com/343cfde27b2dc5779a5cff12f0d511b57e3b3deb
Cr-Commit-Position: refs/heads/master@{#438313}