Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2510053002:
Add a count for Suggested Articles usage. (Closed)
|
Created:
4 years, 1 month ago by markusheintz_ Modified:
4 years, 1 month ago CC:
chromium-reviews, asvitkine+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a count for Suggested Articles usage.
BUG=665873
Committed: https://crrev.com/b0ed4eb7afa96fadc7404597c9b5d3b01db1e547
Cr-Commit-Position: refs/heads/master@{#433182}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix histogram name const #Patch Set 3 : Improve the user action description #Patch Set 4 : drop the count suffic from the action name #Patch Set 5 : sync #Messages
Total messages: 25 (11 generated)
Description was changed from ========== Add a count for Suggested Articles usage. BUG=665873 ========== to ========== Add a count for Suggested Articles usage. BUG=665873 ==========
markusheintz@chromium.org changed reviewers: + treib@chromium.org
PTAL
https://codereview.chromium.org/2510053002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2510053002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:17: using base::UserMetricsAction; nit: I'd prefer specifying the "base::" in the one place where it's used below. https://codereview.chromium.org/2510053002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:180: UMA_HISTOGRAM_ENUMERATION(kHistogramArticlesUsageTimeLocal, bucket, Bad rebase? You renamed the constant here, but not above where it's defined. https://codereview.chromium.org/2510053002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:184: UserMetricsAction("NewTabPage_ContentSuggestions_ArticlesUsageCount")); "Count" is implied by the fact that it's a UserAction :) "NewTabPage_ContentSuggestions_ArticlesUsed" ? https://codereview.chromium.org/2510053002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2510053002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10290: Tab Page. Usage is defined by scrolling to the section and opening an nit: s/and/or/ (as it is, IMO it sounds like it'd only be recorded after both events have happened, but in fact it's recorded for the scroll and then again for the click)
Fix histogram name const
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...
Improve the user action description
drop the count suffic from the action name
https://codereview.chromium.org/2510053002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2510053002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:17: using base::UserMetricsAction; On 2016/11/17 11:10:23, Marc Treib wrote: > nit: I'd prefer specifying the "base::" in the one place where it's used below. done https://codereview.chromium.org/2510053002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:180: UMA_HISTOGRAM_ENUMERATION(kHistogramArticlesUsageTimeLocal, bucket, On 2016/11/17 11:10:23, Marc Treib wrote: > Bad rebase? You renamed the constant here, but not above where it's defined. Done. https://codereview.chromium.org/2510053002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:184: UserMetricsAction("NewTabPage_ContentSuggestions_ArticlesUsageCount")); On 2016/11/17 11:10:23, Marc Treib wrote: > "Count" is implied by the fact that it's a UserAction :) > "NewTabPage_ContentSuggestions_ArticlesUsed" ? Done. https://codereview.chromium.org/2510053002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2510053002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:10290: Tab Page. Usage is defined by scrolling to the section and opening an On 2016/11/17 11:10:23, Marc Treib wrote: > nit: s/and/or/ > (as it is, IMO it sounds like it'd only be recorded after both events have > happened, but in fact it's recorded for the scroll and then again for the click) I change the description PTAL
LGTM, thanks!
markusheintz@chromium.org changed reviewers: + holte@chromium.org
On 2016/11/17 13:54:02, Marc Treib wrote: > LGTM, thanks! Steven could you take a look at the actions.xml changes pls. Thanks
lgtm
The CQ bit was checked by markusheintz@chromium.org
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
Failed to apply the patch.
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/2510053002/#ps80001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a count for Suggested Articles usage. BUG=665873 ========== to ========== Add a count for Suggested Articles usage. BUG=665873 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add a count for Suggested Articles usage. BUG=665873 ========== to ========== Add a count for Suggested Articles usage. BUG=665873 Committed: https://crrev.com/b0ed4eb7afa96fadc7404597c9b5d3b01db1e547 Cr-Commit-Position: refs/heads/master@{#433182} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b0ed4eb7afa96fadc7404597c9b5d3b01db1e547 Cr-Commit-Position: refs/heads/master@{#433182} |
