Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2953083005:
Implementing sign-in promo histograms for settings (Closed)
|
Created:
3 years, 6 months ago by jlebel Modified:
3 years, 5 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplementing sign-in promo histograms for settings
Histograms implemented:
- Number of times the sign-in promo has been displayed when the settings view
is closed without user actions on the sign-in promo view.
- Number of times the sign-in promo has been displayed when the user taps on it
to sign-in.
Also after displaying the sign-in promo 20 times, the previous sign-in promo is
shown (smaller promo).
BUG=709286
Review-Url: https://codereview.chromium.org/2953083005
Cr-Commit-Position: refs/heads/master@{#483245}
Committed: https://chromium.googlesource.com/chromium/src/+/76ad8e535567e85334767cf5722f57206cc9d4ae
Patch Set 1 #
Total comments: 4
Patch Set 2 : . #Patch Set 3 : Renaming histograms #Patch Set 4 : Merge #
Total comments: 2
Patch Set 5 : Adding "iSO only" #
Depends on Patchset: Messages
Total messages: 56 (48 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from
==========
Adding histogram to setting view
BUG=709286
==========
to
==========
Implementing sign-in promo histograms for settings
Histograms implemented:
- Number of times the sign-in promo has been displayed when the settings view
is closed without user actions on the sign-in promo view.
- Number of times the sign-in promo has been displayed when the user taps on it
to sign-in.
Also the sign-in promo view is automatically hidden when after being displayed
20 times.
BUG=709286
==========
Description was changed from
==========
Implementing sign-in promo histograms for settings
Histograms implemented:
- Number of times the sign-in promo has been displayed when the settings view
is closed without user actions on the sign-in promo view.
- Number of times the sign-in promo has been displayed when the user taps on it
to sign-in.
Also the sign-in promo view is automatically hidden when after being displayed
20 times.
BUG=709286
==========
to
==========
Implementing sign-in promo histograms for settings
Histograms implemented:
- Number of times the sign-in promo has been displayed when the settings view
is closed without user actions on the sign-in promo view.
- Number of times the sign-in promo has been displayed when the user taps on it
to sign-in.
Also after displaying the sign-in promo 20 times, the previous sign-in promo is
shown (smaller promo).
BUG=709286
==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by jlebel@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jlebel@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jlebel@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jlebel@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.
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
The CQ bit was checked by jlebel@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.
jlebel@chromium.org changed reviewers: + sdefresne@chromium.org
Hello Sylvain, Can you review this patch? Thanks,
lgtm https://codereview.chromium.org/2953083005/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_collection_view_controller.mm (right): https://codereview.chromium.org/2953083005/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_collection_view_controller.mm:353: ++displayedCount; nit: it is clearer in my opinion to just pass "displayedCount + 1" and to mark the variable "displayedCount" as const prefs->SetInteger(prefs::kIosSettingsSeigninPromoDisplayedCount, displayedCount + 1); https://codereview.chromium.org/2953083005/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_collection_view_controller.mm:1209: - (void)sendCountTillSigninHistogram { nit: in your other CL and the histogram.xml file you used "Til" instead of "Till", maybe use the same naming convention here too
thanks https://codereview.chromium.org/2953083005/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_collection_view_controller.mm (right): https://codereview.chromium.org/2953083005/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_collection_view_controller.mm:353: ++displayedCount; On 2017/06/26 06:34:35, sdefresne wrote: > nit: it is clearer in my opinion to just pass "displayedCount + 1" and to mark > the variable "displayedCount" as const > > prefs->SetInteger(prefs::kIosSettingsSeigninPromoDisplayedCount, > displayedCount + 1); Done. https://codereview.chromium.org/2953083005/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_collection_view_controller.mm:1209: - (void)sendCountTillSigninHistogram { On 2017/06/26 06:34:35, sdefresne wrote: > nit: in your other CL and the histogram.xml file you used "Til" instead of > "Till", maybe use the same naming convention here too Yes, I definitely made a mistake here. Thanks.
Patchset #3 (id:220001) has been deleted
Patchset #3 (id:240001) has been deleted
jlebel@chromium.org changed reviewers: + rkaplow@chromium.org
Hello Robert, Can you review my patch to add new histograms to Chrome iOS? This time this is for settings view. Thanks,
The CQ bit was checked by jlebel@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jlebel@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.
lgtm https://codereview.chromium.org/2953083005/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2953083005/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:32774: + Counts how many times the signin promo is implicitly dismissed (by closing if these are only iOS, ca you mention in the summary.
I also added for the previous histograms. Thanks https://codereview.chromium.org/2953083005/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2953083005/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:32774: + Counts how many times the signin promo is implicitly dismissed (by closing On 2017/06/28 15:20:16, rkaplow wrote: > if these are only iOS, ca you mention in the summary. Done.
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2953083005/#ps300001 (title: "Adding "iSO only"")
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": 300001, "attempt_start_ts": 1498694468762670,
"parent_rev": "090e405d8593f529416c11b589134ab4b7232554", "commit_rev":
"e9cabf0797f6eef73d0bade2592c40e82d8e11e8"}
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1498694468762670,
"parent_rev": "94075a59632a747bd29f5947725d78484cb6fa40", "commit_rev":
"76ad8e535567e85334767cf5722f57206cc9d4ae"}
Message was sent while issue was closed.
Description was changed from
==========
Implementing sign-in promo histograms for settings
Histograms implemented:
- Number of times the sign-in promo has been displayed when the settings view
is closed without user actions on the sign-in promo view.
- Number of times the sign-in promo has been displayed when the user taps on it
to sign-in.
Also after displaying the sign-in promo 20 times, the previous sign-in promo is
shown (smaller promo).
BUG=709286
==========
to
==========
Implementing sign-in promo histograms for settings
Histograms implemented:
- Number of times the sign-in promo has been displayed when the settings view
is closed without user actions on the sign-in promo view.
- Number of times the sign-in promo has been displayed when the user taps on it
to sign-in.
Also after displaying the sign-in promo 20 times, the previous sign-in promo is
shown (smaller promo).
BUG=709286
Review-Url: https://codereview.chromium.org/2953083005
Cr-Commit-Position: refs/heads/master@{#483245}
Committed:
https://chromium.googlesource.com/chromium/src/+/76ad8e535567e85334767cf5722f...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:300001) as https://chromium.googlesource.com/chromium/src/+/76ad8e535567e85334767cf5722f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
