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

Issue 2694893002: Integrate SMS service with Desktop iOS promotion (Closed)

Created:
3 years, 10 months ago by mrefaat
Modified:
3 years, 10 months ago
Reviewers:
vasilii, Mark P, sky, justincohen
CC:
chromium-reviews, tfarina, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+573 lines, -183 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 2 chunks +13 lines, -2 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h View 1 2 3 4 5 6 7 8 9 1 chunk +50 lines, -4 lines 0 comments Download
M chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc View 1 2 3 4 5 6 7 8 1 chunk +107 lines, -4 lines 0 comments Download
M chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h View 1 2 3 4 5 3 chunks +30 lines, -13 lines 0 comments Download
M chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc View 1 2 3 4 5 6 7 4 chunks +59 lines, -6 lines 0 comments Download
A chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc View 1 2 3 4 5 6 7 1 chunk +140 lines, -0 lines 0 comments Download
A chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_view.h View 1 2 3 4 5 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/browser/ui/passwords/manage_passwords_bubble_model.cc View 2 3 4 5 6 7 1 chunk +0 lines, -2 lines 0 comments Download
A + chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.h View 1 2 3 4 5 6 7 8 2 chunks +20 lines, -10 lines 0 comments Download
A + chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc View 1 2 3 4 5 6 7 8 9 5 chunks +42 lines, -13 lines 0 comments Download
M chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.h View 1 2 3 4 5 1 chunk +0 lines, -37 lines 0 comments Download
M chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_view.cc View 1 2 3 4 5 1 chunk +0 lines, -81 lines 0 comments Download
M chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 5 6 7 1 chunk +25 lines, -3 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 chunks +51 lines, -4 lines 0 comments Download

Messages

Total messages: 78 (50 generated)
mrefaat
mpearson@ for histogram changes sky@ for all other changes.
3 years, 10 months ago (2017-02-15 22:54:28 UTC) #6
sky
Please add test coverage.
3 years, 10 months ago (2017-02-15 23:50:13 UTC) #7
sky
https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc#newcode20 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:20: : weak_ptr_factory_(this), Order of member initialize list should match ...
3 years, 10 months ago (2017-02-16 00:13:36 UTC) #8
mrefaat
still working in util_test & controller_test But all other ready for review https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc ...
3 years, 10 months ago (2017-02-16 21:02:40 UTC) #9
Mark P
Please include checking the histograms using about:histograms in your testing. (I found a bug with ...
3 years, 10 months ago (2017-02-16 23:18:58 UTC) #10
sky
https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc#newcode95 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:95: recovery_number_ = number; On 2017/02/16 21:02:39, mrefaat wrote: > ...
3 years, 10 months ago (2017-02-17 00:04:12 UTC) #11
mrefaat
On 2017/02/17 00:04:12, sky wrote: > https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc > File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc > (right): > > https://codereview.chromium.org/2694893002/diff/40001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc#newcode95 ...
3 years, 10 months ago (2017-02-17 00:20:35 UTC) #12
mrefaat
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc#newcode130 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 ...
3 years, 10 months ago (2017-02-17 00:21:00 UTC) #13
sky
+vasilii for changes to c/b/ui/passwords https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h#newcode19 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:19: virtual ~DesktopIOSPromotion() = default; ...
3 years, 10 months ago (2017-02-17 00:57:23 UTC) #15
mrefaat
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h#newcode19 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:19: virtual ~DesktopIOSPromotion() = default; On 2017/02/17 00:57:22, sky wrote: ...
3 years, 10 months ago (2017-02-17 04:31:50 UTC) #16
Mark P
tentative metrics lgtm with more revisions requested I'm disappointed you didn't say anything about my ...
3 years, 10 months ago (2017-02-17 06:15:28 UTC) #17
mrefaat
On 2017/02/17 06:15:28, Mark P wrote: > tentative metrics lgtm with more revisions requested > ...
3 years, 10 months ago (2017-02-17 12:52:12 UTC) #19
mrefaat
https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h#newcode40 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h:40: enum class PromotionEntryPoint { On 2017/02/17 06:15:28, Mark P ...
3 years, 10 months ago (2017-02-17 12:55:17 UTC) #22
vasilii
https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h#newcode13 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:13: // Interface for The Desktop iOS Promotion. Interface for ...
3 years, 10 months ago (2017-02-17 12:59:15 UTC) #23
mrefaat
https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h (right): https://codereview.chromium.org/2694893002/diff/100001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h#newcode13 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion.h:13: // Interface for The Desktop iOS Promotion. On 2017/02/17 ...
3 years, 10 months ago (2017-02-17 14:13:50 UTC) #25
vasilii
c/b/ui/passwords and manage_passwords_bubble_view.cc LGTM https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc#newcode35 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/passwords/manage_passwords_bubble_model.h ...
3 years, 10 months ago (2017-02-17 14:19:37 UTC) #26
mrefaat
https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2694893002/diff/160001/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc#newcode35 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 ...
3 years, 10 months ago (2017-02-17 14:44:47 UTC) #27
sky
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc#newcode35 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 ...
3 years, 10 months ago (2017-02-17 19:06:30 UTC) #29
mrefaat
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc#newcode35 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 ...
3 years, 10 months ago (2017-02-17 21:53:15 UTC) #30
sky
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc#newcode833 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 ...
3 years, 10 months ago (2017-02-17 22:49:32 UTC) #41
Mark P
histograms.xml still lgtm, one trivial comment --mark https://codereview.chromium.org/2694893002/diff/270001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2694893002/diff/270001/tools/metrics/histograms/histograms.xml#newcode10313 tools/metrics/histograms/histograms.xml:10313: + returned ...
3 years, 10 months ago (2017-02-17 23:41:20 UTC) #42
mrefaat
https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2694893002/diff/60001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc#newcode833 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 ...
3 years, 10 months ago (2017-02-18 00:03:25 UTC) #43
sky
https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc#newcode113 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/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc File ...
3 years, 10 months ago (2017-02-18 16:40:45 UTC) #64
mrefaat
https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/380001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc#newcode113 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 ...
3 years, 10 months ago (2017-02-18 17:33:14 UTC) #66
sky
LGTM with the following updated. https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc#newcode108 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:108: if (success) { Generally ...
3 years, 10 months ago (2017-02-18 18:03:51 UTC) #67
mrefaat
https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc (right): https://codereview.chromium.org/2694893002/diff/400001/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc#newcode108 chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:108: if (success) { On 2017/02/18 18:03:51, sky wrote: > ...
3 years, 10 months ago (2017-02-18 19:12:00 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2694893002/420001
3 years, 10 months ago (2017-02-18 22:23:35 UTC) #75
commit-bot: I haz the power
3 years, 10 months ago (2017-02-18 22:28:35 UTC) #78
Message was sent while issue was closed.
Committed patchset #10 (id:420001) as
https://chromium.googlesource.com/chromium/src/+/4ce7ff3ecc5d5b638cfdd38b956e...

Powered by Google App Engine
This is Rietveld 408576698