Allow server endpoint to be injected into the NetMetricsLogUploader from chrome/

Allow server endpoint to be injected into the NetMetricsLogUploader from chrome/

As discussed in https://codereview.chromium.org/866163002/, this change will
allow consumer of the metrics component to configure the metrics uploader endpoint
without modifying the component.

BUG=453987
Committed: https://crrev.com/1fbce4562b2a12e9a40cca38b3dcebd71dd02063
Cr-Commit-Position: refs/heads/master@{#316915}

[ivansham, 2015-01-30 23:16:42]

ivansham@amazon.com changed reviewers:
+ asvitkine@chromium.org

[ivansham, 2015-01-30 23:16:42]

Hi Alexei,
Here's an implementation of the changes for pulling out the url constant as
suggested in https://codereview.chromium.org/866163002/.  WDYT?

PTAL

[gunsch, 2015-01-31 20:02:29]

On 2015/01/30 23:16:42, isham wrote:
> Hi Alexei,
> Here's an implementation of the changes for pulling out the url constant as
> suggested in https://codereview.chromium.org/866163002/.  WDYT?
> 
> PTAL

If you make this change, can you also change CastMetricsServiceClient in
chromecast/ ?

[Alexei Svitkine (slow), 2015-02-02 15:07:18]

asvitkine@chromium.org changed reviewers:
+ gunsch@chromium.org

[Alexei Svitkine (slow), 2015-02-02 15:07:18]

asvitkine@chromium.org changed required reviewers:
+ gunsch@chromium.org

[Alexei Svitkine (slow), 2015-02-02 15:07:19]

LGTM

[Alexei Svitkine (slow), 2015-02-02 15:09:12]

not lgtm

https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
File chrome/browser/metrics/url_constants.h (right):

https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
chrome/browser/metrics/url_constants.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
It's 2015.

https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
chrome/browser/metrics/url_constants.h:8: namespace metrics {
Nit: This actually shouldn't have namespace metrics, which is reserved for the
the component.

[gunsch, 2015-02-02 15:55:56]

On 2015/02/02 15:09:12, Alexei Svitkine wrote:
> not lgtm
> 
>
https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
> File chrome/browser/metrics/url_constants.h (right):
> 
>
https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
> chrome/browser/metrics/url_constants.h:1: // Copyright 2014 The Chromium
> Authors. All rights reserved.
> It's 2015.
> 
>
https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
> chrome/browser/metrics/url_constants.h:8: namespace metrics {
> Nit: This actually shouldn't have namespace metrics, which is reserved for the
> the component.

I'm still unclear on how this makes the URL any more "injectable" than before.
Just like in the Chromecast code, a content embedder can already create a
MetricsLogUploader pointed elsewhere. Are you trying to change the URL that the
"chrome" binary uses itself?

[ivansham, 2015-02-02 20:33:50]

On 2015/02/02 15:55:56, gunsch wrote:
> On 2015/02/02 15:09:12, Alexei Svitkine wrote:
> > not lgtm
> > 
> >
>
https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
> > File chrome/browser/metrics/url_constants.h (right):
> > 
> >
>
https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
> > chrome/browser/metrics/url_constants.h:1: // Copyright 2014 The Chromium
> > Authors. All rights reserved.
> > It's 2015.
> > 
> >
>
https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
> > chrome/browser/metrics/url_constants.h:8: namespace metrics {
> > Nit: This actually shouldn't have namespace metrics, which is reserved for
the
> > the component.
> 
> I'm still unclear on how this makes the URL any more "injectable" than before.
> Just like in the Chromecast code, a content embedder can already create a
> MetricsLogUploader pointed elsewhere. Are you trying to change the URL that
the
> "chrome" binary uses itself?

I think that's a fair point.  It makes sense for both chrome and chromecast to
share a single constant.
As such we probably don't need the constructor injection.

To better explain our motivation for this change, we would like to make sure
that all url constants are defined in url_constants.{h, cc} files, a convention
that already appears to exist in chromium.  This will get us better visibility
into which third-party urls are included in the product.

WDYT about adding a url_constants file for the metrics component?

[ivansham, 2015-02-03 21:41:56]

Added new patch set, PTAL.  Thanks.

[ivansham, 2015-02-03 21:41:58]

Added new patch set, PTAL.  Thanks.

[ivansham, 2015-02-04 00:43:01]

addressed comments

https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
File chrome/browser/metrics/url_constants.h (right):

https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
chrome/browser/metrics/url_constants.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/02/02 15:09:11, asvitkine (slow this week) wrote:
> It's 2015.

Done.

https://codereview.chromium.org/886173002/diff/1/chrome/browser/metrics/url_c...
chrome/browser/metrics/url_constants.h:8: namespace metrics {
On 2015/02/02 15:09:12, asvitkine (slow this week) wrote:
> Nit: This actually shouldn't have namespace metrics, which is reserved for the
> the component.

i moved this file to components/metrics so that it can be shared by chrome/ and
chromecast/ (and kept the metrics namespace).

[Alexei Svitkine (slow), 2015-02-09 19:13:16]

asvitkine@chromium.org changed reviewers:
+ mal@chromium.org

[Alexei Svitkine (slow), 2015-02-09 19:13:16]

+mal@ for the AUTHORS clarification.

Otherwise, I'm OK with this change.

https://codereview.chromium.org/886173002/diff/20001/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/886173002/diff/20001/AUTHORS#newcode198
AUTHORS:198: Ivan Sham <ivansham@amazon.com>
Sorry, you don't appear to be listed on the corporate CLA for amazon.com, which
is needed per:

http://www.chromium.org/developers/contributing-code/external-contributor-che...

Could you address that and get listed?

Also, I wonder if Amazon should just have a corporate entry at the bottom of
this file instead of listing authors individually. cc'ing mal@chromium.org for
clarification.

[Mark Larson, 2015-02-09 19:24:03]

On 2015/02/09 19:13:16, Alexei Svitkine wrote:
> +mal@ for the AUTHORS clarification.
> 
> Otherwise, I'm OK with this change.
> 
> https://codereview.chromium.org/886173002/diff/20001/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/886173002/diff/20001/AUTHORS#newcode198
> AUTHORS:198: Ivan Sham <mailto:ivansham@amazon.com>
> Sorry, you don't appear to be listed on the corporate CLA for
http://amazon.com, which
> is needed per:
> 
>
http://www.chromium.org/developers/contributing-code/external-contributor-che...
> 
> Could you address that and get listed?
> 
> Also, I wonder if Amazon should just have a corporate entry at the bottom of
> this file instead of listing authors individually. cc'ing
mailto:mal@chromium.org for
> clarification.

No. Amazon has an authorized contributors list, not a blanket authorization. So
folks need to be listed individually and they need to be on the list in Amazon's
CLA.

[gunsch, 2015-02-09 21:36:12]

https://codereview.chromium.org/886173002/diff/20001/chromecast/browser/metri...
File chromecast/browser/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/886173002/diff/20001/chromecast/browser/metri...
chromecast/browser/metrics/cast_metrics_service_client.cc:138: std::string
uma_server_url(server_url);
this won't compile.

[ivansham, 2015-02-10 17:39:24]

thanks for pointing that out.  can you point me to instructions for compiling /
testing chromecast targets as well?  i want to make sure no targets are broken.

https://codereview.chromium.org/886173002/diff/20001/chromecast/browser/metri...
File chromecast/browser/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/886173002/diff/20001/chromecast/browser/metri...
chromecast/browser/metrics/cast_metrics_service_client.cc:138: std::string
uma_server_url(server_url);
On 2015/02/09 21:36:11, gunsch wrote:
> this won't compile.

Done.

[gunsch, 2015-02-10 17:54:10]

build/gyp_chromium -Dchromecast=1
ninja -C out/Debug cast_shell

https://codereview.chromium.org/886173002/diff/20001/chromecast/browser/metri...
File chromecast/browser/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/886173002/diff/20001/chromecast/browser/metri...
chromecast/browser/metrics/cast_metrics_service_client.cc:138: std::string
uma_server_url(server_url);
On 2015/02/10 17:39:24, ivansham wrote:
> On 2015/02/09 21:36:11, gunsch wrote:
> > this won't compile.
> 
> Done.

No new patch set?

[ivansham, 2015-02-10 17:54:38]

On 2015/02/09 19:24:03, Mark Larson wrote:
> On 2015/02/09 19:13:16, Alexei Svitkine wrote:
> > +mal@ for the AUTHORS clarification.
> > 
> > Otherwise, I'm OK with this change.
> > 
> > https://codereview.chromium.org/886173002/diff/20001/AUTHORS
> > File AUTHORS (right):
> > 
> > https://codereview.chromium.org/886173002/diff/20001/AUTHORS#newcode198
> > AUTHORS:198: Ivan Sham <mailto:ivansham@amazon.com>
> > Sorry, you don't appear to be listed on the corporate CLA for
> http://amazon.com, which
> > is needed per:
> > 
> >
>
http://www.chromium.org/developers/contributing-code/external-contributor-che...
> > 
> > Could you address that and get listed?
> > 
> > Also, I wonder if Amazon should just have a corporate entry at the bottom of
> > this file instead of listing authors individually. cc'ing
> mailto:mal@chromium.org for
> > clarification.
> 
> No. Amazon has an authorized contributors list, not a blanket authorization.
So
> folks need to be listed individually and they need to be on the list in
Amazon's
> CLA.

asvitkine@ / mal@:  i just checked that i am in the Amazon CCLA Authorized
Contributor group (listed under ivansham).  i also just changed the nickname for
this account to ivansham to avoid confusion.

[Alexei Svitkine (slow), 2015-02-10 18:11:33]

Hmm, I don't see you in the internal spreadsheet. mal@ - could you check
what's wrong there?

On Tue, Feb 10, 2015 at 12:54 PM, <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2015/02/09 19:24:03, Mark Larson wrote:
>
>> On 2015/02/09 19:13:16, Alexei Svitkine wrote:
>> > +mal@ for the AUTHORS clarification.
>> >
>> > Otherwise, I'm OK with this change.
>> >
>> > https://codereview.chromium.org/886173002/diff/20001/AUTHORS
>> > File AUTHORS (right):
>> >
>> > https://codereview.chromium.org/886173002/diff/20001/AUTHORS#newcode198
>> > AUTHORS:198: Ivan Sham <mailto:ivansham@amazon.com>
>> > Sorry, you don't appear to be listed on the corporate CLA for
>> http://amazon.com, which
>> > is needed per:
>> >
>> >
>>
>
> http://www.chromium.org/developers/contributing-code/
> external-contributor-checklist
>
>> >
>> > Could you address that and get listed?
>> >
>> > Also, I wonder if Amazon should just have a corporate entry at the
>> bottom of
>> > this file instead of listing authors individually. cc'ing
>> mailto:mal@chromium.org for
>> > clarification.
>>
>
>  No. Amazon has an authorized contributors list, not a blanket
>> authorization.
>>
> So
>
>> folks need to be listed individually and they need to be on the list in
>>
> Amazon's
>
>> CLA.
>>
>
> asvitkine@ / mal@:  i just checked that i am in the Amazon CCLA Authorized
> Contributor group (listed under ivansham).  i also just changed the
> nickname for
> this account to ivansham to avoid confusion.
>
> https://codereview.chromium.org/886173002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ivansham, 2015-02-10 21:01:22]

fixed the compilation problems with chromecast, PTAL.  thanks!

[gunsch, 2015-02-10 21:09:07]

chromecast/ lgtm

[Mark Larson, 2015-02-16 22:09:57]

LGTM from a CLA standpoint

[ivansham, 2015-02-16 22:34:32]

The CQ bit was checked by ivansham@amazon.com

[ivansham, 2015-02-16 22:34:33]

The CQ bit was unchecked by ivansham@amazon.com

[ivansham, 2015-02-16 22:39:33]

The CQ bit was checked by ivansham@amazon.com

[ivansham, 2015-02-16 22:39:33]

The CQ bit was unchecked by ivansham@amazon.com

[commit-bot: I haz the power, 2015-02-16 22:40:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886173002/40001

[commit-bot: I haz the power, 2015-02-16 22:40:15]

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

[commit-bot: I haz the power, 2015-02-16 22:40:17]

A disapproval has been posted by following reviewers: asvitkine@chromium.org.
Please make sure to get positive review before another CQ attempt.

[Alexei Svitkine (slow), 2015-02-17 00:46:18]

lgtm

[Alexei Svitkine (slow), 2015-02-17 00:46:45]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-02-17 00:47:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886173002/40001

[commit-bot: I haz the power, 2015-02-17 01:02:41]

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

[commit-bot: I haz the power, 2015-02-17 01:02:42]

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

[ivansham, 2015-02-17 18:02:58]

New patchsets have been uploaded after l-g-t-m from
gunsch@chromium.org,mal@chromium.org,asvitkine@chromium.org

[ivansham, 2015-02-17 18:04:11]

added the missing entries to the gn file

[ivansham, 2015-02-17 18:04:18]

The CQ bit was checked by ivansham@amazon.com

[ivansham, 2015-02-17 18:04:29]

The CQ bit was unchecked by ivansham@amazon.com

[ivansham, 2015-02-17 19:56:14]

The CQ bit was checked by ivansham@amazon.com

[commit-bot: I haz the power, 2015-02-17 19:57:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886173002/60001

[commit-bot: I haz the power, 2015-02-17 21:32:04]

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

[commit-bot: I haz the power, 2015-02-17 21:32:05]

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

[ivansham, 2015-02-18 22:37:36]

The CQ bit was checked by ivansham@amazon.com

[commit-bot: I haz the power, 2015-02-18 22:39:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886173002/60001

[commit-bot: I haz the power, 2015-02-18 23:27:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-18 23:28:01]

Patchset 4 (id:??) landed as
https://crrev.com/1fbce4562b2a12e9a40cca38b3dcebd71dd02063
Cr-Commit-Position: refs/heads/master@{#316915}