Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563443004:
Adds histograms for collecting data about Zine usage for each day of the week. (Closed)
|
Created:
4 years ago by markusheintz_ Modified:
4 years ago CC:
chromium-reviews, asvitkine+watch_chromium.org, ntp-dev+reviews_chromium.org, tschumann Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds histograms for collecting data about Zine usage for each day of the week.
The goal is to compare usage patterns between the different days of the week.
BUG=665873
Committed: https://crrev.com/a393080dc65d0a7f3db31a84b87a9ab66f76c683
Cr-Commit-Position: refs/heads/master@{#438157}
Patch Set 1 #Patch Set 2 : Add a fallback histogram #Patch Set 3 : " #
Total comments: 9
Patch Set 4 : Address comments treib #Patch Set 5 : rebase #Patch Set 6 : Run git cl format #
Total comments: 1
Patch Set 7 : rebase #Patch Set 8 : rebase #
Messages
Total messages: 37 (20 generated)
Description was changed from ========== Adds histograms for collecting data about Zine usage for each day of the week. The goal is to compare usage patterns between the different days of the week. BUG=665873 ========== to ========== Adds histograms for collecting data about Zine usage for each day of the week. The goal is to compare usage patterns between the different days of the week. BUG=665873 ==========
markusheintz@chromium.org changed reviewers: + treib@chromium.org, tschumann@chromium.org
PTAL
Add a fallback histogram
https://codereview.chromium.org/2563443004/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2563443004/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:219: std::string day_of_week_str("Unknown"); nit: Just leave this empty; it should never be used anyway. https://codereview.chromium.org/2563443004/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:220: switch (now_exploded.day_of_week) { optional: Make a static array kWeekdayNames and just index into that? Would be a bit less verbose (and also avoid the c-string -> std::string -> c-string roundtrip). I'm quite surprised that we don't have a helper function for this somewhere, but I can't find anything... https://codereview.chromium.org/2563443004/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:234: day_of_week_str = "Thrusday"; Thursday https://codereview.chromium.org/2563443004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2563443004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38362: +<histogram name="NewTabPage.ContentSuggestions.UsageTimeLocal.Friday"> You could specify a histogram_suffix, then these don't need to be listed individually. https://codereview.chromium.org/2563443004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38416: +<histogram name="NewTabPage.ContentSuggestions.UsageTimeLocal.Unknown"> I don't think this one is necessary - when would this ever happen?
Address comments treib
https://codereview.chromium.org/2563443004/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2563443004/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:219: std::string day_of_week_str("Unknown"); On 2016/12/09 12:43:59, Marc Treib wrote: > nit: Just leave this empty; it should never be used anyway. Done. https://codereview.chromium.org/2563443004/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:220: switch (now_exploded.day_of_week) { On 2016/12/09 12:43:59, Marc Treib wrote: > optional: Make a static array kWeekdayNames and just index into that? Would be a > bit less verbose (and also avoid the c-string -> std::string -> c-string > roundtrip). > > I'm quite surprised that we don't have a helper function for this somewhere, but > I can't find anything... Done I was also surprise. Maybe we both didn't search hard enough. Anyway we can always change the code in case we find something. https://codereview.chromium.org/2563443004/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:234: day_of_week_str = "Thrusday"; On 2016/12/09 12:43:59, Marc Treib wrote: > Thursday Done. https://codereview.chromium.org/2563443004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2563443004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38416: +<histogram name="NewTabPage.ContentSuggestions.UsageTimeLocal.Unknown"> On 2016/12/09 12:43:59, Marc Treib wrote: > I don't think this one is necessary - when would this ever happen? Removed
rebase
Run git cl format
markusheintz@chromium.org changed reviewers: + holte@chromium.org - tschumann@chromium.org
On 2016/12/09 15:12:33, markusheintz_ wrote: > Run git cl format Steven PTAL at the histograms.xml changes. Thanks
The CQ bit was checked by markusheintz@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: This issue passed the CQ dry run.
Thanks! LGTM https://codereview.chromium.org/2563443004/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2563443004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:221: base::StringPrintf("%s.%s", kHistogramArticlesUsageTimeLocal, optional: You could rename kPerCategoryHistogramFormat to (say) kHistogramWithSuffixFormat and reuse it here.
lgtm
rebase
The CQ bit was checked by markusheintz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2563443004/#ps120001 (title: "rebase")
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
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...)
The CQ bit was checked by markusheintz@chromium.org to run a CQ dry run
rebase
The CQ bit was checked by markusheintz@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: This issue passed the CQ dry run.
The CQ bit was checked by markusheintz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2563443004/#ps140001 (title: "rebase")
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": 140001, "attempt_start_ts": 1481637431140230,
"parent_rev": "4a252987acf5e874393f4cd02835d125c948973e", "commit_rev":
"cea6c591402f164b739ed895ef92980dafcf90d5"}
Message was sent while issue was closed.
Description was changed from ========== Adds histograms for collecting data about Zine usage for each day of the week. The goal is to compare usage patterns between the different days of the week. BUG=665873 ========== to ========== Adds histograms for collecting data about Zine usage for each day of the week. The goal is to compare usage patterns between the different days of the week. BUG=665873 Review-Url: https://codereview.chromium.org/2563443004 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Adds histograms for collecting data about Zine usage for each day of the week. The goal is to compare usage patterns between the different days of the week. BUG=665873 Review-Url: https://codereview.chromium.org/2563443004 ========== to ========== Adds histograms for collecting data about Zine usage for each day of the week. The goal is to compare usage patterns between the different days of the week. BUG=665873 Committed: https://crrev.com/a393080dc65d0a7f3db31a84b87a9ab66f76c683 Cr-Commit-Position: refs/heads/master@{#438157} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a393080dc65d0a7f3db31a84b87a9ab66f76c683 Cr-Commit-Position: refs/heads/master@{#438157} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
