Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650453002:
Add data usage tracking for image fetcher service (Closed)
DescriptionAdd data usage tracking for image fetcher service
BUG=655749
Review-Url: https://codereview.chromium.org/2650453002
Cr-Commit-Position: refs/heads/master@{#445435}
Committed: https://chromium.googlesource.com/chromium/src/+/d5be401a40c154be41657992cb271a6b3a128f32
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rebased and fixed nits #
Total comments: 3
Patch Set 3 : rebased #
Total comments: 2
Patch Set 4 : Fixed treib@ comments #
Messages
Total messages: 26 (14 generated)
rajendrant@chromium.org changed reviewers: + holte@chromium.org, treib@chromium.org
treib: ptal components/image_fetcher/image_data_fetcher.cc holte: ptal histograms.xml
The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rajendrant@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: ptal components/data_use_measurement/core/*
lgtm % nits https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... File components/data_use_measurement/core/data_use_user_data.cc (right): https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... components/data_use_measurement/core/data_use_user_data.cc:106: return "INVALID"; Does this "INVALID" ever happen? If not, you may want to add NOTREACHED() before returning INVALID. https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... components/data_use_measurement/core/data_use_user_data.h:53: IMAGE_FETCHER_UNTAGGED, What does it mean by "Untagged?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms lgtm
https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... File components/data_use_measurement/core/data_use_user_data.cc (right): https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... components/data_use_measurement/core/data_use_user_data.cc:106: return "INVALID"; On 2017/01/20 20:09:21, tbansal1 wrote: > Does this "INVALID" ever happen? > If not, you may want to add NOTREACHED() before returning INVALID. Makes sense. Done https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... components/data_use_measurement/core/data_use_user_data.h:53: IMAGE_FETCHER_UNTAGGED, On 2017/01/20 20:09:21, tbansal1 wrote: > What does it mean by "Untagged? Image fetcher is used by a bunch of other components (NTP, Search, Suggestions, etc). The users can set the type via ImageFetcher::SetDataUseServiceName(). If it is not set, it will be recorded as IMAGE_FETCHER_UNTAGGED instead of NOT_TAGGED. If the usage is more, it can be further broken out for the other services.
https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/2650453002/diff/1/components/data_use_measure... components/data_use_measurement/core/data_use_user_data.h:53: IMAGE_FETCHER_UNTAGGED, On 2017/01/20 23:37:12, Raj wrote: > On 2017/01/20 20:09:21, tbansal1 wrote: > > What does it mean by "Untagged? > > Image fetcher is used by a bunch of other components (NTP, Search, Suggestions, > etc). The users can set the type via ImageFetcher::SetDataUseServiceName(). If > it is not set, it will be recorded as IMAGE_FETCHER_UNTAGGED instead of > NOT_TAGGED. > > If the usage is more, it can be further broken out for the other services. Thanks for the explanation. You may want to add that as a comment somewhere.
lgtm % nit https://codereview.chromium.org/2650453002/diff/20001/components/data_use_mea... File components/data_use_measurement/core/data_use_user_data.cc (right): https://codereview.chromium.org/2650453002/diff/20001/components/data_use_mea... components/data_use_measurement/core/data_use_user_data.cc:113: return "INVALID"; I *think* it should be possible to remove this return "INVALID" statement. If you remove it, compiler would complain in future if you add an enum to the list of values, but forget to add here.
The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
https://codereview.chromium.org/2650453002/diff/20001/components/data_use_mea... File components/data_use_measurement/core/data_use_user_data.cc (right): https://codereview.chromium.org/2650453002/diff/20001/components/data_use_mea... components/data_use_measurement/core/data_use_user_data.cc:110: return "Image Fetcher Untagged"; nit: None of the other strings have spaces. So maybe "ImageFetcherUntagged"? https://codereview.chromium.org/2650453002/diff/20001/components/data_use_mea... components/data_use_measurement/core/data_use_user_data.cc:113: return "INVALID"; On 2017/01/21 00:30:38, tbansal1 wrote: > I *think* it should be possible to remove this return "INVALID" statement. If > you remove it, compiler would complain in future if you add an enum to the list > of values, but forget to add here. I think the compiler would throw an error anyway if the switch above doesn't handle all enum entries. https://codereview.chromium.org/2650453002/diff/40001/components/image_fetche... File components/image_fetcher/image_data_fetcher.cc (right): https://codereview.chromium.org/2650453002/diff/40001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:54: if (data_use_service_name_ != DataUseUserData::IMAGE_FETCHER_UNTAGGED) { I think the "if" should be removed now. As it is, IMAGE_FETCHER_UNTAGGED requests will not get a data usage tag at all.
treib: ptal https://codereview.chromium.org/2650453002/diff/40001/components/image_fetche... File components/image_fetcher/image_data_fetcher.cc (right): https://codereview.chromium.org/2650453002/diff/40001/components/image_fetche... components/image_fetcher/image_data_fetcher.cc:54: if (data_use_service_name_ != DataUseUserData::IMAGE_FETCHER_UNTAGGED) { On 2017/01/23 10:39:02, Marc Treib wrote: > I think the "if" should be removed now. As it is, IMAGE_FETCHER_UNTAGGED > requests will not get a data usage tag at all. Done.
image_fetcher lgtm
The CQ bit was checked by rajendrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2650453002/#ps60001 (title: "Fixed treib@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485195273160600,
"parent_rev": "d0e46de3ff797f53376fa63297a5d41241568fdf", "commit_rev":
"d5be401a40c154be41657992cb271a6b3a128f32"}
Message was sent while issue was closed.
Description was changed from ========== Add data usage tracking for image fetcher service BUG=655749 ========== to ========== Add data usage tracking for image fetcher service BUG=655749 Review-Url: https://codereview.chromium.org/2650453002 Cr-Commit-Position: refs/heads/master@{#445435} Committed: https://chromium.googlesource.com/chromium/src/+/d5be401a40c154be41657992cb27... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d5be401a40c154be41657992cb27... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
