Report data usage UMA for Chrome services

Report data usage UMA for Chrome services

data_use_measurement is a component which records the data use of the
services, if a DataUseUserData is attached to their request. Hence to
measure and compare data use of different services, a small change is
needed in the services' code where they create a URLFetcher. After
creating a URLFetcher by a service, a proper tag for that service should
be attached to the request.
Besides the necessary changes in the service, the enum in DataUseUserData
and the function returning the names based on this enum should be updated
for different services too. Also the name of target services should be
added to proper enum and histograms in histograms.xml file.
Lastly the dependency files should be updated accordingly too.

TBR=bartfab@chromium.org, andrewhayden@chromium.org, rogerta@chromium.org, isherman@chromium.org, rouslan@chromium.org, estade@chromium.org, nyquist@chromium.org, pkasting@chromium.org, zea@chromium.org, mmenke@chromium.org, bengr@chromium.org, timvolodine@chromium.org

BUG=527304
Committed: https://crrev.com/aa789c8328a65922859dfd8ca2d9b18212dc3c94
Cr-Commit-Position: refs/heads/master@{#348561}

[amohammadkhan, 2015-09-04 21:48:06]

amohammadkhan@google.com changed reviewers:
+ andrewhayden@chromium.org, asvitkine@chromium.org, bartfab@chromium.org,
bengr@chromium.org, estade@chromium.org, isherman@chromium.org,
mmenke@chromium.org, nyquist@chromium.org, pkasting@chromium.org,
rogerta@chromium.org, rouslan@chromium.org, sclittle@chromium.org,
thestig@chromium.org, timvolodine@chromium.org, zea@chromium.org

[amohammadkhan, 2015-09-04 21:48:06]

rogerta@chromium.org: Please review changes in 
    google_apis/
isherman@chromium.org: Please review changes in 
    components/google/
pkasting@chromium.org: Please review changes in 
    components/omnibox/
nyquist@chromium.org: Please review changes in 
    components/invalidation
rouslan@chromium.org: Please review changes in 
    chrome/browser/spellchecker
bartfab@chromium.org: Please review changes in 
    components/policy/
zea@chromium.org: Please review changes in 
    sync/
    google-apis/gcm/
mmenke@chromium.org: Please review changes in 
    chrome/browser/profiles/
estade@chromium.org: Please review changes in 
    components/autofill/
thestig@chromium.org: Please review changes in 
    chrome/browser/
timvolodine@chromium.org: Please review changes in 
    content/browser/geolocation
asvitkine@chromium.org: Please review changes in 
    components/metrics/
    components/rappor/
    chrome/browser/metrics/variations/
    histograms.xml
andrewhayden@chromium.org: Please review changes in 
    components/translate/
sclittle@chromium.org: Please review changes in 
    components/data_use_measurement/
bengr@chromium.org: Please review changes in 
    components/data_use_measurement/

[Lei Zhang, 2015-09-04 21:56:52]

Instead of having 15 reviewers, please consider having just 1 or 2 primary
reviewer(s) and TBRing the rest.

[Ilya Sherman, 2015-09-04 22:10:40]

https://codereview.chromium.org/1330443002/diff/20001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/1330443002/diff/20001/components/autofill.gyp...
components/autofill.gypi:72:
'../components/components.gyp:data_use_measurement_core',
nit: Add this below, with the other components dependencies?

https://codereview.chromium.org/1330443002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1330443002/diff/20001/components/autofill/cor...
components/autofill/core/browser/BUILD.gn:146:
"//components/data_use_measurement/core",
nit: Please alphabetize

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.cc (right):

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.cc:66: default:
nit: Please remove the default case -- that way, the clang compiler will
actually issue a compiler error if you forget to list a case.

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:18: class
DataUseUserData : public base::SupportsUserData::Data {
Optional nit: Perhaps "DataUseService" would be a clearer name for this class?

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:36: URL_TRACKER,
nit: Let's name this GOOGLE_URL_TRACKER, to be clearer about what service this
is.

https://codereview.chromium.org/1330443002/diff/20001/components/google.gypi
File components/google.gypi (right):

https://codereview.chromium.org/1330443002/diff/20001/components/google.gypi#...
components/google.gypi:15: 'components.gyp:data_use_measurement_core',
nit: Can this be moved below, to match e.g. keyed_service_core?

https://codereview.chromium.org/1330443002/diff/20001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/1330443002/diff/20001/components/metrics.gypi...
components/metrics.gypi:18: 'components.gyp:data_use_measurement_core',
nit: Can "components.gyp" be omitted?

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55243: +  <int value="12" label="URL
Tracker"/>
nit: "Google URL Tracker"

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55245: +  <int value="14"
label="Variations"/>
nit: It's a little odd to separate UMA, Rappor, and Variations from one another,
since they're all facets of the same service.  Any particular reason you didn't
choose to group them together?

[sclittle, 2015-09-04 22:18:14]

https://codereview.chromium.org/1330443002/diff/20001/components/invalidation...
File components/invalidation/impl/DEPS (right):

https://codereview.chromium.org/1330443002/diff/20001/components/invalidation...
components/invalidation/impl/DEPS:2: "+components/data_use_measurement/core",
nit: move this down above the "+components/gcm_driver" line, since the
"+components/invalidation/public" DEP is special.

https://codereview.chromium.org/1330443002/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/1330443002/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.cc:88: url_fetcher_.get(),
data_use_measurement::DataUseUserData::SUGGESTIONS);
This looks like the wrong enum value.

[amohammadkhan, 2015-09-04 23:03:17]

sclittle@, thestig@, asvltkine@ PTAL.

Great idea. I kept sclittle@, thestig@, asvltkine@ as primary reviewers and
added the rest to TBR. Please let me know if it is better to change it.

https://codereview.chromium.org/1330443002/diff/20001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/1330443002/diff/20001/components/autofill.gyp...
components/autofill.gypi:72:
'../components/components.gyp:data_use_measurement_core',
On 2015/09/04 22:10:39, Ilya Sherman wrote:
> nit: Add this below, with the other components dependencies?

Done.

https://codereview.chromium.org/1330443002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1330443002/diff/20001/components/autofill/cor...
components/autofill/core/browser/BUILD.gn:146:
"//components/data_use_measurement/core",
On 2015/09/04 22:10:39, Ilya Sherman wrote:
> nit: Please alphabetize

Done.

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.cc (right):

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.cc:66: default:
On 2015/09/04 22:10:39, Ilya Sherman wrote:
> nit: Please remove the default case -- that way, the clang compiler will
> actually issue a compiler error if you forget to list a case.

Done.

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:18: class
DataUseUserData : public base::SupportsUserData::Data {
On 2015/09/04 22:10:39, Ilya Sherman wrote:
> Optional nit: Perhaps "DataUseService" would be a clearer name for this class?

I see your point, but for now I think it is better to keep the old name.

https://codereview.chromium.org/1330443002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:36: URL_TRACKER,
On 2015/09/04 22:10:39, Ilya Sherman wrote:
> nit: Let's name this GOOGLE_URL_TRACKER, to be clearer about what service this
> is.

Done.

https://codereview.chromium.org/1330443002/diff/20001/components/google.gypi
File components/google.gypi (right):

https://codereview.chromium.org/1330443002/diff/20001/components/google.gypi#...
components/google.gypi:15: 'components.gyp:data_use_measurement_core',
On 2015/09/04 22:10:39, Ilya Sherman wrote:
> nit: Can this be moved below, to match e.g. keyed_service_core?

Done.

https://codereview.chromium.org/1330443002/diff/20001/components/invalidation...
File components/invalidation/impl/DEPS (right):

https://codereview.chromium.org/1330443002/diff/20001/components/invalidation...
components/invalidation/impl/DEPS:2: "+components/data_use_measurement/core",
On 2015/09/04 22:18:14, sclittle wrote:
> nit: move this down above the "+components/gcm_driver" line, since the
> "+components/invalidation/public" DEP is special.

Done.

https://codereview.chromium.org/1330443002/diff/20001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/1330443002/diff/20001/components/metrics.gypi...
components/metrics.gypi:18: 'components.gyp:data_use_measurement_core',
On 2015/09/04 22:10:39, Ilya Sherman wrote:
> nit: Can "components.gyp" be omitted?

Done.

https://codereview.chromium.org/1330443002/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/1330443002/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/unregistration_request.cc:88: url_fetcher_.get(),
data_use_measurement::DataUseUserData::SUGGESTIONS);
On 2015/09/04 22:18:14, sclittle wrote:
> This looks like the wrong enum value.

Done.

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55243: +  <int value="12" label="URL
Tracker"/>
On 2015/09/04 22:10:40, Ilya Sherman wrote:
> nit: "Google URL Tracker"

Done.

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55245: +  <int value="14"
label="Variations"/>
On 2015/09/04 22:10:39, Ilya Sherman wrote:
> nit: It's a little odd to separate UMA, Rappor, and Variations from one
another,
> since they're all facets of the same service.  Any particular reason you
didn't
> choose to group them together?

Yes. By having them separated, the contribution of each one is more clear and it
will help me understand different sources of chatter in chrome more precisely.
My ideal goal was even having more granularity but it was not possible for now.

[sclittle, 2015-09-04 23:17:57]

Please reword the title of this CL - "Necessary changes in the services to
support data use measurement" is vague. Instead, maybe something more like
"Report data usage UMA for Chrome services" or something.

Update the title in the CL description as well.

https://codereview.chromium.org/1330443002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.cc (right):

https://codereview.chromium.org/1330443002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.cc:55: return
"URLTracker";
nit: Change to "GoogleURLTracker" to match the enum.

https://codereview.chromium.org/1330443002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/1330443002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:28: COMPLETE,
nit: It's not clear what service COMPLETE links to, maybe you could call it
OMNIBOX_COMPLETION or something?

https://codereview.chromium.org/1330443002/diff/40001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/1330443002/diff/40001/components/metrics.gypi...
components/metrics.gypi:18: 'data_use_measurement_core',
nit: alphabetize

[amohammadkhan, 2015-09-04 23:28:01]

https://codereview.chromium.org/1330443002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.cc (right):

https://codereview.chromium.org/1330443002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.cc:55: return
"URLTracker";
On 2015/09/04 23:17:57, sclittle wrote:
> nit: Change to "GoogleURLTracker" to match the enum.

Done.

https://codereview.chromium.org/1330443002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/1330443002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:28: COMPLETE,
On 2015/09/04 23:17:57, sclittle wrote:
> nit: It's not clear what service COMPLETE links to, maybe you could call it
> OMNIBOX_COMPLETION or something?

Done.

https://codereview.chromium.org/1330443002/diff/40001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/1330443002/diff/40001/components/metrics.gypi...
components/metrics.gypi:18: 'data_use_measurement_core',
On 2015/09/04 23:17:57, sclittle wrote:
> nit: alphabetize

Done.

[sclittle, 2015-09-04 23:34:17]

https://codereview.chromium.org/1330443002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/base_search_provider.cc (right):

https://codereview.chromium.org/1330443002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider.cc:64: deletion_fetcher_.get(),
data_use_measurement::DataUseUserData::COMPLETE);
Update these enum values to say OMNIBOX.

https://codereview.chromium.org/1330443002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1330443002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55235: +  <int value="4"
label="Complete(OmniBox)"/>
nit: change label here to "Omnibox"

[bartfab (slow), 2015-09-07 14:27:19]

https://codereview.chromium.org/1330443002/diff/60001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/1330443002/diff/60001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:39: DEVICE_MANAGEMENT,
What are the expected semantics of |DEVICE_MANAGEMENT| and |POLICY|? You used
|DEVICE_MANAGEMENT| for the core URL fetchers that talk to the policy server and
|POLICY| for auxiliary fetchers that talk to other services. I find that
strange.

[Andrew Hayden (chromium.org), 2015-09-07 15:28:48]

components/translate/* LGTM

[please use gerrit instead, 2015-09-07 15:55:35]

chrome/browser/spellchecker/* LGTM

[amohammadkhan, 2015-09-08 13:26:55]

https://codereview.chromium.org/1330443002/diff/60001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/1330443002/diff/60001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:39: DEVICE_MANAGEMENT,
On 2015/09/07 14:27:19, bartfab wrote:
> What are the expected semantics of |DEVICE_MANAGEMENT| and |POLICY|? You used
> |DEVICE_MANAGEMENT| for the core URL fetchers that talk to the policy server
and
> |POLICY| for auxiliary fetchers that talk to other services. I find that
> strange.

I wanted to have a breakdown between different types of request in this service.
But I think you are right and it is not necessary, so I changed all requests'
tags to |POLICY|.

https://codereview.chromium.org/1330443002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/base_search_provider.cc (right):

https://codereview.chromium.org/1330443002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider.cc:64: deletion_fetcher_.get(),
data_use_measurement::DataUseUserData::COMPLETE);
On 2015/09/04 23:34:17, sclittle wrote:
> Update these enum values to say OMNIBOX.

Done.

https://codereview.chromium.org/1330443002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1330443002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55235: +  <int value="4"
label="Complete(OmniBox)"/>
On 2015/09/04 23:34:17, sclittle wrote:
> nit: change label here to "Omnibox"

Done.

[bartfab (slow), 2015-09-08 15:05:00]

components/policy/* LGTM

[Roger Tawa OOO till Jul 10th, 2015-09-08 15:21:04]

Hi Ali,

//google_apis should not depend on //components/...

Also, I don't think GOOGLE_APIS is a good service type.  It's too generic. 
Would be better to have (optional?) argument passed into to google_api classes
that callers can specify, much like the |source| arg to ctors of many of the
google_apis\gaia classes.

another option would be to add a (pure?) virtual method like
GetDataReporterServiceType() to OAuth2AccessTokenConsumer and GaiaAuthConsumer
and OAuth2ApiCallFlow to have callers return an appropriate value.

[amohammadkhan, 2015-09-08 17:07:52]

On 2015/09/08 15:21:04, Roger Tawa wrote:
> Hi Ali,
> 
> //google_apis should not depend on //components/...
> 
> Also, I don't think GOOGLE_APIS is a good service type.  It's too generic. 
> Would be better to have (optional?) argument passed into to google_api classes
> that callers can specify, much like the |source| arg to ctors of many of the
> google_apis\gaia classes.
> 
> another option would be to add a (pure?) virtual method like
> GetDataReporterServiceType() to OAuth2AccessTokenConsumer and GaiaAuthConsumer
> and OAuth2ApiCallFlow to have callers return an appropriate value.

Hi Roger,

I see. So for now, I reverted the changes in //google_apis.

[Nicolas Zea, 2015-09-08 17:13:57]

sync/ shouldn't really be depending on components/. You might be able to get
around that by plumbing a base::Closure (which wraps your static method) down
through the various sync objects.

[Roger Tawa OOO till Jul 10th, 2015-09-08 17:16:34]

On 2015/09/08 17:07:52, amohammadkhan wrote:
> On 2015/09/08 15:21:04, Roger Tawa wrote:
> > Hi Ali,
> > 
> > //google_apis should not depend on //components/...
> > 
> > Also, I don't think GOOGLE_APIS is a good service type.  It's too generic. 
> > Would be better to have (optional?) argument passed into to google_api
classes
> > that callers can specify, much like the |source| arg to ctors of many of the
> > google_apis\gaia classes.
> > 
> > another option would be to add a (pure?) virtual method like
> > GetDataReporterServiceType() to OAuth2AccessTokenConsumer and
GaiaAuthConsumer
> > and OAuth2ApiCallFlow to have callers return an appropriate value.
> 
> Hi Roger,
> 
> I see. So for now, I reverted the changes in //google_apis.

OK.  So nothing left for me to review really.  Thanks.

[Alexei Svitkine (slow), 2015-09-08 17:26:47]

LGTM, looks great!

[nyquist, 2015-09-08 17:31:26]

components/invalidation lgtm

[Alexei Svitkine (slow), 2015-09-08 17:34:30]

Actually, on a closer look, found a couple of things.

https://codereview.chromium.org/1330443002/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/1330443002/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_service.cc:5: #include
"chrome/browser/metrics/variations/variations_service.h"
You need to rebase. This file has moved to components.

https://codereview.chromium.org/1330443002/diff/100001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/1330443002/diff/100001/components/metrics.gyp...
components/metrics.gypi:20: 'data_use_measurement_core',
Nit: Can this be moved to metrics_net target below?

https://codereview.chromium.org/1330443002/diff/100001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/1330443002/diff/100001/components/metrics/BUI...
components/metrics/BUILD.gn:68: "//components/data_use_measurement/core",
Nit: Can this dependency be moved to static_library("net") below?

[bengr, 2015-09-08 18:26:45]

Lgtm, modulo Alexei's comments.

[sclittle, 2015-09-08 18:28:29]

https://codereview.chromium.org/1330443002/diff/100001/components/data_use_me...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/1330443002/diff/100001/components/data_use_me...
components/data_use_measurement/core/data_use_user_data.h:33: GOOGLE_APIS,
Remove the GOOGLE_APIS enum value here if you're not adding the google_apis
reporting yet. Same with the histograms.

[Lei Zhang, 2015-09-08 18:35:28]

chrome/ lgtm

https://codereview.chromium.org/1330443002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/DEPS (right):

https://codereview.chromium.org/1330443002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/DEPS:3: "+components/data_use_measurement/core",
You don't need this. The same rule is already in chrome/browser/DEPS.

[Ilya Sherman, 2015-09-08 21:45:26]

autofill lgtm, with a couple of nits on the remainder of the CL:

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55245: +  <int value="14"
label="Variations"/>
On 2015/09/04 23:03:16, amohammadkhan wrote:
> On 2015/09/04 22:10:39, Ilya Sherman wrote:
> > nit: It's a little odd to separate UMA, Rappor, and Variations from one
> another,
> > since they're all facets of the same service.  Any particular reason you
> didn't
> > choose to group them together?
> 
> Yes. By having them separated, the contribution of each one is more clear and
it
> will help me understand different sources of chatter in chrome more precisely.
> My ideal goal was even having more granularity but it was not possible for
now.

Sorry, I see now that my question wasn't clear.  What I meant was: Would it make
sense to sort this list so that these related services are adjacent to one
another?

https://codereview.chromium.org/1330443002/diff/100001/components/data_use_me...
File components/data_use_measurement/core/data_use_user_data.cc (right):

https://codereview.chromium.org/1330443002/diff/100001/components/data_use_me...
components/data_use_measurement/core/data_use_user_data.cc:65: NOTREACHED() <<
"Unknown service type in DataUseUserData";
nit: Please omit at least the string after the NOTREACHED -- it adds weight to
the binary, without providing any additional information beyond what a comment
could do.  Better yet, IMO, would be to remove the NOTREACHED() stmt entirely,
since the clang compiler will actually enforce that this code is not reachable.

[amohammadkhan, 2015-09-08 23:28:31]

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1330443002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55245: +  <int value="14"
label="Variations"/>
On 2015/09/08 21:45:25, Ilya Sherman wrote:
> On 2015/09/04 23:03:16, amohammadkhan wrote:
> > On 2015/09/04 22:10:39, Ilya Sherman wrote:
> > > nit: It's a little odd to separate UMA, Rappor, and Variations from one
> > another,
> > > since they're all facets of the same service.  Any particular reason you
> > didn't
> > > choose to group them together?
> > 
> > Yes. By having them separated, the contribution of each one is more clear
and
> it
> > will help me understand different sources of chatter in chrome more
precisely.
> > My ideal goal was even having more granularity but it was not possible for
> now.
> 
> Sorry, I see now that my question wasn't clear.  What I meant was: Would it
make
> sense to sort this list so that these related services are adjacent to one
> another?

Sorry I didn't understand first time what you meant. I am not sure about the
relations between other services but I made UMA/Variations/Rapport adjacent.

https://codereview.chromium.org/1330443002/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/1330443002/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/variations/variations_service.cc:5: #include
"chrome/browser/metrics/variations/variations_service.h"
On 2015/09/08 17:34:30, Alexei Svitkine wrote:
> You need to rebase. This file has moved to components.

Done.

https://codereview.chromium.org/1330443002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/DEPS (right):

https://codereview.chromium.org/1330443002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/DEPS:3: "+components/data_use_measurement/core",
On 2015/09/08 18:35:28, Lei Zhang wrote:
> You don't need this. The same rule is already in chrome/browser/DEPS.

Done.

https://codereview.chromium.org/1330443002/diff/100001/components/data_use_me...
File components/data_use_measurement/core/data_use_user_data.cc (right):

https://codereview.chromium.org/1330443002/diff/100001/components/data_use_me...
components/data_use_measurement/core/data_use_user_data.cc:65: NOTREACHED() <<
"Unknown service type in DataUseUserData";
On 2015/09/08 21:45:25, Ilya Sherman wrote:
> nit: Please omit at least the string after the NOTREACHED -- it adds weight to
> the binary, without providing any additional information beyond what a comment
> could do.  Better yet, IMO, would be to remove the NOTREACHED() stmt entirely,
> since the clang compiler will actually enforce that this code is not
reachable.

Done.

https://codereview.chromium.org/1330443002/diff/100001/components/data_use_me...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/1330443002/diff/100001/components/data_use_me...
components/data_use_measurement/core/data_use_user_data.h:33: GOOGLE_APIS,
On 2015/09/08 18:28:28, sclittle wrote:
> Remove the GOOGLE_APIS enum value here if you're not adding the google_apis
> reporting yet. Same with the histograms.

Done.

https://codereview.chromium.org/1330443002/diff/100001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/1330443002/diff/100001/components/metrics.gyp...
components/metrics.gypi:20: 'data_use_measurement_core',
On 2015/09/08 17:34:30, Alexei Svitkine wrote:
> Nit: Can this be moved to metrics_net target below?

Done.

https://codereview.chromium.org/1330443002/diff/100001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/1330443002/diff/100001/components/metrics/BUI...
components/metrics/BUILD.gn:68: "//components/data_use_measurement/core",
On 2015/09/08 17:34:30, Alexei Svitkine wrote:
> Nit: Can this dependency be moved to static_library("net") below?

Done.

[Alexei Svitkine (slow), 2015-09-09 15:05:00]

lgtm

[amohammadkhan, 2015-09-09 18:49:22]

Zea@ based on your very helpful guidance I did plumbing for Sync service, PTAL.

https://codereview.chromium.org/1330443002/diff/140001/chrome/browser/sync/gl...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/1330443002/diff/140001/chrome/browser/sync/gl...
chrome/browser/sync/glue/sync_backend_host_core.cc:43: // namespace syncer
I'll move the comment up in next CL upload.

[Nicolas Zea, 2015-09-09 19:56:51]

Sync LGTM with some nits, thanks for fixing that up.

https://codereview.chromium.org/1330443002/diff/140001/sync/internal_api/publ...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/1330443002/diff/140001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:59:
base::Callback<void(net::URLFetcher*)> bind_to_tracker_callback);
nit: pass the callback by const ref here and elsewhere, e.g. const
base::Callback<void(net::URLFetcher*)>&. Also consider making a typedef for the
callback and using that instead of redefining the callback type everywhere.

[amohammadkhan, 2015-09-09 20:41:33]

On 2015/09/09 19:56:51, Nicolas Zea wrote:
> Sync LGTM with some nits, thanks for fixing that up.
> 
>
https://codereview.chromium.org/1330443002/diff/140001/sync/internal_api/publ...
> File sync/internal_api/public/http_bridge.h (right):
> 
>
https://codereview.chromium.org/1330443002/diff/140001/sync/internal_api/publ...
> sync/internal_api/public/http_bridge.h:59:
> base::Callback<void(net::URLFetcher*)> bind_to_tracker_callback);
> nit: pass the callback by const ref here and elsewhere, e.g. const
> base::Callback<void(net::URLFetcher*)>&. Also consider making a typedef for
the
> callback and using that instead of redefining the callback type everywhere.

@zea: Can I use network_time_update_callback.h to place the definition of my
callback? If not, do you have any suggestion?

[Nicolas Zea, 2015-09-09 21:32:52]

On 2015/09/09 20:41:33, amohammadkhan wrote:
> On 2015/09/09 19:56:51, Nicolas Zea wrote:
> > Sync LGTM with some nits, thanks for fixing that up.
> > 
> >
>
https://codereview.chromium.org/1330443002/diff/140001/sync/internal_api/publ...
> > File sync/internal_api/public/http_bridge.h (right):
> > 
> >
>
https://codereview.chromium.org/1330443002/diff/140001/sync/internal_api/publ...
> > sync/internal_api/public/http_bridge.h:59:
> > base::Callback<void(net::URLFetcher*)> bind_to_tracker_callback);
> > nit: pass the callback by const ref here and elsewhere, e.g. const
> > base::Callback<void(net::URLFetcher*)>&. Also consider making a typedef for
> the
> > callback and using that instead of redefining the callback type everywhere.
> 
> @zea: Can I use network_time_update_callback.h to place the definition of my
> callback? If not, do you have any suggestion?

No, network_time_update_callback.h shouldn't be used for this. Can't you use
http_bridge.h?

[amohammadkhan, 2015-09-09 22:21:57]

https://codereview.chromium.org/1330443002/diff/140001/sync/internal_api/publ...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/1330443002/diff/140001/sync/internal_api/publ...
sync/internal_api/public/http_bridge.h:59:
base::Callback<void(net::URLFetcher*)> bind_to_tracker_callback);
On 2015/09/09 19:56:51, Nicolas Zea wrote:
> nit: pass the callback by const ref here and elsewhere, e.g. const
> base::Callback<void(net::URLFetcher*)>&. Also consider making a typedef for
the
> callback and using that instead of redefining the callback type everywhere.

Done. I put the typedef of my callback in http_post_provider_factory.h, it
caused minimum change to other files, please let me know if it is not a good
place for it.

[Nicolas Zea, 2015-09-09 23:00:52]

One more nit, else LGTM

https://codereview.chromium.org/1330443002/diff/160001/sync/internal_api/publ...
File sync/internal_api/public/http_post_provider_factory.h (right):

https://codereview.chromium.org/1330443002/diff/160001/sync/internal_api/publ...
sync/internal_api/public/http_post_provider_factory.h:17: typedef
base::Callback<void(net::URLFetcher*)> BindToTrackerCallback;
nit: move this into syncer namespace

[sclittle, 2015-09-09 23:19:53]

lgtm

[timvolodine, 2015-09-10 12:16:40]

content/browser/geolocation/ lgtm

[Peter Kasting, 2015-09-10 21:09:35]

LGTM

[amohammadkhan, 2015-09-10 23:08:37]

https://codereview.chromium.org/1330443002/diff/160001/sync/internal_api/publ...
File sync/internal_api/public/http_post_provider_factory.h (right):

https://codereview.chromium.org/1330443002/diff/160001/sync/internal_api/publ...
sync/internal_api/public/http_post_provider_factory.h:17: typedef
base::Callback<void(net::URLFetcher*)> BindToTrackerCallback;
On 2015/09/09 23:00:52, Nicolas Zea wrote:
> nit: move this into syncer namespace

Done.

[mmenke, 2015-09-11 14:02:08]

On 2015/09/10 23:08:37, amohammadkhan wrote:
>
https://codereview.chromium.org/1330443002/diff/160001/sync/internal_api/publ...
> File sync/internal_api/public/http_post_provider_factory.h (right):
> 
>
https://codereview.chromium.org/1330443002/diff/160001/sync/internal_api/publ...
> sync/internal_api/public/http_post_provider_factory.h:17: typedef
> base::Callback<void(net::URLFetcher*)> BindToTrackerCallback;
> On 2015/09/09 23:00:52, Nicolas Zea wrote:
> > nit: move this into syncer namespace
> 
> Done.

domain_reliability, profiles LGTM

[amohammadkhan, 2015-09-11 21:10:30]

The CQ bit was checked by amohammadkhan@google.com

[amohammadkhan, 2015-09-11 21:10:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
andrewhayden@chromium.org, rouslan@chromium.org, bartfab@chromium.org,
thestig@chromium.org, isherman@chromium.org, nyquist@chromium.org,
bengr@chromium.org, asvitkine@chromium.org, sclittle@chromium.org,
timvolodine@chromium.org, pkasting@chromium.org, zea@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1330443002/#ps220001
(title: "Fixing the problem in previous rebase.")

[commit-bot: I haz the power, 2015-09-11 21:10:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/220001

[commit-bot: I haz the power, 2015-09-11 21:24:02]

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

[commit-bot: I haz the power, 2015-09-11 21:24:05]

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

[sclittle, 2015-09-11 22:09:06]

still lgtm

[amohammadkhan, 2015-09-11 22:10:46]

The CQ bit was checked by amohammadkhan@google.com

[amohammadkhan, 2015-09-11 22:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, andrewhayden@chromium.org, thestig@chromium.org,
isherman@chromium.org, rouslan@chromium.org, timvolodine@chromium.org,
nyquist@chromium.org, bengr@chromium.org, asvitkine@chromium.org,
pkasting@chromium.org, zea@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1330443002/#ps260001
(title: "Removing Geolocation Service from target services.")

[commit-bot: I haz the power, 2015-09-11 22:12:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-11 23:06:47]

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

[commit-bot: I haz the power, 2015-09-11 23:06:49]

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

[amohammadkhan, 2015-09-11 23:17:28]

The CQ bit was checked by amohammadkhan@google.com

[commit-bot: I haz the power, 2015-09-11 23:17:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-12 00:05:36]

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

[commit-bot: I haz the power, 2015-09-12 00:05:38]

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

[amohammadkhan, 2015-09-12 00:06:52]

The CQ bit was checked by amohammadkhan@google.com

[commit-bot: I haz the power, 2015-09-12 00:07:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-12 00:25:47]

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

[commit-bot: I haz the power, 2015-09-12 00:25:49]

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

[amohammadkhan, 2015-09-12 00:32:42]

The CQ bit was checked by amohammadkhan@google.com

[commit-bot: I haz the power, 2015-09-12 00:33:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-12 01:19:20]

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

[commit-bot: I haz the power, 2015-09-12 01:19:23]

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

[amohammadkhan1, 2015-09-12 01:27:00]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-12 01:27:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-12 02:31:45]

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

[commit-bot: I haz the power, 2015-09-12 02:31:48]

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

[amohammadkhan1, 2015-09-12 02:46:17]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-12 02:47:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-12 03:28:56]

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

[commit-bot: I haz the power, 2015-09-12 03:28:59]

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

[amohammadkhan1, 2015-09-12 03:44:59]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-12 03:46:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-12 04:33:04]

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

[commit-bot: I haz the power, 2015-09-12 04:33:06]

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

[amohammadkhan1, 2015-09-12 04:49:07]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-12 04:49:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-12 09:07:29]

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

[commit-bot: I haz the power, 2015-09-12 09:07:31]

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

[amohammadkhan1, 2015-09-12 15:06:26]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-12 15:06:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-12 21:08:14]

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

[commit-bot: I haz the power, 2015-09-12 21:08:16]

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

[amohammadkhan1, 2015-09-12 23:22:45]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-12 23:23:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-13 05:24:13]

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

[commit-bot: I haz the power, 2015-09-13 05:24:15]

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

[amohammadkhan1, 2015-09-13 05:52:59]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-13 05:53:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-13 11:54:39]

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

[commit-bot: I haz the power, 2015-09-13 11:54:42]

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

[amohammadkhan1, 2015-09-13 17:55:03]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-13 17:55:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-13 23:57:34]

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

[commit-bot: I haz the power, 2015-09-13 23:57:36]

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

[amohammadkhan1, 2015-09-14 00:09:40]

The CQ bit was checked by amohammadkhan@chromium.org

[commit-bot: I haz the power, 2015-09-14 00:09:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1330443002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1330443002/260001

[commit-bot: I haz the power, 2015-09-14 04:54:44]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-09-14 04:55:21]

Patchset 14 (id:??) landed as
https://crrev.com/aa789c8328a65922859dfd8ca2d9b18212dc3c94
Cr-Commit-Position: refs/heads/master@{#348561}

[kinuko, 2015-09-14 05:26:43]

Looks like this breads compile on multiple bots (conflicting change or trybot
may not have caught it):

http://build.chromium.org/p/chromium/builders/Win/builds/35609/steps/compile/...

> c:\b\build\slave\win\build\src\sync\tools\sync_client.cc(408) :error C2660:
'syncer::HttpPostProviderFactory::Init' : function does not take 1 arguments

[dcheng, 2015-09-14 05:40:25]

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

[dcheng, 2015-09-14 05:40:26]

This was manually reverted in https://codereview.chromium.org/1339943003

[commit-bot: I haz the power, 2015-09-23 12:29:23]

Patchset 15 (id:??) landed as
https://crrev.com/aa789c8328a65922859dfd8ca2d9b18212dc3c94
Cr-Commit-Position: refs/heads/master@{#348561}