Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(620)

Issue 2942923002: Implementing sign-in promo histograms for bookmark (Closed)

Created:
3 years, 6 months ago by jlebel
Modified:
3 years, 5 months ago
Reviewers:
rkaplow, sdefresne
CC:
chromium-reviews, marq+watch_chromium.org, tfarina, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Implementing sign-in promo histograms for bookmark Histograms implemented: - Number of times the sign-in promo has been displayed when the bookmark 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. - Number of times the sign-in promo has been displayed when the user taps on the X button to close it. Also the sign-in promo view is automatically hidden when after being displayed 20 times. BUG=709286 Review-Url: https://codereview.chromium.org/2942923002 Cr-Commit-Position: refs/heads/master@{#482994} Committed: https://chromium.googlesource.com/chromium/src/+/a24738a4699277f620a0ed12a183fa475be03580

Patch Set 1 #

Total comments: 16

Patch Set 2 : . #

Total comments: 4

Patch Set 3 : Adding spaces in comments #

Patch Set 4 : A little cleanup #

Total comments: 5

Patch Set 5 : Inline histograms #

Patch Set 6 : Fixing dependencies & tests #

Patch Set 7 : Merge #

Total comments: 9

Patch Set 8 : Renaming histograms #

Unified diffs Side-by-side diffs Delta from patch set Stats (+236 lines, -7 lines) Patch
M ios/chrome/browser/pref_names.h View 1 chunk +1 line, -0 lines 0 comments Download
M ios/chrome/browser/pref_names.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M ios/chrome/browser/prefs/browser_prefs.mm View 2 chunks +2 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/authentication/BUILD.gn View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h View 1 2 3 4 2 chunks +38 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm View 1 2 3 4 5 6 7 8 chunks +106 lines, -1 line 0 comments Download
M ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M ios/chrome/browser/ui/bookmarks/bookmark_collection_view.h View 3 chunks +12 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm View 1 2 3 4 5 chunks +31 lines, -2 lines 0 comments Download
M ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm View 1 chunk +2 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_table_view_controller.mm View 1 chunk +2 lines, -1 line 0 comments Download
M ios/chrome/browser/ui/settings/settings_collection_view_controller.mm View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm View 1 chunk +2 lines, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 1 chunk +29 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 72 (56 generated)
jlebel
Hello Sylvain, Can you review this patch? Thanks,
3 years, 6 months ago (2017-06-19 05:37:56 UTC) #9
sdefresne
https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h#newcode46 ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:46: @property(nonatomic) std::string countTilSigninHistogramName; Shouldn't this be "countTillSigninHistogramName"? https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h#newcode49 ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:49: ...
3 years, 6 months ago (2017-06-19 09:07:51 UTC) #11
jlebel
https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h#newcode46 ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:46: @property(nonatomic) std::string countTilSigninHistogramName; On 2017/06/19 09:07:50, sdefresne wrote: > ...
3 years, 6 months ago (2017-06-19 14:38:12 UTC) #12
sdefresne
lgtm lgtm https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h#newcode46 ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:46: @property(nonatomic) std::string countTilSigninHistogramName; On 2017/06/19 14:38:12, jlebel ...
3 years, 6 months ago (2017-06-22 12:40:39 UTC) #13
sdefresne
lgtm https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h#newcode46 ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:46: @property(nonatomic) std::string countTilSigninHistogramName; On 2017/06/19 14:38:12, jlebel wrote: ...
3 years, 6 months ago (2017-06-22 12:40:39 UTC) #14
jlebel
Thanks https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2942923002/diff/120001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h#newcode46 ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:46: @property(nonatomic) std::string countTilSigninHistogramName; On 2017/06/22 12:40:38, sdefresne wrote: ...
3 years, 6 months ago (2017-06-23 01:24:06 UTC) #15
jlebel
Hello Alexei, Can you review my patch to add histograms? Thanks,
3 years, 6 months ago (2017-06-23 01:26:27 UTC) #17
jlebel
Sorry Alexei, I didn't "very slow". I'm going to send my patch to someone else.
3 years, 6 months ago (2017-06-23 01:28:59 UTC) #18
jlebel
Hello Robert, Can you review my patch to add new histograms to Chrome iOS? Thanks,
3 years, 6 months ago (2017-06-23 01:30:22 UTC) #20
rkaplow
sorry for us being slow! https://codereview.chromium.org/2942923002/diff/180001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm (right): https://codereview.chromium.org/2942923002/diff/180001/ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm#newcode157 ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:157: UMA_HISTOGRAM_COUNTS_100(_dismissalCountHistogramName, displayedCount); people generally ...
3 years, 6 months ago (2017-06-23 19:33:50 UTC) #21
jlebel
Thanks for your help. PS: I don't call that slow :) Thanks for your reactivity. ...
3 years, 6 months ago (2017-06-23 23:42:36 UTC) #23
rkaplow
lgtm mostly LG - still some suggestions related to the name of the histograms. LMK ...
3 years, 5 months ago (2017-06-27 21:13:57 UTC) #55
jlebel
Thanks for your inputs. https://codereview.chromium.org/2942923002/diff/320001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2942923002/diff/320001/tools/metrics/histograms/histograms.xml#newcode32493 tools/metrics/histograms/histograms.xml:32493: +<histogram name="MobileSignInPromo.BookmarkManager.CountTilSignin" On 2017/06/27 21:13:57, ...
3 years, 5 months ago (2017-06-28 11:10:31 UTC) #57
jlebel
https://codereview.chromium.org/2942923002/diff/320001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2942923002/diff/320001/tools/metrics/histograms/histograms.xml#newcode32493 tools/metrics/histograms/histograms.xml:32493: +<histogram name="MobileSignInPromo.BookmarkManager.CountTilSignin" On 2017/06/28 11:10:31, jlebel wrote: > On ...
3 years, 5 months ago (2017-06-28 11:37:52 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2942923002/380001
3 years, 5 months ago (2017-06-28 15:21:29 UTC) #68
commit-bot: I haz the power
3 years, 5 months ago (2017-06-28 15:27:53 UTC) #72
Message was sent while issue was closed.
Committed patchset #8 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/a24738a4699277f620a0ed12a183...

Powered by Google App Engine
This is Rietveld 408576698