|
|
DescriptionIntegrate SMS service with Desktop iOS Promotion.
1- Use sms_service to send sms from desktopIOSPromotionController
2- Add the desktopIOSPromotion Feature flag to aboutflags
3- Update local/profile prefs with promotion shown & and with user interaction
with the promotion.
4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature.
5- Log histograms for promotion dismissal & impressions
This is how the promotion look like: https://drive.google.com/a/google.com/file/d/0B9T1TKmsVG94SGRlaENRZHA5cVBJN0xuR3FwMWJpVjlXLXRB/view
BUG=676655
Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any
account to signin to Chrome, (3)Go to a website with that requires login that
passwords have not been saved for before, (4)Login successfully and click save
when prompt to save password, (5)Promotion should appear and clicking send SMS
should send SMS to the recovery phone number attached to that account.
(6) Open (chrome://flags/) check Desktop to iOS promotions.
Review-Url: https://codereview.chromium.org/2694893002
Cr-Commit-Position: refs/heads/master@{#451496}
Committed: https://chromium.googlesource.com/chromium/src/+/4ce7ff3ecc5d5b638cfdd38b956e80671426742a
Patch Set 1 : SMS integration & loggin #
Total comments: 31
Patch Set 2 : Addressing comments/Update phone number usage/Change view-controller relation/Pending tests #
Total comments: 82
Patch Set 3 : Comments #
Total comments: 28
Patch Set 4 : passwords and histogram changes #
Total comments: 4
Patch Set 5 : add desktop_ios_promotion_util unittest #
Total comments: 31
Patch Set 6 : Rename desktop_ios_promotion_view #
Total comments: 2
Patch Set 7 : tweak text #
Total comments: 2
Patch Set 8 : change the ownership of desktopioscontroller #
Total comments: 4
Patch Set 9 : unique_ptr #
Total comments: 6
Patch Set 10 : nits #Messages
Total messages: 78 (50 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== not complete CL for integrating SMS service with iospromotion BUG= ========== to ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. ==========
mrefaat@chromium.org changed reviewers: + mpearson@chromium.org, sky@chromium.org
mrefaat@chromium.org changed reviewers: + justincohen@chromium.org
mpearson@ for histogram changes sky@ for all other changes.
Please add test coverage.
https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:20: : weak_ptr_factory_(this), Order of member initialize list should match that of declaration order. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:21: recovery_number_(std::string()), Don't bother initializing members that have empty constructors. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:24: sms_service_ = SMSServiceFactory::GetForProfile(profile); Move to member initializer. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:41: (int)entry_point_); Use c++ casts. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:52: desktop_ios_promotion::kEntryPointLocalPrefs[(int)entry_point_ - 1][( Again with the casts, also, use a constant for the -1 everywhere. Probably the constant should be part of the enum to make it obvious. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:76: (int)entry_point_, 5); Use a constant for 5. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:94: if (success) { no {} https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:95: recovery_number_ = number; AFAICT this isn't used. Based on my understanding it looks like you show the bubble, and then query for the phone number. Given that the query can fail, this seems bad. You should query first and only show the bubble if you can get the number, right? https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:25: // Should be instantiated on the UI thread. Should->Must https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:42: desktop_ios_promotion::PromotionEntryPoint entry_point_; const https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:44: std::string recovery_number_; Add descriptions of non-obvious members. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:46: void OnQueryPhoneNumber(SMSService::Request* request, Functions before members. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:46: void OnQueryPhoneNumber(SMSService::Request* request, Add descriptions for functions. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:63: PromotionEntryPoint); All function arguments should be named (see style guide). https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:36: promotion_controller_ = Who owns this? Also, I tend to think you have the ownership wrong. Right now you have the view owning the controller, but I suspect you're going to want the controller to own the view. Otherwise if you need to do processing once the view is deleted you'll be stuck with weird lifetime issues.
still working in util_test & controller_test But all other ready for review https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:20: : weak_ptr_factory_(this), On 2017/02/16 00:13:35, sky wrote: > Order of member initialize list should match that of declaration order. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:21: recovery_number_(std::string()), On 2017/02/16 00:13:35, sky wrote: > Don't bother initializing members that have empty constructors. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:24: sms_service_ = SMSServiceFactory::GetForProfile(profile); On 2017/02/16 00:13:36, sky wrote: > Move to member initializer. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:41: (int)entry_point_); On 2017/02/16 00:13:35, sky wrote: > Use c++ casts. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:52: desktop_ios_promotion::kEntryPointLocalPrefs[(int)entry_point_ - 1][( On 2017/02/16 00:13:36, sky wrote: > Again with the casts, also, use a constant for the -1 everywhere. Probably the > constant should be part of the enum to make it obvious. made a constant but i think it shouldn't be in the enum - just to avoid doing 1 more cast. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:76: (int)entry_point_, 5); On 2017/02/16 00:13:36, sky wrote: > Use a constant for 5. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:94: if (success) { On 2017/02/16 00:13:36, sky wrote: > no {} Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:95: recovery_number_ = number; On 2017/02/16 00:13:36, sky wrote: > AFAICT this isn't used. > > Based on my understanding it looks like you show the bubble, and then query for > the phone number. Given that the query can fail, this seems bad. You should > query first and only show the bubble if you can get the number, right? Our Targeting set will only have the user if their phone number is on the account. But anyway we initially put the bubble with message that doesn't have the number if the number isn't ready, but after the number is ready we update the bubble. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:25: // Should be instantiated on the UI thread. On 2017/02/16 00:13:36, sky wrote: > Should->Must Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:44: std::string recovery_number_; On 2017/02/16 00:13:36, sky wrote: > Add descriptions of non-obvious members. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:46: void OnQueryPhoneNumber(SMSService::Request* request, On 2017/02/16 00:13:36, sky wrote: > Functions before members. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:46: void OnQueryPhoneNumber(SMSService::Request* request, On 2017/02/16 00:13:36, sky wrote: > Functions before members. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:46: void OnQueryPhoneNumber(SMSService::Request* request, On 2017/02/16 00:13:36, sky wrote: > Functions before members. Done. https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:63: PromotionEntryPoint); On 2017/02/16 00:13:36, sky wrote: > All function arguments should be named (see style guide). Ack, typo https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:36: promotion_controller_ = On 2017/02/16 00:13:36, sky wrote: > Who owns this? > > Also, I tend to think you have the ownership wrong. Right now you have the view > owning the controller, but I suspect you're going to want the controller to own > the view. Otherwise if you need to do processing once the view is deleted you'll > be stuck with weird lifetime issues. Done.
Please include checking the histograms using about:histograms in your testing. (I found a bug with logging the wrong value in one place; I hope there are not others. It would be good to check.) --mark https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:130: ->Add((int)desktop_ios_promotion::PromotionDismissalReason::SEND_SMS); Should this be |reason|? Also, nit: mixing different casts within the same function (int) vs. static_cast<int> looks weird. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:27: // values must never be reordered or deleted and reused. Can you confirm that nothing caused these values to be written to logs or stored anywhere before this changelist? https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:30: NO_THANKS, Following histogram enum best practices, please explicitly set = 1, = 2, etc. ( https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... ) https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:50: constexpr int kPromotionEntrypointIndexOffset = -1; Rather than do this hack, why don't you just put an empty entry in kEntryPointLocalPrefs? Then you don't have to remember at *every single use* to apply the offset. https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:84604: - <int value="3" label="Bookmarks existing bubble."/> To confirm, this is just clarifying the name/behavior, not changing the behavior or meaning, right? https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:113564: - <suffix name="BookmarksExistingBubble"/> Do not delete old histograms or old suffixes; instead mark them as obsolete. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... (exception if they've never be written to. Is that the case here?) https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10196: + UserAction be prefixed by the entry point promotion name that the user UserAction -> DismissalReason ? also missing word "will" https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10197: + interacted with. Upon lost focus, does the promotion disappear, or can the user come back and interact with it? https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10206: + will be logged everytime user sees promotion from any entry point. Does "from any entry point" add anything? Do you mean regardless of which they launch Chrome? Or from any other devices? Or ... You can think about omitting the phrase. https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10246: + Weather querying the recovery phone number for a user for desktop to iOS nit: Weather -> Whether also, what does "querying" mean in this description and the histogram name? Checking to see if the phone number if valid according to a database? Sending a message to it and the message not being blocked as undeliverable? Having the user acknowledge that the phone number is theirs? etc. Perhaps revise the histogram name and description for something clearer. https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10254: + Weather sending sms from desktop to iOS promotion was successfull or not. nit: spelling This means Google acknowledged it would send the message, right? It does not guarantee that the target a valid number and can receive the message, right?
https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:95: recovery_number_ = number; On 2017/02/16 21:02:39, mrefaat wrote: > On 2017/02/16 00:13:36, sky wrote: > > AFAICT this isn't used. > > > > Based on my understanding it looks like you show the bubble, and then query > for > > the phone number. Given that the query can fail, this seems bad. You should > > query first and only show the bubble if you can get the number, right? > > Our Targeting set will only have the user if their phone number is on the > account. Not sure I follow. That sort of implies success is always true. Is that right? If it is possible for success to be false, then again, you should verify you really have a number before showing the bubble. Otherwise if you can't actually get the number, then why show the bubble? > But anyway we initially put the bubble with message that doesn't have the number > if the number isn't ready, but after the number is ready we update the bubble.
On 2017/02/17 00:04:12, sky wrote: > https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... > File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc > (right): > > https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/deskt... > chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:95: > recovery_number_ = number; > On 2017/02/16 21:02:39, mrefaat wrote: > > On 2017/02/16 00:13:36, sky wrote: > > > AFAICT this isn't used. > > > > > > Based on my understanding it looks like you show the bubble, and then query > > for > > > the phone number. Given that the query can fail, this seems bad. You should > > > query first and only show the bubble if you can get the number, right? > > > > Our Targeting set will only have the user if their phone number is on the > > account. > > Not sure I follow. That sort of implies success is always true. Is that right? > If it is possible for success to be false, then again, you should verify you > really have a number before showing the bubble. Otherwise if you can't actually > get the number, then why show the bubble? > > > But anyway we initially put the bubble with message that doesn't have the > number > > if the number isn't ready, but after the number is ready we update the bubble. We resolved this on chat, basically we the misunderstanding was related to that the sendSMS api doesn't care about if we have the number or not it actually query for the number by it self.
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:130: ->Add((int)desktop_ios_promotion::PromotionDismissalReason::SEND_SMS); On 2017/02/16 23:18:57, Mark P wrote: > Should this be |reason|? > > Also, nit: mixing different casts within the same function (int) vs. > static_cast<int> looks weird. Thanks for catching this one https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:27: // values must never be reordered or deleted and reused. On 2017/02/16 23:18:57, Mark P wrote: > Can you confirm that nothing caused these values to be written to logs or stored > anywhere before this changelist? Yes, these values have not been written to logs before - the feature is disabled and loging is being added just now. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:30: NO_THANKS, On 2017/02/16 23:18:57, Mark P wrote: > Following histogram enum best practices, please explicitly set = 1, = 2, etc. > > ( > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > ) Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:50: constexpr int kPromotionEntrypointIndexOffset = -1; On 2017/02/16 23:18:57, Mark P wrote: > Rather than do this hack, why don't you just put an empty entry in > kEntryPointLocalPrefs? Then you don't have to remember at *every single use* to > apply the offset. I was going to do that but was afraid of confusion or mistakenly using the first entry ( as no exception will happen so it may skip testing). But i will change it now. https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:84604: - <int value="3" label="Bookmarks existing bubble."/> On 2017/02/16 23:18:57, Mark P wrote: > To confirm, this is just clarifying the name/behavior, not changing the behavior > or meaning, right? Yes, but this is more obvious name. https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:113564: - <suffix name="BookmarksExistingBubble"/> On 2017/02/16 23:18:57, Mark P wrote: > Do not delete old histograms or old suffixes; instead mark them as obsolete. > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > (exception if they've never be written to. Is that the case here?) They have not been used ever before. https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10196: + UserAction be prefixed by the entry point promotion name that the user On 2017/02/16 23:18:57, Mark P wrote: > UserAction -> DismissalReason ? > also missing word "will" Done. https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10197: + interacted with. On 2017/02/16 23:18:57, Mark P wrote: > Upon lost focus, does the promotion disappear, or can the user come back and > interact with it? it disappears https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10206: + will be logged everytime user sees promotion from any entry point. On 2017/02/16 23:18:57, Mark P wrote: > Does "from any entry point" add anything? Do you mean regardless of which they > launch Chrome? Or from any other devices? Or ... > > You can think about omitting the phrase. I'll delete it it adds no meaning https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10246: + Weather querying the recovery phone number for a user for desktop to iOS On 2017/02/16 23:18:57, Mark P wrote: > nit: Weather -> Whether > > also, what does "querying" mean in this description and the histogram name? > Checking to see if the phone number if valid according to a database? Sending a > message to it and the message not being blocked as undeliverable? Having the > user acknowledge that the phone number is theirs? etc. Perhaps revise the > histogram name and description for something clearer. We query for the phone number to show it the user so they know that when they click on send sms this will be the number that recieves the sms. https://codereview.chromium.org/2694893002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10254: + Weather sending sms from desktop to iOS promotion was successfull or not. On 2017/02/16 23:18:57, Mark P wrote: > nit: spelling > > This means Google acknowledged it would send the message, right? It does not > guarantee that the target a valid number and can receive the message, right? Yes, we can't know if the user actually received the message but the api return a success when it initiate the sending.
sky@chromium.org changed reviewers: + vasilii@chromium.org
+vasilii for changes to c/b/ui/passwords https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:19: virtual ~DesktopIOSPromotion() = default; You shouldn't actually need the = default here. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:35: void SetPromotionView(DesktopIOSPromotion* promotion); Document ownership. Can you pass the view into the constructor rather than a setter? Less error prone that way. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:35: void SetPromotionView(DesktopIOSPromotion* promotion); I find it confusing that this function is called SetPromotionView, but the class it takes isn't a view. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:53: void OnQueryPhoneNumber(SMSService::Request* request, optional: name this OnGotPhoneNumber to indicate what this means. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:66: // User's recovery phone number, This is updated by the sms_service. 'This' -> 'this' https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:68: base::WeakPtrFactory<DesktopIOSPromotionController> weak_ptr_factory_; WeakPtrFactory should be your last member. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:69: DesktopIOSPromotion* promotion_view_; You don't initialize this in the initializer list. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:35: switches::kForceDesktopIOSPromotion)) {} https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:330: return CreateDesktopIOSPromotion(); Why does this always call Create? I wouldn't expect a function named Get to create. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:63: VLOG(0) << base::UTF8ToUTF16( Remove this. If you really really want to log, use DVLOG(1). https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:96: if (recovery_phone_label_) {} https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:96: if (recovery_phone_label_) Under what circumstances could recovery_phone_label_ be null? https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.h:28: void UpdateRecoveryPhoneLabel(); override, and comment where override comes from, e.g.: // DesktopIOSPromotion: https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:833: AddChildView( What are the lifetimes here? You have the controller owned by ManagePasswordsUIController, with a reference to the view and the view has a reference back to ManagePasswordsUIController. You have Get always creating a new view. Can the view outlive the controller? Can there be multiple views created with the same controller? https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:834: static_cast<DesktopIOSPromotionView*>(model_.GetDesktopIOSPromotion())); This cast indicates a problem in your design. If GetDesktopIOSPromotion() has to return a view, then DesktopIOSPromotion should *be* a View. Why is DesktopIOSPromotion pure virtual in the first place? https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2391: const char kIOSPromotionEligible[] = "ios.desktoptomobileeligible"; Why is this used? I see it checked, but the value never changed? https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2396: const char kIOSPromotionDone[] = "ios.desktop_ios_promo_done"; When is this used? I see you check it, but never actually update it. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2404: // Indexes of all the entry points presented to the user for "desktop to iOS" Isn't this a bitmask? Please update comment accordingly. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2410: // This is the same time of SMS sending if the user optin to send SMS. Is this second part really true? I see you set it on shown, but isn't the sms only sent if the accepts it?
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:19: virtual ~DesktopIOSPromotion() = default; On 2017/02/17 00:57:22, sky wrote: > You shouldn't actually need the = default here. Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:35: void SetPromotionView(DesktopIOSPromotion* promotion); On 2017/02/17 00:57:22, sky wrote: > I find it confusing that this function is called SetPromotionView, but the class > it takes isn't a view. Acknowledged. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:35: void SetPromotionView(DesktopIOSPromotion* promotion); On 2017/02/17 00:57:23, sky wrote: > Document ownership. Can you pass the view into the constructor rather than a > setter? Less error prone that way. It's not possible as the controller is created before the view, and view takes the controller on its constructor then set it self in the controller. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:35: void SetPromotionView(DesktopIOSPromotion* promotion); On 2017/02/17 00:57:22, sky wrote: > I find it confusing that this function is called SetPromotionView, but the class > it takes isn't a view. Acknowledged. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:35: void SetPromotionView(DesktopIOSPromotion* promotion); On 2017/02/17 00:57:23, sky wrote: > Document ownership. Can you pass the view into the constructor rather than a > setter? Less error prone that way. I added weak pointer comment on the header file, but i prefer not to write comment about the owner here, Should not care about which promotion entry point created it. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:53: void OnQueryPhoneNumber(SMSService::Request* request, On 2017/02/17 00:57:22, sky wrote: > optional: name this OnGotPhoneNumber to indicate what this means. Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:66: // User's recovery phone number, This is updated by the sms_service. On 2017/02/17 00:57:22, sky wrote: > 'This' -> 'this' Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:68: base::WeakPtrFactory<DesktopIOSPromotionController> weak_ptr_factory_; On 2017/02/17 00:57:23, sky wrote: > WeakPtrFactory should be your last member. Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:69: DesktopIOSPromotion* promotion_view_; On 2017/02/17 00:57:23, sky wrote: > You don't initialize this in the initializer list. Acknowledged. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:35: switches::kForceDesktopIOSPromotion)) On 2017/02/17 00:57:23, sky wrote: > {} Not sure why ? https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:330: return CreateDesktopIOSPromotion(); On 2017/02/17 00:57:23, sky wrote: > Why does this always call Create? I wouldn't expect a function named Get to > create. removed it. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:63: VLOG(0) << base::UTF8ToUTF16( On 2017/02/17 00:57:23, sky wrote: > Remove this. If you really really want to log, use DVLOG(1). i had it for debugging - forgot to delete it. Thanks for catching it. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:96: if (recovery_phone_label_) On 2017/02/17 00:57:23, sky wrote: > {} Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:96: if (recovery_phone_label_) On 2017/02/17 00:57:23, sky wrote: > Under what circumstances could recovery_phone_label_ be null? If this was called before the constructor finishes. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.h:28: void UpdateRecoveryPhoneLabel(); On 2017/02/17 00:57:23, sky wrote: > override, and comment where override comes from, e.g.: > > // DesktopIOSPromotion: Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:833: AddChildView( On 2017/02/17 00:57:23, sky wrote: > What are the lifetimes here? You have the controller owned by > ManagePasswordsUIController, with a reference to the view and the view has a > reference back to ManagePasswordsUIController. You have Get always creating a > new view. Can the view outlive the controller? Can there be multiple views > created with the same controller? The parent view (BubbleView) owns the view. and the password UI controller owns the controller. the UI controller already outlive the Bubble view so there is no way that view will outlive the controller. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:834: static_cast<DesktopIOSPromotionView*>(model_.GetDesktopIOSPromotion())); On 2017/02/17 00:57:23, sky wrote: > This cast indicates a problem in your design. If GetDesktopIOSPromotion() has to > return a view, then DesktopIOSPromotion should *be* a View. Why is > DesktopIOSPromotion pure virtual in the first place? Because we can't have dependency from from the controller to the view. So i created a pure class that only has the function that is needed. from the view and made the view implement it. same for the dependency between the ui and ui/views i understood that ui/views can use ui But the reverse is not encouraged. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2391: const char kIOSPromotionEligible[] = "ios.desktoptomobileeligible"; On 2017/02/17 00:57:23, sky wrote: > Why is this used? I see it checked, but the value never changed? The value is set by a pipeline job that updates chrome sync from the growth table. Targeting only users who have active iOS devices & recovery phone numbers & some other features. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2396: const char kIOSPromotionDone[] = "ios.desktop_ios_promo_done"; On 2017/02/17 00:57:23, sky wrote: > When is this used? I see you check it, but never actually update it. The value is changed on the iOS client when the user complete the flow. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2404: // Indexes of all the entry points presented to the user for "desktop to iOS" On 2017/02/17 00:57:23, sky wrote: > Isn't this a bitmask? Please update comment accordingly. Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2410: // This is the same time of SMS sending if the user optin to send SMS. On 2017/02/17 00:57:23, sky wrote: > Is this second part really true? I see you set it on shown, but isn't the sms > only sent if the accepts it? Yes but once the user click SMS then this will not show again So the last impression time is the same as the time of the sending SMS for users who choose to send sms.
tentative metrics lgtm with more revisions requested I'm disappointed you didn't say anything about my earlier request: >>> Please include checking the histograms using about:histograms in your testing. (I found a bug with logging the wrong value in one place; I hope there are not others. It would be good to check.) >>> Have you tried it? thanks, :-) mark https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:40: enum class PromotionEntryPoint { optional nit: it's clear you explicitly decided to skip 0, that's why you need to jump through hoops with the array below. In this case, please add a comment inside the { here, something like // Intentionally skipped the = 0 value because ... https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10206: + will be logged everytime user sees desktop to iOS promotion. nit: I think it should be "every time" also, add "the" before "user", and add "the" before "desktop" (both of these last two are for consistency with the previous sentence; it reads better) https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10246: + Wether querying the recovery phone number for a user for desktop to iOS nit: Wether -> Whether (spelling wrong) https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10246: + Wether querying the recovery phone number for a user for desktop to iOS nit: add "the" or "a" before desktop https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10247: + promotion was successfull or not, This phone number is presented to the user nit: successful (spelling) https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10247: + promotion was successfull or not, This phone number is presented to the user nit: , -> . https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10248: + so they know that we Chrome will send SMS to this number. nit: drop "we" https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10248: + so they know that we Chrome will send SMS to this number. On 2017/02/17 00:20:59, mrefaat wrote: > On 2017/02/16 23:18:57, Mark P wrote: > > nit: Weather -> Whether > > > > also, what does "querying" mean in this description and the histogram name? > > Checking to see if the phone number if valid according to a database? Sending > a > > message to it and the message not being blocked as undeliverable? Having the > > user acknowledge that the phone number is theirs? etc. Perhaps revise the > > histogram name and description for something clearer. > > We query for the phone number to show it the user so they know that when they > click on send sms this will be the number that recieves the sms. I still understand. By "querying the recovery number" means you ask the user to enter a recovery number? Or you show the user a recovery number and then ask for confirmation that it's right? (If so, does "successful" means the user confirm its correctness? That's an odd word choice for that too.) This description (and possibly histogram name) still needs work. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10255: + Wether the SMS service api iniatiated SMS sending successfully from desktop nit: two spelling errors in this line https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10255: + Wether the SMS service api iniatiated SMS sending successfully from desktop nit: add "the" or "a" before desktop
Patchset #3 (id:80001) has been deleted
On 2017/02/17 06:15:28, Mark P wrote: > tentative metrics lgtm with more revisions requested > > I'm disappointed you didn't say anything about my earlier request: > >>> > Please include checking the histograms using about:histograms in your testing. > (I found a bug with logging the wrong value in one place; I hope there are not > others. It would be good to check.) > >>> > > Have you tried it? > Sorry i didn't notice you comment last time, yes i've tried it when i added to about_flags file. > thanks, :-) > mark > > https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... > File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h > (right): > > https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... > chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:40: enum > class PromotionEntryPoint { > optional nit: it's clear you explicitly decided to skip 0, that's why you need > to jump through hoops with the array below. In this case, please add a comment > inside the { here, something like > // Intentionally skipped the = 0 value because ... > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10206: + will be logged everytime > user sees desktop to iOS promotion. > nit: I think it should be "every time" > also, add "the" before "user", > and add "the" before "desktop" > (both of these last two are for consistency with the previous sentence; it reads > better) > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10246: + Wether querying the recovery > phone number for a user for desktop to iOS > nit: Wether -> Whether > (spelling wrong) > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10246: + Wether querying the recovery > phone number for a user for desktop to iOS > nit: add "the" or "a" before desktop > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10247: + promotion was successfull or > not, This phone number is presented to the user > nit: successful > (spelling) > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10247: + promotion was successfull or > not, This phone number is presented to the user > nit: , -> . > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10248: + so they know that we Chrome > will send SMS to this number. > nit: drop "we" > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10248: + so they know that we Chrome > will send SMS to this number. > On 2017/02/17 00:20:59, mrefaat wrote: > > On 2017/02/16 23:18:57, Mark P wrote: > > > nit: Weather -> Whether > > > > > > also, what does "querying" mean in this description and the histogram name? > > > Checking to see if the phone number if valid according to a database? > Sending > > a > > > message to it and the message not being blocked as undeliverable? Having > the > > > user acknowledge that the phone number is theirs? etc. Perhaps revise the > > > histogram name and description for something clearer. > > > > We query for the phone number to show it the user so they know that when they > > click on send sms this will be the number that recieves the sms. > > I still understand. By "querying the recovery number" means you ask the user to > enter a recovery number? Or you show the user a recovery number and then ask > for confirmation that it's right? (If so, does "successful" means the user > confirm its correctness? That's an odd word choice for that too.) > > This description (and possibly histogram name) still needs work. > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10255: + Wether the SMS service api > iniatiated SMS sending successfully from desktop > nit: two spelling errors in this line > > https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:10255: + Wether the SMS service api > iniatiated SMS sending successfully from desktop > nit: add "the" or "a" before desktop
Description was changed from ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. ========== to ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. Also Run about_flags unit test ==========
Description was changed from ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. Also Run about_flags unit test ========== to ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. (6) Open (chrome://flags/) check Desktop to iOS promotions. ==========
https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:40: enum class PromotionEntryPoint { On 2017/02/17 06:15:28, Mark P wrote: > optional nit: it's clear you explicitly decided to skip 0, that's why you need > to jump through hoops with the array below. In this case, please add a comment > inside the { here, something like > // Intentionally skipped the = 0 value because ... Done. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10206: + will be logged everytime user sees desktop to iOS promotion. On 2017/02/17 06:15:28, Mark P wrote: > nit: I think it should be "every time" > also, add "the" before "user", > and add "the" before "desktop" > (both of these last two are for consistency with the previous sentence; it reads > better) Done. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10246: + Wether querying the recovery phone number for a user for desktop to iOS On 2017/02/17 06:15:28, Mark P wrote: > nit: Wether -> Whether > (spelling wrong) Done. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10246: + Wether querying the recovery phone number for a user for desktop to iOS On 2017/02/17 06:15:28, Mark P wrote: > nit: add "the" or "a" before desktop Done. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10247: + promotion was successfull or not, This phone number is presented to the user On 2017/02/17 06:15:28, Mark P wrote: > nit: successful > (spelling) Done. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10247: + promotion was successfull or not, This phone number is presented to the user On 2017/02/17 06:15:28, Mark P wrote: > nit: , -> . Acknowledged. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10248: + so they know that we Chrome will send SMS to this number. On 2017/02/17 06:15:28, Mark P wrote: > nit: drop "we" Done. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10248: + so they know that we Chrome will send SMS to this number. On 2017/02/17 06:15:28, Mark P wrote: > On 2017/02/17 00:20:59, mrefaat wrote: > > On 2017/02/16 23:18:57, Mark P wrote: > > > nit: Weather -> Whether > > > > > > also, what does "querying" mean in this description and the histogram name? > > > Checking to see if the phone number if valid according to a database? > Sending > > a > > > message to it and the message not being blocked as undeliverable? Having > the > > > user acknowledge that the phone number is theirs? etc. Perhaps revise the > > > histogram name and description for something clearer. > > > > We query for the phone number to show it the user so they know that when they > > click on send sms this will be the number that recieves the sms. > > I still understand. By "querying the recovery number" means you ask the user to > enter a recovery number? Or you show the user a recovery number and then ask > for confirmation that it's right? (If so, does "successful" means the user > confirm its correctness? That's an odd word choice for that too.) > > This description (and possibly histogram name) still needs work. I changed the description a bit, We use the QueryPhone number service call to get a phone number that we present to the user. we don't get a confirmation from the user if the number is correct or not. we only present it so the user knows the number that will receive the SMS. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10255: + Wether the SMS service api iniatiated SMS sending successfully from desktop On 2017/02/17 06:15:28, Mark P wrote: > nit: add "the" or "a" before desktop Done. https://codereview.chromium.org/2694893002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10255: + Wether the SMS service api iniatiated SMS sending successfully from desktop On 2017/02/17 06:15:28, Mark P wrote: > nit: two spelling errors in this line Done.
https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:13: // Interface for The Desktop iOS Promotion. Interface for a view or a controller? https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:121: DesktopIOSPromotion* CreateDesktopIOSPromotion(); The model should create DesktopIOSPromotionController in ReplaceToShowPromotionIfNeeded(). You only expose a simple getter for the controller. https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.h:180: std::unique_ptr<DesktopIOSPromotionController> ios_promotion_controller_; This shouldn't be here. Move it to ManagePasswordsBubbleModel. ManagePasswordsUIController and PasswordsModelDelegate should stay intact. https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:833: AddChildView(static_cast<DesktopIOSPromotionView*>( You can create DesktopIOSPromotionView here directly. If you don't plan to support Mac then I'd drop CreateDesktopIOSPromotionView entirely.
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:13: // Interface for The Desktop iOS Promotion. On 2017/02/17 12:59:14, vasilii wrote: > Interface for a view or a controller? Acknowledged. https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:121: DesktopIOSPromotion* CreateDesktopIOSPromotion(); On 2017/02/17 12:59:14, vasilii wrote: > The model should create DesktopIOSPromotionController in > ReplaceToShowPromotionIfNeeded(). You only expose a simple getter for the > controller. Done. https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.h:180: std::unique_ptr<DesktopIOSPromotionController> ios_promotion_controller_; On 2017/02/17 12:59:14, vasilii wrote: > This shouldn't be here. Move it to ManagePasswordsBubbleModel. > ManagePasswordsUIController and PasswordsModelDelegate should stay intact. Done. https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:833: AddChildView(static_cast<DesktopIOSPromotionView*>( On 2017/02/17 12:59:15, vasilii wrote: > You can create DesktopIOSPromotionView here directly. If you don't plan to > support Mac then I'd drop CreateDesktopIOSPromotionView entirely. Planning to support mac but not with this release so i'll remove it now.
c/b/ui/passwords and manage_passwords_bubble_view.cc LGTM https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:35: #include "chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h" not needed. https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:121: DesktopIOSPromotionController* GetDesktopIOSPromotionController(); Optionally: rename to ios_promotion_controller() and inline. Currently it looks more difficult than it is.
https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:35: #include "chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h" On 2017/02/17 14:19:37, vasilii wrote: > not needed. Done. https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:121: DesktopIOSPromotionController* GetDesktopIOSPromotionController(); On 2017/02/17 14:19:37, vasilii wrote: > Optionally: rename to ios_promotion_controller() and inline. Currently it looks > more difficult than it is. Done.
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:35: switches::kForceDesktopIOSPromotion)) On 2017/02/17 04:31:49, mrefaat wrote: > On 2017/02/17 00:57:23, sky wrote: > > {} > Not sure why ? Generally when the conditional spans multiple lines you use {} https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:36: int bubbleWidth = bubble_width https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:96: if (recovery_phone_label_) On 2017/02/17 04:31:49, mrefaat wrote: > On 2017/02/17 00:57:23, sky wrote: > > {} > > Done. Initialize recovery_phone_label_ in the member initialize list to avoid that problem. It's better practice to initialize objects in the member initialize list for this very issue. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:833: AddChildView( On 2017/02/17 04:31:50, mrefaat wrote: > On 2017/02/17 00:57:23, sky wrote: > > What are the lifetimes here? You have the controller owned by > > ManagePasswordsUIController, with a reference to the view and the view has a > > reference back to ManagePasswordsUIController. You have Get always creating a > > new view. Can the view outlive the controller? Can there be multiple views > > created with the same controller? > > The parent view (BubbleView) owns the view. and the password UI controller owns > the controller. > the UI controller already outlive the Bubble view so there is no way that view > will outlive the controller. How does the lifetime of the password ui controller (is that ManagePasswordsBubbleModel?) relate to that of the bubble? Is there a single ManagePasswordsBubbleModel for every bubble? This should be documented. Your code does never unsets the view from the controller, so I want to make sure there isn't the possibility that the controller outlives the view (bubble). Earlier I suggested the controller should likely own the view. That was based on my understanding that you had some long running operations that needed to outlive the view. That no longer seems to be the case. Should DesktopIOSPromotionViewsView own and create the controller? That way lifetime is clearer. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:834: static_cast<DesktopIOSPromotionView*>(model_.GetDesktopIOSPromotion())); On 2017/02/17 04:31:50, mrefaat wrote: > On 2017/02/17 00:57:23, sky wrote: > > This cast indicates a problem in your design. If GetDesktopIOSPromotion() has > to > > return a view, then DesktopIOSPromotion should *be* a View. Why is > > DesktopIOSPromotion pure virtual in the first place? > > Because we can't have dependency from from the controller to the view. > So i created a pure class that only has the function that is needed. from the > view and made the view implement it. > same for the dependency between the ui and ui/views i understood that ui/views > can use ui But the reverse is not encouraged. > You changed it to create the view directly, which makes much more sense. thanks! https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2391: const char kIOSPromotionEligible[] = "ios.desktoptomobileeligible"; On 2017/02/17 04:31:50, mrefaat wrote: > On 2017/02/17 00:57:23, sky wrote: > > Why is this used? I see it checked, but the value never changed? > > The value is set by a pipeline job that updates chrome sync from the growth > table. > Targeting only users who have active iOS devices & recovery phone numbers & some > other features. Are you saying the value is set on the server side? Please update the comment accordingly. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2396: const char kIOSPromotionDone[] = "ios.desktop_ios_promo_done"; On 2017/02/17 04:31:50, mrefaat wrote: > On 2017/02/17 00:57:23, sky wrote: > > When is this used? I see you check it, but never actually update it. > > The value is changed on the iOS client when the user complete the flow. Please update the comment to better indicate this. From the description it isn't clear 'and signed in' refers to the ios client. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2410: // This is the same time of SMS sending if the user optin to send SMS. On 2017/02/17 04:31:50, mrefaat wrote: > On 2017/02/17 00:57:23, sky wrote: > > Is this second part really true? I see you set it on shown, but isn't the sms > > only sent if the accepts it? > > Yes but once the user click SMS then this will not show again So the last > impression time is the same as the time of the sending SMS for users who choose > to send sms. Don't you set the value on initial show which is not necessarily the same as when the user clicks ok? https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:9: class DesktopIOSPromotion { As mentioned, please rename this to DesktopIOPromotionView. You can rename the existing one to DesktopIOPromotionViewViews. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:11: virtual void UpdateRecoveryPhoneLabel() = 0; My previous comment about not needing the destructor was more around = default. Chromium style is to have a virtual destructor. You can either inline or = default, which ever you prefer. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:33: desktop_ios_promotion::PromotionEntryPoint GetEntryPoint(); Name this entry_point() and inline. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:36: void SetPromotion(DesktopIOSPromotion* promotion); Once you rename DesktopIOSPromotion rename this to SetPromotionView. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:69: // A Weak pointer to the promotion. promotion view https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:124: 1, static_cast<int>(PromotionEntryPoint::ENTRY_POINT_MAX_VALUE), I assume 1 is the minimum value, which you are assuming aligns with that of the enum. You should use a constant to make that clear. Perhaps ENTRY_POINT_MIN_VALUE = SAVE_PASSWORD_BUBBLE. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:125: static_cast<int>(PromotionEntryPoint::ENTRY_POINT_MAX_VALUE) + 1, Why the +1 here? https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:56: {"", ""}, Why the empty values with no comment? https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:18: namespace { This is a good start on test coverage, but you need coverage of the Controller <-> View interaction. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:29: }; DISALLOW... https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:69: }; private: DISALLOW... https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:91: {false, 0, false, 0, 1, 0, false, false, false}, Use the enum values in here as otherwise it isn't at all clear where values like 1, 3 and 10 mean. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:462: ios_promotion_controller_.reset(new DesktopIOSPromotionController( Use MakeUnique (see threads on chromium-dev for details).
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:35: switches::kForceDesktopIOSPromotion)) On 2017/02/17 19:06:25, sky wrote: > On 2017/02/17 04:31:49, mrefaat wrote: > > On 2017/02/17 00:57:23, sky wrote: > > > {} > > Not sure why ? > > Generally when the conditional spans multiple lines you use {} Acknowledged. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:36: int bubbleWidth = On 2017/02/17 19:06:26, sky wrote: > bubble_width Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc:96: if (recovery_phone_label_) On 2017/02/17 19:06:25, sky wrote: > On 2017/02/17 04:31:49, mrefaat wrote: > > On 2017/02/17 00:57:23, sky wrote: > > > {} > > > > Done. > > Initialize recovery_phone_label_ in the member initialize list to avoid that > problem. It's better practice to initialize objects in the member initialize > list for this very issue. Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:833: AddChildView( On 2017/02/17 19:06:26, sky wrote: > On 2017/02/17 04:31:50, mrefaat wrote: > > On 2017/02/17 00:57:23, sky wrote: > > > What are the lifetimes here? You have the controller owned by > > > ManagePasswordsUIController, with a reference to the view and the view has a > > > reference back to ManagePasswordsUIController. You have Get always creating > a > > > new view. Can the view outlive the controller? Can there be multiple views > > > created with the same controller? > > > > The parent view (BubbleView) owns the view. and the password UI controller > owns > > the controller. > > the UI controller already outlive the Bubble view so there is no way that view > > will outlive the controller. > > How does the lifetime of the password ui controller (is that > ManagePasswordsBubbleModel?) relate to that of the bubble? Is there a single > ManagePasswordsBubbleModel for every bubble? This should be documented. Your > code does never unsets the view from the controller, so I want to make sure > there isn't the possibility that the controller outlives the view (bubble). > > Earlier I suggested the controller should likely own the view. That was based on > my understanding that you had some long running operations that needed to > outlive the view. That no longer seems to be the case. Should > DesktopIOSPromotionViewsView own and create the controller? That way lifetime is > clearer. I think moving the controller to the model and made the password model it also clear. And it guarantees that the controller dies with the model that controls the parent view & the view dies with the parent view. https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:834: static_cast<DesktopIOSPromotionView*>(model_.GetDesktopIOSPromotion())); On 2017/02/17 19:06:26, sky wrote: > On 2017/02/17 04:31:50, mrefaat wrote: > > On 2017/02/17 00:57:23, sky wrote: > > > This cast indicates a problem in your design. If GetDesktopIOSPromotion() > has > > to > > > return a view, then DesktopIOSPromotion should *be* a View. Why is > > > DesktopIOSPromotion pure virtual in the first place? > > > > Because we can't have dependency from from the controller to the view. > > So i created a pure class that only has the function that is needed. from the > > view and made the view implement it. > > same for the dependency between the ui and ui/views i understood that ui/views > > can use ui But the reverse is not encouraged. > > > > You changed it to create the view directly, which makes much more sense. thanks! Done that https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2391: const char kIOSPromotionEligible[] = "ios.desktoptomobileeligible"; On 2017/02/17 19:06:29, sky wrote: > On 2017/02/17 04:31:50, mrefaat wrote: > > On 2017/02/17 00:57:23, sky wrote: > > > Why is this used? I see it checked, but the value never changed? > > > > The value is set by a pipeline job that updates chrome sync from the growth > > table. > > Targeting only users who have active iOS devices & recovery phone numbers & > some > > other features. > > Are you saying the value is set on the server side? Please update the comment > accordingly. Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2396: const char kIOSPromotionDone[] = "ios.desktop_ios_promo_done"; On 2017/02/17 19:06:27, sky wrote: > On 2017/02/17 04:31:50, mrefaat wrote: > > On 2017/02/17 00:57:23, sky wrote: > > > When is this used? I see you check it, but never actually update it. > > > > The value is changed on the iOS client when the user complete the flow. > > Please update the comment to better indicate this. From the description it isn't > clear 'and signed in' refers to the ios client. Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2410: // This is the same time of SMS sending if the user optin to send SMS. On 2017/02/17 19:06:29, sky wrote: > On 2017/02/17 04:31:50, mrefaat wrote: > > On 2017/02/17 00:57:23, sky wrote: > > > Is this second part really true? I see you set it on shown, but isn't the > sms > > > only sent if the accepts it? > > > > Yes but once the user click SMS then this will not show again So the last > > impression time is the same as the time of the sending SMS for users who > choose > > to send sms. > > Don't you set the value on initial show which is not necessarily the same as > when the user clicks ok? we only care about the day. so it may be off by couple seconds but it still represent the sms click time. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:9: class DesktopIOSPromotion { On 2017/02/17 19:06:29, sky wrote: > As mentioned, please rename this to DesktopIOPromotionView. You can rename the > existing one to DesktopIOPromotionViewViews. Done. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:11: virtual void UpdateRecoveryPhoneLabel() = 0; On 2017/02/17 19:06:29, sky wrote: > My previous comment about not needing the destructor was more around = default. > Chromium style is to have a virtual destructor. You can either inline or = > default, which ever you prefer. Done. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:33: desktop_ios_promotion::PromotionEntryPoint GetEntryPoint(); On 2017/02/17 19:06:29, sky wrote: > Name this entry_point() and inline. Done. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:36: void SetPromotion(DesktopIOSPromotion* promotion); On 2017/02/17 19:06:29, sky wrote: > Once you rename DesktopIOSPromotion rename this to SetPromotionView. Done. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:69: // A Weak pointer to the promotion. On 2017/02/17 19:06:29, sky wrote: > promotion view Done. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:124: 1, static_cast<int>(PromotionEntryPoint::ENTRY_POINT_MAX_VALUE), On 2017/02/17 19:06:29, sky wrote: > I assume 1 is the minimum value, which you are assuming aligns with that of the > enum. You should use a constant to make that clear. Perhaps > ENTRY_POINT_MIN_VALUE = SAVE_PASSWORD_BUBBLE. Done. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:125: static_cast<int>(PromotionEntryPoint::ENTRY_POINT_MAX_VALUE) + 1, On 2017/02/17 19:06:29, sky wrote: > Why the +1 here? These are the number of buckets should be max_boundry +1 https://cs.chromium.org/chromium/src/base/metrics/histogram_macros_internal.h... https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:56: {"", ""}, On 2017/02/17 19:06:29, sky wrote: > Why the empty values with no comment? Added comment. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:18: namespace { On 2017/02/17 19:06:30, sky wrote: > This is a good start on test coverage, but you need coverage of the Controller > <-> View interaction. If possible, i prefer to do this on a separate CL as these interactions can be seen from the visual test (i'm already working on it and it and it also adds finch checking but probably first day next week will send it), will add you to that cl. For now this is not enabled and doesn't work for any user (even canary with the feature on - you have to use the switch to be eligible) the eligibility pipeline didn't run yet and wont run for some time (so all users are ineligible) https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:29: }; On 2017/02/17 19:06:29, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:69: }; On 2017/02/17 19:06:29, sky wrote: > private: DISALLOW... Done. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:91: {false, 0, false, 0, 1, 0, false, false, false}, On 2017/02/17 19:06:29, sky wrote: > Use the enum values in here as otherwise it isn't at all clear where values like > 1, 3 and 10 mean. Non of them has enum values on desktop_ios_promotion_util, But they should be clear from the struct variable names But added a comment to make it also more clear. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:462: ios_promotion_controller_.reset(new DesktopIOSPromotionController( On 2017/02/17 19:06:30, sky wrote: > Use MakeUnique (see threads on chromium-dev for details). Done.
Patchset #5 (id:170001) has been deleted
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. (6) Open (chrome://flags/) check Desktop to iOS promotions. ========== to ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions This is how the promotion look like: https://drive.google.com/a/google.com/file/d/0B9T1TKmsVG94SGRlaENRZHA5cVBJN0x... BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. (6) Open (chrome://flags/) check Desktop to iOS promotions. ==========
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:833: AddChildView( On 2017/02/17 21:53:14, mrefaat wrote: > On 2017/02/17 19:06:26, sky wrote: > > On 2017/02/17 04:31:50, mrefaat wrote: > > > On 2017/02/17 00:57:23, sky wrote: > > > > What are the lifetimes here? You have the controller owned by > > > > ManagePasswordsUIController, with a reference to the view and the view has > a > > > > reference back to ManagePasswordsUIController. You have Get always > creating > > a > > > > new view. Can the view outlive the controller? Can there be multiple views > > > > created with the same controller? > > > > > > The parent view (BubbleView) owns the view. and the password UI controller > > owns > > > the controller. > > > the UI controller already outlive the Bubble view so there is no way that > view > > > will outlive the controller. > > > > How does the lifetime of the password ui controller (is that > > ManagePasswordsBubbleModel?) relate to that of the bubble? Is there a single > > ManagePasswordsBubbleModel for every bubble? This should be documented. Your > > code does never unsets the view from the controller, so I want to make sure > > there isn't the possibility that the controller outlives the view (bubble). > > > > Earlier I suggested the controller should likely own the view. That was based > on > > my understanding that you had some long running operations that needed to > > outlive the view. That no longer seems to be the case. Should > > DesktopIOSPromotionViewsView own and create the controller? That way lifetime > is > > clearer. > > I think moving the controller to the model and made the password model it also > clear. > And it guarantees that the controller dies with the model that controls the > parent view & the view dies with the parent view. Sorry if I wasn't clear, too many models and controllers. ManagePasswordsBubbleModel doesn't use DesktopIOSPromotionController at all. So, why not have DesktopIOSPromotionBubbleView create and own DesktopIOSPromotionController? Lifetime is even easier then. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2410: // This is the same time of SMS sending if the user optin to send SMS. On 2017/02/17 21:53:14, mrefaat wrote: > On 2017/02/17 19:06:29, sky wrote: > > On 2017/02/17 04:31:50, mrefaat wrote: > > > On 2017/02/17 00:57:23, sky wrote: > > > > Is this second part really true? I see you set it on shown, but isn't the > > sms > > > > only sent if the accepts it? > > > > > > Yes but once the user click SMS then this will not show again So the last > > > impression time is the same as the time of the sending SMS for users who > > choose > > > to send sms. > > > > Don't you set the value on initial show which is not necessarily the same as > > when the user clicks ok? > > we only care about the day. so it may be off by couple seconds but it still > represent the sms click time. Sure, but the comment doesn't indicate this. Please update the comment. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:18: namespace { On 2017/02/17 21:53:15, mrefaat wrote: > On 2017/02/17 19:06:30, sky wrote: > > This is a good start on test coverage, but you need coverage of the Controller > > <-> View interaction. > > If possible, i prefer to do this on a separate CL as these interactions can be > seen from the visual test (i'm already working on it and it and it also adds > finch checking but probably first day next week will send it), will add you to > that cl. > > For now this is not enabled and doesn't work for any user (even canary with the > feature on - you have to use the switch to be eligible) the eligibility pipeline > didn't run yet and wont run for some time (so all users are ineligible) In the future, please have test coverage ready at the time of review. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:91: {false, 0, false, 0, 1, 0, false, false, false}, On 2017/02/17 21:53:15, mrefaat wrote: > On 2017/02/17 19:06:29, sky wrote: > > Use the enum values in here as otherwise it isn't at all clear where values > like > > 1, 3 and 10 mean. > > Non of them has enum values on desktop_ios_promotion_util, But they should be > clear from the struct variable names > But added a comment to make it also more clear. Aren't the bitmask values created from an enum? I'm suggesting you have that code here. For example, 1 << SAVE_PASSWORD_BUBBLE https://codereview.chromium.org/2694893002/diff/250001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_view.h (right): https://codereview.chromium.org/2694893002/diff/250001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_view.h:9: class DesktopIOSPromotionView { Thanks for the rename, much better! https://codereview.chromium.org/2694893002/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.h (right): https://codereview.chromium.org/2694893002/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.h:21: class DesktopIOSPromotionBubbleView : public DesktopIOSPromotionView, Thanks for the rename much better!
histograms.xml still lgtm, one trivial comment --mark https://codereview.chromium.org/2694893002/diff/270001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2694893002/diff/270001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10313: + returned the a phone number or failed. This phone number is presented to the nit: "the a" -> "a"
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:833: AddChildView( On 2017/02/17 22:49:32, sky wrote: > On 2017/02/17 21:53:14, mrefaat wrote: > > On 2017/02/17 19:06:26, sky wrote: > > > On 2017/02/17 04:31:50, mrefaat wrote: > > > > On 2017/02/17 00:57:23, sky wrote: > > > > > What are the lifetimes here? You have the controller owned by > > > > > ManagePasswordsUIController, with a reference to the view and the view > has > > a > > > > > reference back to ManagePasswordsUIController. You have Get always > > creating > > > a > > > > > new view. Can the view outlive the controller? Can there be multiple > views > > > > > created with the same controller? > > > > > > > > The parent view (BubbleView) owns the view. and the password UI controller > > > owns > > > > the controller. > > > > the UI controller already outlive the Bubble view so there is no way that > > view > > > > will outlive the controller. > > > > > > How does the lifetime of the password ui controller (is that > > > ManagePasswordsBubbleModel?) relate to that of the bubble? Is there a single > > > ManagePasswordsBubbleModel for every bubble? This should be documented. Your > > > code does never unsets the view from the controller, so I want to make sure > > > there isn't the possibility that the controller outlives the view (bubble). > > > > > > Earlier I suggested the controller should likely own the view. That was > based > > on > > > my understanding that you had some long running operations that needed to > > > outlive the view. That no longer seems to be the case. Should > > > DesktopIOSPromotionViewsView own and create the controller? That way > lifetime > > is > > > clearer. > > > > I think moving the controller to the model and made the password model it also > > clear. > > And it guarantees that the controller dies with the model that controls the > > parent view & the view dies with the parent view. > > Sorry if I wasn't clear, too many models and controllers. > ManagePasswordsBubbleModel doesn't use DesktopIOSPromotionController at all. So, > why not have DesktopIOSPromotionBubbleView create and own > DesktopIOSPromotionController? Lifetime is even easier then. Done. https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/common/pref_name... chrome/common/pref_names.cc:2410: // This is the same time of SMS sending if the user optin to send SMS. On 2017/02/17 22:49:32, sky wrote: > On 2017/02/17 21:53:14, mrefaat wrote: > > On 2017/02/17 19:06:29, sky wrote: > > > On 2017/02/17 04:31:50, mrefaat wrote: > > > > On 2017/02/17 00:57:23, sky wrote: > > > > > Is this second part really true? I see you set it on shown, but isn't > the > > > sms > > > > > only sent if the accepts it? > > > > > > > > Yes but once the user click SMS then this will not show again So the last > > > > impression time is the same as the time of the sending SMS for users who > > > choose > > > > to send sms. > > > > > > Don't you set the value on initial show which is not necessarily the same as > > > when the user clicks ok? > > > > we only care about the day. so it may be off by couple seconds but it still > > represent the sms click time. > > Sure, but the comment doesn't indicate this. Please update the comment. Acknowledged. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc (right): https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:18: namespace { On 2017/02/17 22:49:32, sky wrote: > On 2017/02/17 21:53:15, mrefaat wrote: > > On 2017/02/17 19:06:30, sky wrote: > > > This is a good start on test coverage, but you need coverage of the > Controller > > > <-> View interaction. > > > > If possible, i prefer to do this on a separate CL as these interactions can be > > seen from the visual test (i'm already working on it and it and it also adds > > finch checking but probably first day next week will send it), will add you to > > that cl. > > > > For now this is not enabled and doesn't work for any user (even canary with > the > > feature on - you have to use the switch to be eligible) the eligibility > pipeline > > didn't run yet and wont run for some time (so all users are ineligible) > > In the future, please have test coverage ready at the time of review. Acknowledged. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:18: namespace { On 2017/02/17 22:49:32, sky wrote: > On 2017/02/17 21:53:15, mrefaat wrote: > > On 2017/02/17 19:06:30, sky wrote: > > > This is a good start on test coverage, but you need coverage of the > Controller > > > <-> View interaction. > > > > If possible, i prefer to do this on a separate CL as these interactions can be > > seen from the visual test (i'm already working on it and it and it also adds > > finch checking but probably first day next week will send it), will add you to > > that cl. > > > > For now this is not enabled and doesn't work for any user (even canary with > the > > feature on - you have to use the switch to be eligible) the eligibility > pipeline > > didn't run yet and wont run for some time (so all users are ineligible) > > In the future, please have test coverage ready at the time of review. Acknowledged. https://codereview.chromium.org/2694893002/diff/190001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc:91: {false, 0, false, 0, 1, 0, false, false, false}, On 2017/02/17 22:49:32, sky wrote: > On 2017/02/17 21:53:15, mrefaat wrote: > > On 2017/02/17 19:06:29, sky wrote: > > > Use the enum values in here as otherwise it isn't at all clear where values > > like > > > 1, 3 and 10 mean. > > > > Non of them has enum values on desktop_ios_promotion_util, But they should be > > clear from the struct variable names > > But added a comment to make it also more clear. > > Aren't the bitmask values created from an enum? I'm suggesting you have that > code here. For example, 1 << SAVE_PASSWORD_BUBBLE Done. https://codereview.chromium.org/2694893002/diff/270001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2694893002/diff/270001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10313: + returned the a phone number or failed. This phone number is presented to the On 2017/02/17 23:41:20, Mark P wrote: > nit: "the a" -> "a" Done.
Patchset #10 (id:290001) has been deleted
Patchset #6 (id:210001) has been deleted
Patchset #6 (id:230001) has been deleted
Patchset #8 (id:310001) has been deleted
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...
Patchset #8 (id:330001) has been deleted
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #8 (id:340001) has been deleted
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:113: if (promotion_view_) Is this conditional necessary anymore? https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:37: promotion_controller_( I believe you leak this, use a unique_ptr. Now that the view owns the controller can the constructor take the view? That is, no SetPromotionView.
Patchset #8 (id:360001) has been deleted
https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:113: if (promotion_view_) On 2017/02/18 16:40:45, sky wrote: > Is this conditional necessary anymore? removed it. https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:37: promotion_controller_( On 2017/02/18 16:40:45, sky wrote: > I believe you leak this, use a unique_ptr. Now that the view owns the controller > can the constructor take the view? That is, no SetPromotionView. Done.
LGTM with the following updated. https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:108: if (success) { Generally only use {} when conditional spans multiple lines. https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h (right): https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:71: DesktopIOSPromotionView* promotion_view_ = nullptr; Now that you set this in the initializer list it isn't necessary to set here too. In other words remove the '= nullptr'. https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:38: new DesktopIOSPromotionController(profile, this, entry_point)) { Use MakeUnique (see threads on chromium-dev for reason why).
https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:108: if (success) { On 2017/02/18 18:03:51, sky wrote: > Generally only use {} when conditional spans multiple lines. Yes but here i'm doing two things so i need the {} update the number & call the view update https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h (right): https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:71: DesktopIOSPromotionView* promotion_view_ = nullptr; On 2017/02/18 18:03:51, sky wrote: > Now that you set this in the initializer list it isn't necessary to set here > too. In other words remove the '= nullptr'. Done. https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:38: new DesktopIOSPromotionController(profile, this, entry_point)) { On 2017/02/18 18:03:51, sky wrote: > Use MakeUnique (see threads on chromium-dev for reason why). Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mrefaat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, mpearson@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2694893002/#ps420001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 420001, "attempt_start_ts": 1487456608918550, "parent_rev": "a39dd19ca07c097d02063433eda6473860f05d4e", "commit_rev": "4ce7ff3ecc5d5b638cfdd38b956e80671426742a"}
Message was sent while issue was closed.
Description was changed from ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions This is how the promotion look like: https://drive.google.com/a/google.com/file/d/0B9T1TKmsVG94SGRlaENRZHA5cVBJN0x... BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. (6) Open (chrome://flags/) check Desktop to iOS promotions. ========== to ========== Integrate SMS service with Desktop iOS Promotion. 1- Use sms_service to send sms from desktopIOSPromotionController 2- Add the desktopIOSPromotion Feature flag to aboutflags 3- Update local/profile prefs with promotion shown & and with user interaction with the promotion. 4- Create ForceDesktopIOSPromotion switch to make it easier to test the feature. 5- Log histograms for promotion dismissal & impressions This is how the promotion look like: https://drive.google.com/a/google.com/file/d/0B9T1TKmsVG94SGRlaENRZHA5cVBJN0x... BUG=676655 Test= (1)Use cmdline flag ForceDesktopIOSPromotion to launch Chrome, (2)Use any account to signin to Chrome, (3)Go to a website with that requires login that passwords have not been saved for before, (4)Login successfully and click save when prompt to save password, (5)Promotion should appear and clicking send SMS should send SMS to the recovery phone number attached to that account. (6) Open (chrome://flags/) check Desktop to iOS promotions. Review-Url: https://codereview.chromium.org/2694893002 Cr-Commit-Position: refs/heads/master@{#451496} Committed: https://chromium.googlesource.com/chromium/src/+/4ce7ff3ecc5d5b638cfdd38b956e... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:420001) as https://chromium.googlesource.com/chromium/src/+/4ce7ff3ecc5d5b638cfdd38b956e... |