Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2643723004:
Add Desktop iOS promotion logging to chrome ios app. (Closed)
DescriptionAdd Desktop iOS promotion logging to chrome ios app.
Create a sync observer for the desktop promotion and add it
on the application main controller.
When the priority prefs are synced. log to UMA if the user have seen the
desktop promotion on the last 7 days and log if they triggered the sms feature
on the promotion.
Also mark the promotion as completed using priority prefs.
BUG=681885
Review-Url: https://codereview.chromium.org/2643723004
Cr-Commit-Position: refs/heads/master@{#447026}
Committed: https://chromium.googlesource.com/chromium/src/+/54a832541cb8aafced2f60deb296132c90ad1eb4
Patch Set 1 : CL #
Total comments: 8
Patch Set 2 : address comments / change pref name #
Total comments: 16
Patch Set 3 : add histogram #
Total comments: 6
Patch Set 4 : adding histogram changes and addressing comments #
Total comments: 13
Patch Set 5 : Add histogram_macros to desktop_promotion/ #
Total comments: 6
Patch Set 6 : browser_state keyed service #
Total comments: 21
Patch Set 7 : create new file for sync service & move prefs to pref_names.h #
Total comments: 12
Patch Set 8 : remove histogram_macros #
Total comments: 10
Patch Set 9 : histogram summary #
Total comments: 10
Patch Set 10 : change histogram name #
Total comments: 19
Patch Set 11 : address nits #
Total comments: 2
Patch Set 12 : to be submitted #
Total comments: 1
Messages
Total messages: 52 (22 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add Desktop iOS promotion logging to chrome ios app. CL is not final yet. BUG= ========== to ========== Add Desktop iOS promotion logging to chrome ios app. Create a sync observer for the desktop promotion and add it on the application main controller. When the priority prefs are synced. log to UMA if the user have seen the desktop promotion on the last 7 days and log if they triggered the sms feature on the promotion. Also mark the promotion as completed using priority prefs. BUG=681885 ==========
mrefaat@chromium.org changed reviewers: + rohitrao@chromium.org
pkl@chromium.org changed reviewers: + pkl@chromium.org
drive-by https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:20: const int kMaxEntrypoints = 5; What is the maximum number of entry points? I'm confused by this. https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:48: sync_service->GetActiveDataTypes().Has(syncer::PRIORITY_PREFERENCES)) { If a user (cold) launches Chrome multiple times during the first 7 days, wouldn't this cause the histograms to be logged multiple times? https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:62: for (int i = 1; i < kMaxEntrypoints; i++) { It would be helpful to have a comment explaining why this loop doesn't start with i = 0. https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:68: "DesktopIOSPromotion.%s.CompletionTime", This probably doesn't work. See base/metrics/histogram_macros.h which states: "...must be called with |name| as a runtime constant..."
mrefaat@google.com changed reviewers: + mrefaat@google.com
https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:20: const int kMaxEntrypoints = 5; On 2017/01/18 23:47:02, pklpkl wrote: > What is the maximum number of entry points? > I'm confused by this. Required by uma when logging count/enum . https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:48: sync_service->GetActiveDataTypes().Has(syncer::PRIORITY_PREFERENCES)) { On 2017/01/18 23:47:02, pklpkl wrote: > If a user (cold) launches Chrome multiple times during the first 7 days, > wouldn't this cause the histograms to be logged multiple times? thanks for the case, i should check for the completion priority pref too. https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:62: for (int i = 1; i < kMaxEntrypoints; i++) { On 2017/01/18 23:47:02, pklpkl wrote: > It would be helpful to have a comment explaining why this loop doesn't start > with i = 0. Acknowledged. https://codereview.chromium.org/2643723004/diff/40001/ios/chrome/browser/desk... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:68: "DesktopIOSPromotion.%s.CompletionTime", On 2017/01/18 23:47:02, pklpkl wrote: > This probably doesn't work. See base/metrics/histogram_macros.h which states: > "...must be called with |name| as a runtime constant..." I think runtime constant (not compile time constant) here means that we shouldn't change the string value after using it - because the uma logging macro doesn't copy the string it uses the same reference that is given to it by the call. Based on that I think it's okay to pass the string output of StringPrintf - which is std::string created inside the StringPrintf and doesn't change later.
Patchset #3 (id:80001) has been deleted
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
Happened to drive-by when reviewing some histograms descriptions for this work. Hope these comments help improve the code! :-) --mark https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:9: #include "base/metrics/histogram.h" nit: use histogram_macros.h https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:21: const int kMaxEntryPoints = 5; Can't you do arraysize(kDesktopIOSPromotionEntrypointHistogramPrefix) + 1? https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:25: const char kSMSSentIOSSigninReasonHistogram[] = nit: only used in one place; don't need a special variable for it. ditto below https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:26: "DesktopIOSPromotion.SMSSent.IOSSigninReason"; These two histograms aren't listed in histograms.xml. (DesktopIOSPromotion.IOSSigninReason is.) https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:70: base::StringPrintf( You cannot use a UMA_HISTOGRAM macro with a not-statically-defined string. Please see the comments histogram_macros.h. There is an alternate construction that should work for you. https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:76: UMA_HISTOGRAM_ENUMERATION(kNoSMSIOSSigninReasonHistogram, i, I know this histogram isn't yet documented. Yet, I find the fact that it's going to be emitted to in a loop surprising. Listing "no sign in" to multiple entry points sounds like it'll be hard to explain. I look forward to reading the histograms.xml description for this and seeing what it's intended to measure. :-)
https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10017: + entry point promotion on desktop and user signing in Chrome iOS app. Each On 2017/01/20 15:43:31, mrefaat wrote: > On 2017/01/19 23:33:45, Mark P wrote: > > Unanswered: > > On 2017/01/18 20:03:34, Mark P wrote: > > > For my education: are these times both server-measured, i.e., correct? Or > is > > > something his measured client-side and thus can be effected by > possibly-wrong > > > client-side clocks? > > > The time i'll be saving is time from epoch in UTC. This doesn't answer my question. Is this time recorded based on a server timestamp or a client one? https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10027: + promotion that was shown to the user. This will only be logged if at least If this transformation I propose correct? This will only be logged if at least one promotion was shown to this user on the last 7 days. -> This will be logged for each promotion shown to this user on the last 7 days. Otherwise you need to explain how you choose which particular entry point to log if multiple were shown.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:9: #include "base/metrics/histogram.h" On 2017/01/19 23:35:48, Mark P wrote: > nit: use histogram_macros.h Done. https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:21: const int kMaxEntryPoints = 5; On 2017/01/19 23:35:48, Mark P wrote: > Can't you do arraysize(kDesktopIOSPromotionEntrypointHistogramPrefix) + 1? Acknowledged. https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:25: const char kSMSSentIOSSigninReasonHistogram[] = On 2017/01/19 23:35:48, Mark P wrote: > nit: only used in one place; don't need a special variable for it. > ditto below Acknowledged. https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:26: "DesktopIOSPromotion.SMSSent.IOSSigninReason"; On 2017/01/19 23:35:48, Mark P wrote: > These two histograms aren't listed in histograms.xml. > (DesktopIOSPromotion.IOSSigninReason is.) they are defined by a suffix in histograms.xml https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:70: base::StringPrintf( On 2017/01/19 23:35:48, Mark P wrote: > You cannot use a UMA_HISTOGRAM macro with a not-statically-defined string. > Please see the comments histogram_macros.h. There is an alternate construction > that should work for you. From histogram_macros.h: // All of these macros must be called with |name| as a runtime constant. If i understand this sentence right i believe its okay to not use a statically defined string. https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:76: UMA_HISTOGRAM_ENUMERATION(kNoSMSIOSSigninReasonHistogram, i, On 2017/01/19 23:35:48, Mark P wrote: > I know this histogram isn't yet documented. Yet, I find the fact that it's > going to be emitted to in a loop surprising. Listing "no sign in" to multiple > entry points sounds like it'll be hard to explain. I look forward to reading > the histograms.xml description for this and seeing what it's intended to > measure. :-) Acknowledged. https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10017: + entry point promotion on desktop and user signing in Chrome iOS app. Each On 2017/01/20 17:43:40, Mark P wrote: > On 2017/01/20 15:43:31, mrefaat wrote: > > On 2017/01/19 23:33:45, Mark P wrote: > > > Unanswered: > > > On 2017/01/18 20:03:34, Mark P wrote: > > > > For my education: are these times both server-measured, i.e., correct? Or > > is > > > > something his measured client-side and thus can be effected by > > possibly-wrong > > > > client-side clocks? > > > > > The time i'll be saving is time from epoch in UTC. > > This doesn't answer my question. Is this time recorded based on a server > timestamp or a client one? client one. https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10027: + promotion that was shown to the user. This will only be logged if at least On 2017/01/20 17:43:40, Mark P wrote: > If this transformation I propose correct? > This will only be logged if at least one promotion was shown to this user on the > last 7 days. > -> > This will be logged for each promotion shown to this user on the last 7 days. > > Otherwise you need to explain how you choose which particular entry point to log > if multiple were shown. i should have marked this as not ready for review sorry - i'm still working on it. But i'll be logging all the entry points users saw if at least one was shown on the last 7 days.
Patchset #2 (id:60001) has been deleted
Patchset #4 (id:180001) has been deleted
https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:26: "DesktopIOSPromotion.SMSSent.IOSSigninReason"; On 2017/01/20 19:14:59, mrefaat wrote: > On 2017/01/19 23:35:48, Mark P wrote: > > These two histograms aren't listed in histograms.xml. > > (DesktopIOSPromotion.IOSSigninReason is.) > they are defined by a suffix in histograms.xml Ah, I forgot about histogram_suffixes supporting putting stuff in the middle. Thanks. https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:70: base::StringPrintf( On 2017/01/20 19:14:59, mrefaat wrote: > On 2017/01/19 23:35:48, Mark P wrote: > > You cannot use a UMA_HISTOGRAM macro with a not-statically-defined string. > > Please see the comments histogram_macros.h. There is an alternate > construction > > that should work for you. > From histogram_macros.h: // All of these macros must be called with |name| as a > runtime constant. > If i understand this sentence right i believe its okay to not use a statically > defined string. You cannot use a StringPrintf here. From histogram_macros.h: "it must be the same string on all calls from a particular call site." At this call site, you are having multiple different strings at the same call site. You need to use FactoryGet, not a macro here. https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10017: + entry point promotion on desktop and user signing in Chrome iOS app. Each On 2017/01/20 19:14:59, mrefaat wrote: > On 2017/01/20 17:43:40, Mark P wrote: > > On 2017/01/20 15:43:31, mrefaat wrote: > > > On 2017/01/19 23:33:45, Mark P wrote: > > > > Unanswered: > > > > On 2017/01/18 20:03:34, Mark P wrote: > > > > > For my education: are these times both server-measured, i.e., correct? > Or > > > is > > > > > something his measured client-side and thus can be effected by > > > possibly-wrong > > > > > client-side clocks? > > > > > > > The time i'll be saving is time from epoch in UTC. > > > > This doesn't answer my question. Is this time recorded based on a server > > timestamp or a client one? > > client one. And what about the time for user initiated the SMS action on desktop. Is that a client one too? The description here should warn that both of these are client times (or one is a client time and one is a server time), and in any case on different machines so if one or either clock is wrong, then the results will be wrong. Depending on the accuracy you need, you may want server times. IIRC, 1% of desktop users have their clock set wrong by more than a day.
https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/app/main_co... File ios/chrome/app/main_controller.mm (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/app/main_co... ios/chrome/app/main_controller.mm:340: std::unique_ptr<DesktopPromotionSyncObserver> _desktopPromotionSyncObserver; Can this be a BrowserStateKeyedService instead of needing to live in MainController? It looks like this object's lifetime is scoped to a BrowserState already. The tricky part will be figuring out where in the code to create it, we might need Sylvain's help with that. https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/app/main_co... ios/chrome/app/main_controller.mm:1069: [self currentBrowserState])); Are you intentionally calling |currentBrowserState| here? What happens if the app starts up in incognito mode, or if the user switches to incognito before this block runs? Which BrowserState do want to use in those cases? https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h:13: extern const char kDesktopIOSPromotionSMSEntryPoint[]; Could these live with the other prefs in ios/chrome/browser/pref_names.h? https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h:19: namespace desktop_ios_promotion { I would get rid of the namespace here, because you already have "DesktopPromotion" in the method name. https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.mm (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.mm:12: // Which desktop ios promotion initiated sent the sms. Typo: "initiated sent" https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h:17: explicit DesktopPromotionSyncObserver(ios::ChromeBrowserState* browser_state); What's the relationship between DesktopPromotionSyncObserver and BrowserState? Is it 1:1? This constructor implies that it is 1:1, but other code makes it seem like there is no observer for the corresponding incognito BrowserState. Is there only ever one DesktopPromotionSyncObserver per app? (Another way to think about this -- if we added multi-profile support to iOS, so that two users could run chrome with different BrowserStates and different profile directories, would we have two DesktopPromotionSyncObservers or still only one?) https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/pre... File ios/chrome/browser/prefs/browser_prefs.mm (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/pre... ios/chrome/browser/prefs/browser_prefs.mm:113: RegisterVoiceSearchBrowserStatePrefs(registry); This is an example of the naming scheme I was suggesting.
mrefaat@chromium.org changed reviewers: + sdefresne@chromium.org
For moving out of the main controller - i'm still checking how can i do that. and i asked sdefrense@ to take alook https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:26: "DesktopIOSPromotion.SMSSent.IOSSigninReason"; On 2017/01/20 19:40:05, Mark P wrote: > On 2017/01/20 19:14:59, mrefaat wrote: > > On 2017/01/19 23:35:48, Mark P wrote: > > > These two histograms aren't listed in histograms.xml. > > > (DesktopIOSPromotion.IOSSigninReason is.) > > they are defined by a suffix in histograms.xml > > Ah, I forgot about histogram_suffixes supporting putting stuff in the middle. > Thanks. Acknowledged. https://codereview.chromium.org/2643723004/diff/100001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:70: base::StringPrintf( On 2017/01/20 19:40:05, Mark P wrote: > On 2017/01/20 19:14:59, mrefaat wrote: > > On 2017/01/19 23:35:48, Mark P wrote: > > > You cannot use a UMA_HISTOGRAM macro with a not-statically-defined string. > > > Please see the comments histogram_macros.h. There is an alternate > > construction > > > that should work for you. > > From histogram_macros.h: // All of these macros must be called with |name| as > a > > runtime constant. > > If i understand this sentence right i believe its okay to not use a statically > > defined string. > > You cannot use a StringPrintf here. From histogram_macros.h: > "it must be the same string on all calls from a particular call site." At this > call site, you are having multiple different strings at the same call site. > > You need to use FactoryGet, not a macro here. Thanks for the advice, will update that https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10017: + entry point promotion on desktop and user signing in Chrome iOS app. Each On 2017/01/20 19:40:06, Mark P wrote: > On 2017/01/20 19:14:59, mrefaat wrote: > > On 2017/01/20 17:43:40, Mark P wrote: > > > On 2017/01/20 15:43:31, mrefaat wrote: > > > > On 2017/01/19 23:33:45, Mark P wrote: > > > > > Unanswered: > > > > > On 2017/01/18 20:03:34, Mark P wrote: > > > > > > For my education: are these times both server-measured, i.e., correct? > > > Or > > > > is > > > > > > something his measured client-side and thus can be effected by > > > > possibly-wrong > > > > > > client-side clocks? > > > > > > > > > The time i'll be saving is time from epoch in UTC. > > > > > > This doesn't answer my question. Is this time recorded based on a server > > > timestamp or a client one? > > > > client one. > > And what about the time for user initiated the SMS action on desktop. Is that a > client one too? > > The description here should warn that both of these are client times (or one is > a client time and one is a server time), and in any case on different machines > so if one or either clock is wrong, then the results will be wrong. > > Depending on the accuracy you need, you may want server times. IIRC, 1% of > desktop users have their clock set wrong by more than a day. Both will be client time, we are targeting desktop and mobile of same user so there is a high chance that both devices are on the time zone and there will some accounted probability to have errors https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/app/main_co... File ios/chrome/app/main_controller.mm (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/app/main_co... ios/chrome/app/main_controller.mm:1069: [self currentBrowserState])); On 2017/01/23 20:31:39, rohitrao wrote: > Are you intentionally calling |currentBrowserState| here? What happens if the > app starts up in incognito mode, or if the user switches to incognito before > this block runs? Which BrowserState do want to use in those cases? changed to use _mainBrowserState - we don't care about incognito https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h:13: extern const char kDesktopIOSPromotionSMSEntryPoint[]; On 2017/01/23 20:31:39, rohitrao wrote: > Could these live with the other prefs in ios/chrome/browser/pref_names.h? Is there a benefit of doing that, many of the feature directories have their own pref files. specially that all my prefs are profile state priority prefs not local prefs. https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h:19: namespace desktop_ios_promotion { On 2017/01/23 20:31:39, rohitrao wrote: > I would get rid of the namespace here, because you already have > "DesktopPromotion" in the method name. Acknowledged. https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.mm (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.mm:12: // Which desktop ios promotion initiated sent the sms. On 2017/01/23 20:31:39, rohitrao wrote: > Typo: "initiated sent" Acknowledged. https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h:17: explicit DesktopPromotionSyncObserver(ios::ChromeBrowserState* browser_state); On 2017/01/23 20:31:39, rohitrao wrote: > What's the relationship between DesktopPromotionSyncObserver and BrowserState? > Is it 1:1? This constructor implies that it is 1:1, but other code makes it > seem like there is no observer for the corresponding incognito BrowserState. Is > there only ever one DesktopPromotionSyncObserver per app? > > (Another way to think about this -- if we added multi-profile support to iOS, so > that two users could run chrome with different BrowserStates and different > profile directories, would we have two DesktopPromotionSyncObservers or still > only one?) i think it's 1:1 if we ever have more than 1 profile we will need observer for each one. as the sync state is profile related. https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/pre... File ios/chrome/browser/prefs/browser_prefs.mm (right): https://codereview.chromium.org/2643723004/diff/200001/ios/chrome/browser/pre... ios/chrome/browser/prefs/browser_prefs.mm:113: RegisterVoiceSearchBrowserStatePrefs(registry); On 2017/01/23 20:31:39, rohitrao wrote: > This is an example of the naming scheme I was suggesting. Acknowledged.
Patchset #5 (id:220001) has been deleted
This is just the verbose version of what I said on hangout when you asked the same question. I hope this helps. https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h (right): https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h:15: class DesktopPromotionSyncObserver : public syncer::SyncServiceObserver { If you want this observer to be created with the ChromeBrowserState, then you can turn it into a KeyedService. Or maybe better have a factory that register a private object owning the DesktopPromotionSyncObserver (to avoid multiple inheritance). You'll also have to include a call to DesktopPromotionServiceFactory::GetInstance() in EnsureBrowserStateKeyedServiceFactoriesBuilt() (see browser_state_keyed_service_factories.mm). Once this is done, then you can remove the instance of DesktopPromotionSyncObserver owned by the MainController. Code for the factory and service would look like the following in the files desktop_promotion_sync_service.{h,cc}: // desktop_promotion_sync_service.h namespace ios { class ChromeBrowserState; } class DesktopPromotionSyncService; class DesktopPromotionSyncServiceFactory : public BrowserStateKeyedServiceFactory { public: static DesktopPromotionSyncService* GetForBrowserState( ios::ChromeBrowserState* browser_state); static DesktopPromotionSyncServiceFactory* GetInstance(); private: friend struct base::DefaultSingletonTraits<DesktopPromotionSyncServiceFactory>; HistoryServiceFactory(); ~HistoryServiceFactory() override; // BrowserStateKeyedServiceFactory implementation. std::unique_ptr<KeyedService> BuildServiceInstanceFor( web::BrowserState* context) const override; bool ServiceIsCreatedWithBrowserState() const override; bool ServiceIsNULLWhileTesting() const override; // If you want the observer to be created for off-the-record browser states, // then you also need to override GetBrowserStateToUse() to return a non-null // browser state (either the off-the-record browser state or the regular one). DISALLOW_COPY_AND_ASSIGN(DesktopPromotionSyncServiceFactory); }; // desktop_promotion_sync_service.cc class DesktopPromotionSyncService : public KeyedService { public: DesktopPromotionSyncService( PrefService* pref_service, browser_sync::ProfileSyncService* sync_service); ~DekstopPromotionSyncService(); private: DesktopPromotionSyncObserver observer_; } DesktopPromotionSyncService::DesktopPromotionSyncService( PrefService* pref_service, browser_sync::ProfileSyncService* sync_service) : observer_(pref_service, sync_service) {} DesktopPromotionSyncService::~DesktopPromotionSyncService() = default; // static DesktopPromotionSyncService* DesktopPromotionSyncServiceFactory::GetForBrowserState( ios::ChromeBrowserState* browser_state) { return static_cast<DesktopPromotionSyncService*>( GetInstance()->GetServiceForBrowserState(browser_state, true)); } // static DesktopPromotionSyncServiceFactory* DesktopPromotionSyncServiceFactory::GetInstance() { return base::Singleton<DesktopPromotionSyncServiceFactory>::get(); } DesktopPromotionSyncServiceFactory::DesktopPromotionSyncServiceFactory() : BrowserStateKeyedServiceFactory( "DesktopPromotionSyncService", BrowserStateDependencyManager::GetInstance()) { DependsOn(IOSChromeProfileSyncServiceFactory::GetInstance()); } DesktopPromotionSyncServiceFactory::~DesktopPromotionSyncServiceFactory() = default; std::unique_ptr<KeyedService> DesktopPromotionSyncServiceFactory::BuildServiceInstanceFor( web::BrowserState* context) const { ios::ChromeBrowserState* browser_state = ios::ChromeBrowserState::FromBrowserState(context); return base::MakeUnique<DesktopPromotionSyncService>( browser_state->GetPrefs(), IOSChromeProfileSyncServiceFactory::GetForBrowserState(browser_state)); } // By returning true here, we for the keyed service system to create this object when the // chrome browser states are created. bool DesktopPromotionSyncServiceFactory::ServiceIsCreatedWithBrowserState() const { return true; } // I think that not creating the observer for testing browser state will make thing much easier // so I would recommend returning true here. bool DesktopPromotionSyncServiceFactory::ServiceIsNULLWhileTesting() const { return true; } https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:29: browser_sync::ProfileSyncService* sync_service = Unless there is no other way around passing the service depended on in the constructor is better than passing a browser_state and then fetching the service via the factory. For one thing it make the code much easier to test as there is no need to create a test chrome browser state, set up a fake factory to inject a fake/mock implementation on the depended upon service. Another reason is that it easier to reason about the code by looking at the header. If the object take a browser state, it has access to a lot of thing. If it only takes a profile sync service, then I know it access much less things. You'll probably also have to pass the pref service (but for the same reason, it is better to do this). The last reason is that when/if turning this class into a KeyedService, the dependencies need to be explicit in the factory, and passing the service in the constructor is a good reminder to the reviewer to ask for the proper dependency declaration in the factory. So I would recommend changing this constructor to: DesktopPromotionSyncObserver::DesktopPromotionSyncObserver( PrefService* pref_service, browser_sync::ProfileSyncService* sync_service) : pref_service_(pref_service), sync_service_(sync_service) { DCHECK(pref_service_); DCHECK(sync_service_); sync_service_->AddObserver(this); }
https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:43: if (!desktop_metrics_logger_initiated_ && Unrelated to KeyedService, I think you should swap your conditions and return early as it makes the code easier to read IMO: void DesltopPromotionSyncObserver::OnStateChanged() { if (desktop_metrics_logger_initiated_ || !sync_service_->GetActiveDataTypes().Has(syncer::PRIORITY_PREFERENCES)) { return; } desktop_metrics_logger_initiated_ = true; bool done_logging = pref_service_->GetBoolean(prefs::kDesktopIOSPromotionDone); double last_impression = pref_service_->GetDouble(prefs::kDesktopIOSPromotionLastImpression); base::TimeDelta delta = base::Time::Now() - base::Time::FromDoubleT(last_impression); if (done_logging || delta.InDays() >= 7) return; // This user have seen the promotion in the last 7 days so it may be a // reason of the installation. int sms_entrypoint = pref_service_->GetInteger(prefs::kDesktopIOSPromotionSMSEntryPoint); ...
Patchset #6 (id:260001) has been deleted
https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h (right): https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h:15: class DesktopPromotionSyncObserver : public syncer::SyncServiceObserver { On 2017/01/25 09:52:52, sdefresne wrote: > If you want this observer to be created with the ChromeBrowserState, then you > can turn it into a KeyedService. Or maybe better have a factory that register a > private object owning the DesktopPromotionSyncObserver (to avoid multiple > inheritance). > > You'll also have to include a call to > DesktopPromotionServiceFactory::GetInstance() in > EnsureBrowserStateKeyedServiceFactoriesBuilt() (see > browser_state_keyed_service_factories.mm). Once this is done, then you can > remove the instance of DesktopPromotionSyncObserver owned by the MainController. > > Code for the factory and service would look like the following in the files > desktop_promotion_sync_service.{h,cc}: > > // desktop_promotion_sync_service.h > > namespace ios { > class ChromeBrowserState; > } > > class DesktopPromotionSyncService; > class DesktopPromotionSyncServiceFactory : public > BrowserStateKeyedServiceFactory { > public: > static DesktopPromotionSyncService* GetForBrowserState( > ios::ChromeBrowserState* browser_state); > static DesktopPromotionSyncServiceFactory* GetInstance(); > > private: > friend struct > base::DefaultSingletonTraits<DesktopPromotionSyncServiceFactory>; > > HistoryServiceFactory(); > ~HistoryServiceFactory() override; > > // BrowserStateKeyedServiceFactory implementation. > std::unique_ptr<KeyedService> BuildServiceInstanceFor( > web::BrowserState* context) const override; > bool ServiceIsCreatedWithBrowserState() const override; > bool ServiceIsNULLWhileTesting() const override; > > // If you want the observer to be created for off-the-record browser states, > // then you also need to override GetBrowserStateToUse() to return a non-null > // browser state (either the off-the-record browser state or the regular one). > > DISALLOW_COPY_AND_ASSIGN(DesktopPromotionSyncServiceFactory); > }; > > // desktop_promotion_sync_service.cc > > class DesktopPromotionSyncService : public KeyedService { > public: > DesktopPromotionSyncService( > PrefService* pref_service, > browser_sync::ProfileSyncService* sync_service); > ~DekstopPromotionSyncService(); > > private: > DesktopPromotionSyncObserver observer_; > } > > DesktopPromotionSyncService::DesktopPromotionSyncService( > PrefService* pref_service, > browser_sync::ProfileSyncService* sync_service) > : observer_(pref_service, sync_service) {} > > DesktopPromotionSyncService::~DesktopPromotionSyncService() = default; > > // static > DesktopPromotionSyncService* > DesktopPromotionSyncServiceFactory::GetForBrowserState( > ios::ChromeBrowserState* browser_state) { > return static_cast<DesktopPromotionSyncService*>( > GetInstance()->GetServiceForBrowserState(browser_state, true)); > } > > // static > DesktopPromotionSyncServiceFactory* > DesktopPromotionSyncServiceFactory::GetInstance() { > return base::Singleton<DesktopPromotionSyncServiceFactory>::get(); > } > > DesktopPromotionSyncServiceFactory::DesktopPromotionSyncServiceFactory() > : BrowserStateKeyedServiceFactory( > "DesktopPromotionSyncService", > BrowserStateDependencyManager::GetInstance()) { > DependsOn(IOSChromeProfileSyncServiceFactory::GetInstance()); > } > > DesktopPromotionSyncServiceFactory::~DesktopPromotionSyncServiceFactory() = > default; > > std::unique_ptr<KeyedService> > DesktopPromotionSyncServiceFactory::BuildServiceInstanceFor( > web::BrowserState* context) const { > ios::ChromeBrowserState* browser_state = > ios::ChromeBrowserState::FromBrowserState(context); > return base::MakeUnique<DesktopPromotionSyncService>( > browser_state->GetPrefs(), > IOSChromeProfileSyncServiceFactory::GetForBrowserState(browser_state)); > } > > // By returning true here, we for the keyed service system to create this object > when the > // chrome browser states are created. > bool DesktopPromotionSyncServiceFactory::ServiceIsCreatedWithBrowserState() > const { > return true; > } > > // I think that not creating the observer for testing browser state will make > thing much easier > // so I would recommend returning true here. > bool DesktopPromotionSyncServiceFactory::ServiceIsNULLWhileTesting() const { > return true; > } > Thanks for the advice, as per our discussion on hangout this service need to wait for the browserstate so i moved the initialization to browser_state_manager_impl. https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:29: browser_sync::ProfileSyncService* sync_service = On 2017/01/25 09:52:52, sdefresne wrote: > Unless there is no other way around passing the service depended on in the > constructor is better than passing a browser_state and then fetching the service > via the factory. For one thing it make the code much easier to test as there is > no need to create a test chrome browser state, set up a fake factory to inject a > fake/mock implementation on the depended upon service. > > Another reason is that it easier to reason about the code by looking at the > header. If the object take a browser state, it has access to a lot of thing. If > it only takes a profile sync service, then I know it access much less things. > You'll probably also have to pass the pref service (but for the same reason, it > is better to do this). > > The last reason is that when/if turning this class into a KeyedService, the > dependencies need to be explicit in the factory, and passing the service in the > constructor is a good reminder to the reviewer to ask for the proper dependency > declaration in the factory. > > So I would recommend changing this constructor to: > > DesktopPromotionSyncObserver::DesktopPromotionSyncObserver( > PrefService* pref_service, > browser_sync::ProfileSyncService* sync_service) > : pref_service_(pref_service), sync_service_(sync_service) { > DCHECK(pref_service_); > DCHECK(sync_service_); > sync_service_->AddObserver(this); > } Acknowledged. https://codereview.chromium.org/2643723004/diff/240001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:43: if (!desktop_metrics_logger_initiated_ && On 2017/01/25 09:58:22, sdefresne wrote: > Unrelated to KeyedService, I think you should swap your conditions and return > early as it makes the code easier to read IMO: > > void DesltopPromotionSyncObserver::OnStateChanged() { > if (desktop_metrics_logger_initiated_ || > !sync_service_->GetActiveDataTypes().Has(syncer::PRIORITY_PREFERENCES)) { > return; > } > > desktop_metrics_logger_initiated_ = true; > bool done_logging = > pref_service_->GetBoolean(prefs::kDesktopIOSPromotionDone); > double last_impression = > pref_service_->GetDouble(prefs::kDesktopIOSPromotionLastImpression); > base::TimeDelta delta = > base::Time::Now() - base::Time::FromDoubleT(last_impression); > if (done_logging || delta.InDays() >= 7) > return; > > // This user have seen the promotion in the last 7 days so it may be a > // reason of the installation. > int sms_entrypoint = > pref_service_->GetInteger(prefs::kDesktopIOSPromotionSMSEntryPoint); > ... Acknowledged.
lgtm https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h:12: namespace prefs { Can you move those prefs to ios/chrome/browser/pref_names.h? All prefs for ios/chrome should be defined there IIUC. https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h:21: ~DesktopPromotionSyncObserver() override; nit: can you put a blank line before the next line? https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:21: "HistoryPage"}; Can you put a comma after "HistoryPage" so that clang-format do not format this code weirdly? https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service_factory.h (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service_factory.h:18: class DesktopPromotionSyncServiceFactory nit: document (you can copy from another service factory)
https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:61: // Entry points are represented on the preference by integers [1..4]. Where is the meaning of these entry points defined? It needs the standard warning. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:68: UMA_HISTOGRAM_COUNT_1000_NO_CACHE( Please don't use histogram macros from chromecast/ Use FactoryGet yourself; that's typically how chromium developers do it. https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:72: delta.InHours()); Consider explicitly emitting something (0?) if this time is negative, which is possible given how it's calculated. https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9933: + promotion or not. This last part of the description doesn't sound right. It looks to me like you only emit one SMSSent entry, regardless of whether the user sent SMS from multiple promotions. if (sms_entrypoint == i) { ... } https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25946: + Deprecated as of 01/18/2017 in issue 670150. Replaced by It's odd for this to show up in the diff. Did a conflict get resolved incorrectly? https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:110489: + Did you intentionally remove the histograms DesktopIOSPromotion.UserAction and DesktopIOSPromotion.ImpressionFromEntryPoint from this file? They were in your other changelist. (Are they no longer planned to be omitted somewhere?)
Patchset #7 (id:300001) has been deleted
https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_prefs.h:12: namespace prefs { On 2017/01/26 17:03:25, sdefresne wrote: > Can you move those prefs to ios/chrome/browser/pref_names.h? All prefs for > ios/chrome should be defined there IIUC. Done. https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h:21: ~DesktopPromotionSyncObserver() override; On 2017/01/26 17:03:25, sdefresne wrote: > nit: can you put a blank line before the next line? Done. https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:21: "HistoryPage"}; On 2017/01/26 17:03:25, sdefresne wrote: > Can you put a comma after "HistoryPage" so that clang-format do not format this > code weirdly? Done. https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:61: // Entry points are represented on the preference by integers [1..4]. On 2017/01/26 18:55:15, Mark P wrote: > Where is the meaning of these entry points defined? It needs the standard > warning. > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... The meaning will be set on the desktop side code, and will add the warning there. i've put a todo for now https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:68: UMA_HISTOGRAM_COUNT_1000_NO_CACHE( On 2017/01/26 18:55:15, Mark P wrote: > Please don't use histogram macros from chromecast/ > > Use FactoryGet yourself; that's typically how chromium developers do it. These are not chromecast macros' - i defined another file ios/chrome/browser/desktop_promotion/histogram_macros.h where i use the FactoryGet https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:72: delta.InHours()); On 2017/01/26 18:55:15, Mark P wrote: > Consider explicitly emitting something (0?) if this time is negative, which is > possible given how it's calculated. Acknowledged. https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service_factory.h (right): https://codereview.chromium.org/2643723004/diff/280001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service_factory.h:18: class DesktopPromotionSyncServiceFactory On 2017/01/26 17:03:25, sdefresne wrote: > nit: document (you can copy from another service factory) Acknowledged. https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9933: + promotion or not. On 2017/01/26 18:55:15, Mark P wrote: > This last part of the description doesn't sound right. It looks to me like you > only emit one SMSSent entry, regardless of whether the user sent SMS from > multiple promotions. > > if (sms_entrypoint == i) { > ... > } I'll try to explain more. https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25946: + Deprecated as of 01/18/2017 in issue 670150. Replaced by On 2017/01/26 18:55:15, Mark P wrote: > It's odd for this to show up in the diff. > Did a conflict get resolved incorrectly? probably i resolve conflicts every time i pull :S https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:110489: + On 2017/01/26 18:55:15, Mark P wrote: > Did you intentionally remove the histograms DesktopIOSPromotion.UserAction and > DesktopIOSPromotion.ImpressionFromEntryPoint from this file? They were in your > other changelist. (Are they no longer planned to be omitted somewhere?) i removed them as i'm not using them on this cl, they will be used on a different cl, so i'll be adding them later.
https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:110489: + On 2017/01/26 23:34:12, mrefaat wrote: > On 2017/01/26 18:55:15, Mark P wrote: > > Did you intentionally remove the histograms DesktopIOSPromotion.UserAction and > > DesktopIOSPromotion.ImpressionFromEntryPoint from this file? They were in > your > > other changelist. (Are they no longer planned to be omitted somewhere?) > > i removed them as i'm not using them on this cl, they will be used on a > different cl, so i'll be adding them later. Acknowledged. https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:19: const char* kDesktopIOSPromotionEntrypointHistogramPrefix[] = { Please put a warning here, as discussed. https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:63: // TODO(681885): Add reference to the Entry point Constants defined in the fwiw, I don't think I've ever see TODO with bug numbers in them. Is this standard practice in the iOS code? https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:71: UMA_HISTOGRAM_COUNT_1000_NO_CACHE( On 2017/01/26 23:34:12, mrefaat wrote: > On 2017/01/26 18:55:15, Mark P wrote: > > Please don't use histogram macros from chromecast/ > > > > Use FactoryGet yourself; that's typically how chromium developers do it. > > These are not chromecast macros' - i defined another file > ios/chrome/browser/desktop_promotion/histogram_macros.h where i use the > FactoryGet A new header file to define a series of three macros for something used in only one place seems odd to me. Why can't you put the actual code here? Are you planning to reuse the macros elsewhere? https://codereview.chromium.org/2643723004/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10067: + name that was responsible for the SMS sending. Please say something about negative times (due to bad user clocks) being emitted as 0s. https://codereview.chromium.org/2643723004/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10081: + cycle they will not see any other desktop to iOS promotion again. On 2017/01/26 23:34:12, mrefaat wrote: > On 2017/01/26 18:55:15, Mark P wrote: > > This last part of the description doesn't sound right. It looks to me like > you > > only emit one SMSSent entry, regardless of whether the user sent SMS from > > multiple promotions. > > > > if (sms_entrypoint == i) { > > ... > > } > I'll try to explain more. This last part doesn't make sense to me. It says that once the user has received an sms and completed a cycle, that's what stops desktop promotions. Yet I can easily imagine sending an SMS and not doing anything on the iOS device (not even turning it on), then seeing another desktop promotion, sending another SMS. The last part of the sentence implies this is possible, which contradicts the first part of the sentence saying at most only one SMS would be sent. What am I missing?
https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:75: MAX(0, delta.InHours())); BTW, where are you getting MAX from? I normally see code that calls std::max(). I wonder what this one is doing.
https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:19: const char* kDesktopIOSPromotionEntrypointHistogramPrefix[] = { On 2017/01/26 23:47:24, Mark P wrote: > Please put a warning here, as discussed. Done. https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:63: // TODO(681885): Add reference to the Entry point Constants defined in the On 2017/01/26 23:47:24, Mark P wrote: > fwiw, I don't think I've ever see TODO with bug numbers in them. Is this > standard practice in the iOS code? i should have done it crbut.com/bug number i was told about this practice in comments when i was contributing to the desktop code. https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:71: UMA_HISTOGRAM_COUNT_1000_NO_CACHE( On 2017/01/26 23:47:24, Mark P wrote: > On 2017/01/26 23:34:12, mrefaat wrote: > > On 2017/01/26 18:55:15, Mark P wrote: > > > Please don't use histogram macros from chromecast/ > > > > > > Use FactoryGet yourself; that's typically how chromium developers do it. > > > > These are not chromecast macros' - i defined another file > > ios/chrome/browser/desktop_promotion/histogram_macros.h where i use the > > FactoryGet > > A new header file to define a series of three macros for something used in only > one place seems odd to me. Why can't you put the actual code here? Are you > planning to reuse the macros elsewhere? removed it. https://codereview.chromium.org/2643723004/diff/320001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:75: MAX(0, delta.InHours())); On 2017/01/27 17:35:01, Mark P wrote: > BTW, where are you getting MAX from? I normally see code that calls std::max(). > I wonder what this one is doing. replaced by std:max - MAX is predifned in Foundation library but i think its better to use std:max https://codereview.chromium.org/2643723004/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10067: + name that was responsible for the SMS sending. On 2017/01/26 23:47:24, Mark P wrote: > Please say something about negative times (due to bad user clocks) being emitted > as 0s. Done. https://codereview.chromium.org/2643723004/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10081: + cycle they will not see any other desktop to iOS promotion again. On 2017/01/26 23:47:24, Mark P wrote: > On 2017/01/26 23:34:12, mrefaat wrote: > > On 2017/01/26 18:55:15, Mark P wrote: > > > This last part of the description doesn't sound right. It looks to me like > > you > > > only emit one SMSSent entry, regardless of whether the user sent SMS from > > > multiple promotions. > > > > > > if (sms_entrypoint == i) { > > > ... > > > } > > I'll try to explain more. > > This last part doesn't make sense to me. It says that once the user has > received an sms and completed a cycle, that's what stops desktop promotions. > Yet I can easily imagine sending an SMS and not doing anything on the iOS device > (not even turning it on), then seeing another desktop promotion, sending another > SMS. The last part of the sentence implies this is possible, which contradicts > the first part of the sentence saying at most only one SMS would be sent. What > am I missing? It will be logged at most once because you have to get into the iOS client to actual log, so assuming that we target the user with the promotion and they actually clicked the send sms button and didn't open chrome on their iOS device no thing will be logged as we only log when we have the user signed in in the client, then the user is targeted by different promotion or they clear their own local prefs - they may see it again but still nothing will be logged as we only log when they open chrome on ios and signin.
https://codereview.chromium.org/2643723004/diff/340001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/340001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:66: // in the nit: formatting https://codereview.chromium.org/2643723004/diff/340001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:78: 1, 1000, 50, base::Histogram::kUmaTargetedHistogramFlag) Something seems wrong here. In the DesktopIOSPromotion.IOSSigninReason histogram description, you explain that you only log those histograms if the user signed in within 7 days of seeing the promotion. This histogram, DesktopIOSPromotion.%s.CompletionTime, is emitted in the same loop. This means it either has the same restriction (7 days), in which case using a max for the histogram of 1000 hours = 41 days is wrong, or neither histogram has the restriction, in which case the description of the other is wrong. If the 7 day restriction is right, you should say that in this histogram description too, and you should reduce the max of this histogram to something more like 168. https://codereview.chromium.org/2643723004/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10079: + Chrome iOS app, if the value is negative due to bad clock on one of the nit: , -> ; (or period and start a new sentence by capitalizing "if") https://codereview.chromium.org/2643723004/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10091: + least one promotion was shown to this user on the last 7 days IOSigninReason period after "days". You're starting a new sentence. https://codereview.chromium.org/2643723004/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10095: + cycle they will not see any other desktop to iOS promotion again. On 2017/01/27 18:29:42, mrefaat wrote: > On 2017/01/26 23:47:24, Mark P wrote: > > On 2017/01/26 23:34:12, mrefaat wrote: > > > On 2017/01/26 18:55:15, Mark P wrote: > > > > This last part of the description doesn't sound right. It looks to me > like > > > you > > > > only emit one SMSSent entry, regardless of whether the user sent SMS from > > > > multiple promotions. > > > > > > > > if (sms_entrypoint == i) { > > > > ... > > > > } > > > I'll try to explain more. > > > > This last part doesn't make sense to me. It says that once the user has > > received an sms and completed a cycle, that's what stops desktop promotions. > > Yet I can easily imagine sending an SMS and not doing anything on the iOS > device > > (not even turning it on), then seeing another desktop promotion, sending > another > > SMS. The last part of the sentence implies this is possible, which > contradicts > > the first part of the sentence saying at most only one SMS would be sent. > What > > am I missing? > > It will be logged at most once because you have to get into the iOS client to > actual log, so assuming that we target the user with the promotion and they > actually clicked the send sms button and didn't open chrome on their iOS device > no thing will be logged as we only log when we have the user signed in in the > client, then the user is targeted by different promotion or they clear their own > local prefs - they may see it again but still nothing will be logged as we only > log when they open chrome on ios and signin. It sounds like you're saying you only log the SMS sent form if the user entered Chrome due to the SMS. That's why it gets logged only once. If the user sent an SMS and launched Chrome on iOS without having viewed the SMS, then it doesn't get logged. Correct? If so, I suggest revising the last two sentence (well, two sentence after you add the period I asked for) as follows: IOSigninReason will be prefixed to indicate if the user launched Chrome and signed in due to using an SMS sent from a promotion. Hence, DesktopIOSPromotion.SMSSent.IOSSigninReason will be logged at most once. If a user sends an SMS from one or more promotions and doesn't use the SMS to help sign-in to Chrome, all of these will be recorded as NoSMS.
As per our discussion i updated the description on the histogram.xml https://codereview.chromium.org/2643723004/diff/340001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/340001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:66: // in the On 2017/01/27 18:58:59, Mark P wrote: > nit: formatting Done. https://codereview.chromium.org/2643723004/diff/340001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:78: 1, 1000, 50, base::Histogram::kUmaTargetedHistogramFlag) On 2017/01/27 18:58:59, Mark P wrote: > Something seems wrong here. > In the DesktopIOSPromotion.IOSSigninReason histogram description, you explain > that you only log those histograms if the user signed in within 7 days of seeing > the promotion. > This histogram, DesktopIOSPromotion.%s.CompletionTime, is emitted in the same > loop. > This means it either has the same restriction (7 days), in which case using a > max for the histogram of 1000 hours = 41 days is wrong, or neither histogram has > the restriction, in which case the description of the other is wrong. > > If the 7 day restriction is right, you should say that in this histogram > description too, and you should reduce the max of this histogram to something > more like 168. It's only logged if the user saw a promotion on last 7 days, i used the numbers from the macro , changing them now. https://codereview.chromium.org/2643723004/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10079: + Chrome iOS app, if the value is negative due to bad clock on one of the On 2017/01/27 18:58:59, Mark P wrote: > nit: , -> ; (or period and start a new sentence by capitalizing "if") Done. https://codereview.chromium.org/2643723004/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10091: + least one promotion was shown to this user on the last 7 days IOSigninReason On 2017/01/27 18:58:59, Mark P wrote: > period after "days". You're starting a new sentence. Done. https://codereview.chromium.org/2643723004/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10095: + cycle they will not see any other desktop to iOS promotion again. On 2017/01/27 18:58:59, Mark P wrote: > On 2017/01/27 18:29:42, mrefaat wrote: > > On 2017/01/26 23:47:24, Mark P wrote: > > > On 2017/01/26 23:34:12, mrefaat wrote: > > > > On 2017/01/26 18:55:15, Mark P wrote: > > > > > This last part of the description doesn't sound right. It looks to me > > like > > > > you > > > > > only emit one SMSSent entry, regardless of whether the user sent SMS > from > > > > > multiple promotions. > > > > > > > > > > if (sms_entrypoint == i) { > > > > > ... > > > > > } > > > > I'll try to explain more. > > > > > > This last part doesn't make sense to me. It says that once the user has > > > received an sms and completed a cycle, that's what stops desktop promotions. > > > > Yet I can easily imagine sending an SMS and not doing anything on the iOS > > device > > > (not even turning it on), then seeing another desktop promotion, sending > > another > > > SMS. The last part of the sentence implies this is possible, which > > contradicts > > > the first part of the sentence saying at most only one SMS would be sent. > > What > > > am I missing? > > > > It will be logged at most once because you have to get into the iOS client to > > actual log, so assuming that we target the user with the promotion and they > > actually clicked the send sms button and didn't open chrome on their iOS > device > > no thing will be logged as we only log when we have the user signed in in the > > client, then the user is targeted by different promotion or they clear their > own > > local prefs - they may see it again but still nothing will be logged as we > only > > log when they open chrome on ios and signin. > > It sounds like you're saying you only log the SMS sent form if the user entered > Chrome due to the SMS. That's why it gets logged only once. If the user sent > an SMS and launched Chrome on iOS without having viewed the SMS, then it doesn't > get logged. Correct? > > If so, I suggest revising the last two sentence (well, two sentence after you > add the period I asked for) as follows: > IOSigninReason will be prefixed to indicate if the user launched Chrome and > signed in due to using an SMS sent from a promotion. Hence, > DesktopIOSPromotion.SMSSent.IOSSigninReason will be logged at most once. If a > user sends an SMS from one or more promotions and doesn't use the SMS to help > sign-in to Chrome, all of these will be recorded as NoSMS. The user doesn't need to launch chrome by clicking on the SMS, actually the SMS will direct the user to the app store so they can install chrome. But if the user got SMS saw a promotion on last 7 days and signed in to chrome we say that the reason is SMSSent.
optimistically saying lgtm on the metrics; please make the changes below. --mark https://codereview.chromium.org/2643723004/diff/360001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/360001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:78: 0, 168, 24, base::Histogram::kUmaTargetedHistogramFlag) The min always have to be one or more. (Odd I know.) There is an "underflow" bucket that will catch the zeros. https://codereview.chromium.org/2643723004/diff/360001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:78: 0, 168, 24, base::Histogram::kUmaTargetedHistogramFlag) Note that these 24 buckets will be spaced exponentially, so they're going to be something like 1-2, 2-3, 3-5, 5-8, ..., 140-168 is that what you want / intend? (It seems reasonable to be, but I do see that you chose a number of buckets that exactly divides the max of your histogram, which makes me think you want equally-spaced / linear buckets.) https://codereview.chromium.org/2643723004/diff/360001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10074: +<histogram name="DesktopIOSPromotion.CompletionTime" units="hours"> Please consider putting SMS in the histogram name somewhere. Otherwise the fact that this is only logged for SMSs is easily missed. (When renaming, remember to rename places in the code that emits to this histogram too.) https://codereview.chromium.org/2643723004/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10079: + Chrome iOS app. This will only be logged if at least one promotion was shown nit: at least one promotion was shown -> at least one promotion sent an SMS https://codereview.chromium.org/2643723004/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10080: + to the user on the last 7 days. If the value is negative due to bad clock on Before the "If the value is negative" sentence, please add If multiple SMSs were sent, it uses the time of the most recently sent one.
https://codereview.chromium.org/2643723004/diff/360001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/360001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:78: 0, 168, 24, base::Histogram::kUmaTargetedHistogramFlag) On 2017/01/27 20:07:33, Mark P wrote: > Note that these 24 buckets will be spaced exponentially, so they're going to be > something like > 1-2, 2-3, 3-5, 5-8, ..., 140-168 > is that what you want / intend? > > (It seems reasonable to be, but I do see that you chose a number of buckets that > exactly divides the max of your histogram, which makes me think you want > equally-spaced / linear buckets.) i dont have actually good bucket number so i put 24 as random number not more https://codereview.chromium.org/2643723004/diff/360001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:78: 0, 168, 24, base::Histogram::kUmaTargetedHistogramFlag) On 2017/01/27 20:07:33, Mark P wrote: > The min always have to be one or more. (Odd I know.) There is an "underflow" > bucket that will catch the zeros. Acknowledged. https://codereview.chromium.org/2643723004/diff/360001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10074: +<histogram name="DesktopIOSPromotion.CompletionTime" units="hours"> On 2017/01/27 20:07:33, Mark P wrote: > Please consider putting SMS in the histogram name somewhere. Otherwise the fact > that this is only logged for SMSs is easily missed. > > (When renaming, remember to rename places in the code that emits to this > histogram too.) Acknowledged. https://codereview.chromium.org/2643723004/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10079: + Chrome iOS app. This will only be logged if at least one promotion was shown On 2017/01/27 20:07:33, Mark P wrote: > nit: > at least one promotion was shown > -> > at least one promotion sent an SMS That's not true it's if at least one promotion is shown but this will not be logged if no sms was sent so i'll change it to mention both. https://codereview.chromium.org/2643723004/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10080: + to the user on the last 7 days. If the value is negative due to bad clock on On 2017/01/27 20:07:34, Mark P wrote: > Before the "If the value is negative" sentence, please add > If multiple SMSs were sent, it uses the time of the most recently sent one. Done.
one minor comment, still histograms.xml lgtm generally. https://codereview.chromium.org/2643723004/diff/380001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/380001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10094: + to the user on the last 7 days and if at least one SMS was sent. If multiple "to the user on the last 7 days and if at least one SMS was sent" implies that it's true if at least one promotion was shown in the last seven days and one SMS was sent (at any point) Is that right? If so, please add "(at any time)" to the end of the sentence. If not, please replace "and if at least one SMS was sent" with "that sent an SMS".
lgtm with some comments https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/BUILD.gn (right): https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/BUILD.gn:23: "//ios/chrome/browser/sync/", Remove trailing "/" :-) https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:1: // Copyright 2017 The Chromium Authors. All rights reserved. Why is this file a .mm instead of a .cc? It looks like it is only using C++ code. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:18: nit: remove blank newline (or add one before closing the namespace) https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:25: } nit: add "// namespace" https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:88: pref_service_->SetBoolean(prefs::kDesktopIOSPromotionDone, true); nit: should this instance unregister as an Observer from that point as "done_logging" will be true, thus the function will just return in OnStateChanged() calls from that point? https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... File ios/chrome/browser/pref_names.cc (right): https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... ios/chrome/browser/pref_names.cc:142: // Integer which indicates which desktop ios promotion initiated sending the SMS ios -> iOS What does "desktop ios" mean? I don't really understand this comment. Would something like this work? // Index of the entry point that initiated sending the SMS to the user for the // "desktop to iOS" promotion (see corresponding histogram for details). https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... ios/chrome/browser/pref_names.cc:147: // Integer Which indicates which desktop ios promotions did the user see. ditto Maybe // Indexes of the entry points presented to the user for "desktop to iOS" promotion. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... ios/chrome/browser/pref_names.cc:151: // Last desktop ios promotion impression (If SMS was sent it's the time of ditto Maybe // Timestamp of the last "desktop to iOS" promotion SMS dispatch or of the last // impression (if no SMS was sent). https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... ios/chrome/browser/pref_names.cc:156: // True if the user see the promotion, get sms, install and sign into ditto Maybe // True if the promotion was successful, i.e. user installed the application and // signed in after being presented the promotion and receiving the SMS.
https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/BUILD.gn (right): https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/BUILD.gn:23: "//ios/chrome/browser/sync/", On 2017/01/30 15:47:19, sdefresne wrote: > Remove trailing "/" :-) Acknowledged. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm (right): https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:18: On 2017/01/30 15:47:19, sdefresne wrote: > nit: remove blank newline (or add one before closing the namespace) Done. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:25: } On 2017/01/30 15:47:19, sdefresne wrote: > nit: add "// namespace" Done. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.mm:88: pref_service_->SetBoolean(prefs::kDesktopIOSPromotionDone, true); On 2017/01/30 15:47:19, sdefresne wrote: > nit: should this instance unregister as an Observer from that point as > "done_logging" will be true, thus the function will just return in > OnStateChanged() calls from that point? Done. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... File ios/chrome/browser/pref_names.cc (right): https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... ios/chrome/browser/pref_names.cc:142: // Integer which indicates which desktop ios promotion initiated sending the SMS On 2017/01/30 15:47:19, sdefresne wrote: > ios -> iOS > > What does "desktop ios" mean? I don't really understand this comment. > Would something like this work? > > // Index of the entry point that initiated sending the SMS to the user for the > // "desktop to iOS" promotion (see corresponding histogram for details). Acknowledged. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... ios/chrome/browser/pref_names.cc:147: // Integer Which indicates which desktop ios promotions did the user see. On 2017/01/30 15:47:19, sdefresne wrote: > ditto > > Maybe > > // Indexes of the entry points presented to the user for "desktop to iOS" > promotion. Acknowledged. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... ios/chrome/browser/pref_names.cc:151: // Last desktop ios promotion impression (If SMS was sent it's the time of On 2017/01/30 15:47:19, sdefresne wrote: > ditto > > Maybe > > // Timestamp of the last "desktop to iOS" promotion SMS dispatch or of the last > // impression (if no SMS was sent). Acknowledged. https://codereview.chromium.org/2643723004/diff/380001/ios/chrome/browser/pre... ios/chrome/browser/pref_names.cc:156: // True if the user see the promotion, get sms, install and sign into On 2017/01/30 15:47:19, sdefresne wrote: > ditto > > Maybe > > // True if the promotion was successful, i.e. user installed the application and > // signed in after being presented the promotion and receiving the SMS. Acknowledged. https://codereview.chromium.org/2643723004/diff/380001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/380001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10094: + to the user on the last 7 days and if at least one SMS was sent. If multiple On 2017/01/28 05:00:53, Mark P wrote: > "to the user on the last 7 days and if at least one SMS was sent" > implies that it's true if at least one promotion was shown in the last seven > days and one SMS was sent (at any point) > > Is that right? > > If so, please add "(at any time)" to the end of the sentence. > If not, please replace "and if at least one SMS was sent" with "that sent an > SMS". That's right - added (at any time)
The CQ bit was checked by mrefaat@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...
lgtm https://codereview.chromium.org/2643723004/diff/400001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service.h (right): https://codereview.chromium.org/2643723004/diff/400001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service.h:16: // // This class is responsible for creating a DesktopPromotionSyncObserver nit: Remove one of the duplicated "//"
The CQ bit was checked by mrefaat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2643723004/#ps420001 (title: "to be submitted")
https://codereview.chromium.org/2643723004/diff/400001/ios/chrome/browser/des... File ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service.h (right): https://codereview.chromium.org/2643723004/diff/400001/ios/chrome/browser/des... ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service.h:16: // // This class is responsible for creating a DesktopPromotionSyncObserver On 2017/01/30 16:45:54, sdefresne wrote: > nit: Remove one of the duplicated "//" Acknowledged.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Not a big deal, but ... https://codereview.chromium.org/2643723004/diff/420001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2643723004/diff/420001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9935: + to the user on the last 7 days and if at least one SMS was sent. If multiple On 2017/01/30 16:32:23, mrefaat wrote: > On 2017/01/28 05:00:53, Mark P wrote: > > "to the user on the last 7 days and if at least one SMS was sent" > > implies that it's true if at least one promotion was shown in the last seven > > days and one SMS was sent (at any point) > > > > Is that right? > > > > If so, please add "(at any time)" to the end of the sentence. > > If not, please replace "and if at least one SMS was sent" with "that sent an > > SMS". > > That's right - added (at any time) I meant to add it at the end of this sentence "if at least one SMS was sent (at any time)". It doesn't really add much in the place you added it (regarding what happens if multiple SMSs are sent).
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1485797221133750,
"parent_rev": "488178789203e51c2e73761ef9a45d1e6f8a4b92", "commit_rev":
"54a832541cb8aafced2f60deb296132c90ad1eb4"}
Message was sent while issue was closed.
Description was changed from ========== Add Desktop iOS promotion logging to chrome ios app. Create a sync observer for the desktop promotion and add it on the application main controller. When the priority prefs are synced. log to UMA if the user have seen the desktop promotion on the last 7 days and log if they triggered the sms feature on the promotion. Also mark the promotion as completed using priority prefs. BUG=681885 ========== to ========== Add Desktop iOS promotion logging to chrome ios app. Create a sync observer for the desktop promotion and add it on the application main controller. When the priority prefs are synced. log to UMA if the user have seen the desktop promotion on the last 7 days and log if they triggered the sms feature on the promotion. Also mark the promotion as completed using priority prefs. BUG=681885 Review-Url: https://codereview.chromium.org/2643723004 Cr-Commit-Position: refs/heads/master@{#447026} Committed: https://chromium.googlesource.com/chromium/src/+/54a832541cb8aafced2f60deb296... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:420001) as https://chromium.googlesource.com/chromium/src/+/54a832541cb8aafced2f60deb296... |
