ntp_snippets: default to staging API on dev/canary

ntp_snippets: default to staging API on dev/canary

BUG=690406
Review-Url: https://codereview.chromium.org/2718453002
Cr-Commit-Position: refs/heads/master@{#454337}
Committed: https://chromium.googlesource.com/chromium/src/+/cf829c4a52608579f667e6f313a03f412117f926

[sfiera, 2017-02-23 13:06:07]

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

[commit-bot: I haz the power, 2017-02-23 13:06:18]

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

[sfiera, 2017-02-23 13:08:23]

sfiera@chromium.org changed reviewers:
+ jkrcal@chromium.org, treib@chromium.org

[sfiera, 2017-02-23 13:08:24]

I believe crrev.com/2665743002 changed the variation parameter names such that
canary and dev's settings were no longer respected They should go to the staging
server, not the prod server.

Once this split was established, we should have stopped using variation
parameters for this anyway. Fortunately, by definition there is no need to merge
this.

[jkrcal, 2017-02-23 13:25:22]

lgtm % nits

Brilliant, thanks!

https://codereview.chromium.org/2718453002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2718453002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:305:
base::Bind(&ParseJsonDelayed), GetFetchEndpoint(true), kAPIKey,
nit: could you please add /*is_stable_or_beta_channel=*/?

https://codereview.chromium.org/2718453002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2718453002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:417:
base::Bind(&ParseJson), GetFetchEndpoint(true), kAPIKey,
nit: could you please add /*is_stable_or_beta_channel=*/?

[sfiera, 2017-02-23 13:28:47]

https://codereview.chromium.org/2718453002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2718453002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:305:
base::Bind(&ParseJsonDelayed), GetFetchEndpoint(true), kAPIKey,
On 2017/02/23 13:25:22, jkrcal wrote:
> nit: could you please add /*is_stable_or_beta_channel=*/?

Done.

https://codereview.chromium.org/2718453002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2718453002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:417:
base::Bind(&ParseJson), GetFetchEndpoint(true), kAPIKey,
On 2017/02/23 13:25:22, jkrcal wrote:
> nit: could you please add /*is_stable_or_beta_channel=*/?

Done.

[sfiera, 2017-02-23 13:30:27]

Description was changed from

==========
ntp_snippets: default to staging API on dev/canary

BUG=none
==========

to

==========
ntp_snippets: default to staging API on dev/canary

BUG=690406
==========

[Marc Treib, 2017-02-23 13:57:14]

https://codereview.chromium.org/2718453002/diff/20001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2718453002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:193:
GetFetchEndpoint(chrome::GetChannel() >= version_info::Channel::BETA),
nit: Can we list STABLE and BETA explicitly? IMO it's very non-obvious which way
the enum is ordered.

https://codereview.chromium.org/2718453002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_fetcher.h (right):

https://codereview.chromium.org/2718453002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_fetcher.h:90: const
std::string& api_endpoint,
Should this be a GURL?

https://codereview.chromium.org/2718453002/diff/20001/ios/chrome/browser/ntp_...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2718453002/diff/20001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:162:
GetFetchEndpoint(GetChannel() >= version_info::Channel::BETA),
Hm, can we just pass the channel into GetFetchEndpoint, so that we don't have to
duplicate this logic?

[sfiera, 2017-02-23 14:48:09]

sfiera@chromium.org changed reviewers:
+ sky@chromium.org

[sfiera, 2017-02-23 14:48:10]

+sky for version_info DEPS

https://codereview.chromium.org/2718453002/diff/20001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2718453002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:193:
GetFetchEndpoint(chrome::GetChannel() >= version_info::Channel::BETA),
On 2017/02/23 13:57:14, Marc Treib wrote:
> nit: Can we list STABLE and BETA explicitly? IMO it's very non-obvious which
way
> the enum is ordered.

Obsolete.

https://codereview.chromium.org/2718453002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_fetcher.h (right):

https://codereview.chromium.org/2718453002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_fetcher.h:90: const
std::string& api_endpoint,
On 2017/02/23 13:57:14, Marc Treib wrote:
> Should this be a GURL?

Sure.

https://codereview.chromium.org/2718453002/diff/20001/ios/chrome/browser/ntp_...
File
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/2718453002/diff/20001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:162:
GetFetchEndpoint(GetChannel() >= version_info::Channel::BETA),
On 2017/02/23 13:57:14, Marc Treib wrote:
> Hm, can we just pass the channel into GetFetchEndpoint, so that we don't have
to
> duplicate this logic?

Well, if we add a dep on components/version_info.

[sfiera, 2017-02-23 14:48:22]

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

[commit-bot: I haz the power, 2017-02-23 14:48:36]

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

[Marc Treib, 2017-02-23 14:56:43]

lgtm

Thanks!

https://codereview.chromium.org/2718453002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2718453002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:201: return
GURL{endpoint};
A fan of new-style initialization syntax? Haven't seen that around much :)

[sfiera, 2017-02-23 15:14:20]

https://codereview.chromium.org/2718453002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2718453002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:201: return
GURL{endpoint};
On 2017/02/23 14:56:42, Marc Treib wrote:
> A fan of new-style initialization syntax? Haven't seen that around much :)

Well, actually, I first tried {endpoint}, but it didn't work (“GURL is explicit
in copy-initialization”). I added GURL but didn't bother changing the brackets;
in general, I think {} is a good idea because it limits implicit conversions.

[commit-bot: I haz the power, 2017-02-23 16:18:48]

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

[commit-bot: I haz the power, 2017-02-23 16:18:49]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-23 20:12:48]

LGTM

[sfiera, 2017-02-24 09:35:28]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-02-24 09:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2718453002/#ps40001
(title: "Use channel directly")

[commit-bot: I haz the power, 2017-02-24 09:35:42]

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-02-24 09:41:47]

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

[commit-bot: I haz the power, 2017-02-24 09:41:49]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sfiera, 2017-02-24 10:34:01]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-02-24 10:34:01]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
jkrcal@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2718453002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-24 10:34:17]

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-02-24 10:40:29]

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

[commit-bot: I haz the power, 2017-02-24 10:40:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sfiera, 2017-02-27 10:26:55]

sfiera@chromium.org changed reviewers:
+ noyau@chromium.org

[sfiera, 2017-02-27 10:26:56]

+noyau for ios approval

Should we add file://components/ntp_snippets/OWNERS to the ios
ntp_snippets/OWNERS?

[noyau (Ping after 24h), 2017-03-01 16:57:51]

lgtm

[sfiera, 2017-03-02 17:22:52]

The CQ bit was checked by sfiera@chromium.org

[commit-bot: I haz the power, 2017-03-02 17:23:14]

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-03-02 18:25:38]

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

[commit-bot: I haz the power, 2017-03-02 18:25:40]

Failed to apply patch for components/ntp_snippets/DEPS:
While running git apply --index -p1;
  error: patch failed: components/ntp_snippets/DEPS:11
  error: components/ntp_snippets/DEPS: patch does not apply

Patch:       components/ntp_snippets/DEPS
Index: components/ntp_snippets/DEPS
diff --git a/components/ntp_snippets/DEPS b/components/ntp_snippets/DEPS
index
97081df22c4840c2be64634fce4c835b7d1fae9e..ac2c5ac225b8109e2c588615ceb84941956f638d
100644
--- a/components/ntp_snippets/DEPS
+++ b/components/ntp_snippets/DEPS
@@ -11,6 +11,8 @@ include_rules = [
   "+components/sync/driver",
   "+components/translate/core/browser",
   "+components/variations",
+  "+components/version_info",
+  "+grit/components_strings.h",
   "+net/base",
   "+net/http",
   "+net/url_request",

[sfiera, 2017-03-02 18:34:36]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-03-02 18:34:37]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
jkrcal@chromium.org, noyau@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2718453002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-02 18:34:49]

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-03-02 19:29:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488479676642680,
"parent_rev": "ee2bbc8ee23bb9361b5cb78feb37099d4a94b150", "commit_rev":
"cf829c4a52608579f667e6f313a03f412117f926"}

[commit-bot: I haz the power, 2017-03-02 19:30:01]

Description was changed from

==========
ntp_snippets: default to staging API on dev/canary

BUG=690406
==========

to

==========
ntp_snippets: default to staging API on dev/canary

BUG=690406

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

[commit-bot: I haz the power, 2017-03-02 19:30:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cf829c4a52608579f667e6f313a0...