Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(220)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months, 1 week ago by mrefaat
Modified:
7 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 78 (50 generated)
mrefaat
mpearson@ for histogram changes sky@ for all other changes.
7 months, 1 week ago (2017-02-15 22:54:28 UTC) #6
sky
Please add test coverage.
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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: > ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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; ...
7 months, 1 week 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: ...
7 months, 1 week 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 ...
7 months, 1 week 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 > ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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 ...
7 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: > ...
7 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
7 months ago (2017-02-18 22:23:35 UTC) #75
commit-bot: I haz the power
7 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b