Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646823002:
D2MIOS: Add plumbing to query verified number and send SMS from growth server. (Closed)
|
Created:
3 years, 11 months ago by justincohen Modified:
3 years, 10 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionD2MIOS: Add plumbing to query verified number and send SMS from growth server.
- Introduce SMSService class which communicates with the Google growth server.
- Add API to query the logged in users verified phone number and send promotional
desktop-to-mobile text message to SMS number.
A design doc is available here:
https://docs.google.com/document/d/1ah0VqnWcyvuyKZuBLASHvilyVl-GXom-LeDKmekILVQ/view
BUG=676655
Review-Url: https://codereview.chromium.org/2646823002
Cr-Commit-Position: refs/heads/master@{#447561}
Committed: https://chromium.googlesource.com/chromium/src/+/6b2002254c376c156bd5f7d49a6de9d352083dde
Patch Set 1 #
Total comments: 13
Patch Set 2 : Added comments #Patch Set 3 : Add unit tests #
Total comments: 8
Patch Set 4 : Address comments #Patch Set 5 : Address comments #Patch Set 6 : Fix linux builds #Patch Set 7 : Fix android builds #
Total comments: 3
Patch Set 8 : Correct historgram enum #
Total comments: 2
Patch Set 9 : Change to const ref std::string #Messages
Total messages: 43 (24 generated)
Description was changed from ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query verified phone number and send promotion desktop to mobile text to SMS number. BUG=676655 ========== to ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. BUG=676655 ==========
justincohen@chromium.org changed reviewers: + brettw@chromium.org, mrefaat@chromium.org, sky@chromium.org
PTAL! This class is modeled heavily on the WebHistoryService implementation for Google API communication.
justincohen@chromium.org changed reviewers: + msramek@chromium.org
msramek@ would you be able to review here given your work on WebHistoryService, which this is based on? thanks!
Hi! I think that basing this on WebHistoryService makes sense, but I would absolutely not do it via duplication; rather, we should try to inherit the Request logic, or perhaps also the whole service. As a side note, do you have some documentation I could read? I haven't heard of this feature before. https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... File chrome/browser/ui/desktop_ios_promotion/sms_service.cc (right): https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: We don't use (c) anymore. s/\(c\)//g in all files. https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:42: class RequestImpl : public DesktopIOSPromotion::SMSService::Request, WebHistoryService:Request is public. Couldn't we just inherit from it and DesktopIOSPromotion::SMSService::Request to save all this duplication? We can also reuse the *OAuthToken* histograms in WebHistoryService:Request, since we're retrieving the tokens in the same way, so the retrieval will succeed or fail under the very same conditions. https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:258: request->SetPostData("{}"); Is this a placeholder? In that case please add a TODO. Or if you only require a non-empty input, explain that in a comment. https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:285: if (value.get() && value.get()->IsType(base::Value::Type::DICTIONARY)) { This looks complex enough to deserve a test. https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... File chrome/browser/ui/desktop_ios_promotion/sms_service.h (right): https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.h:73: void AddObserver(SMSServiceObserver* observer); These public methods are neither used nor tested; therefore, as a reviewer, it is difficult to evaluate that the design is correct and that it works :) Can we plumb them to at least one callsite, test them, or remove them for now? https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.h:76: void QueryPhoneNumber(const PhoneNumberCallback& callback); Please document public methods.
Here's the internal PRD: https://docs.google.com/document/d/1IsjYNTHf-KnvTMQz1NJ4nub9C2FRBWud7S75mLIZP... I addressed the small stuff, and will work on the remaining test. Regarding the WebHistoryService::Request -- does it make sense to pull that out of WebHistoryService and have both WHS and SMSService inherit from that? Right now the WHS::Request is just a virtual class that WHS::RequestImpl implements -- which would lead to the same code duplication, no? Thanks for the review! https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... File chrome/browser/ui/desktop_ios_promotion/sms_service.cc (right): https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/01/23 21:13:56, msramek wrote: > nit: We don't use (c) anymore. s/\(c\)//g in all files. Done. https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:42: class RequestImpl : public DesktopIOSPromotion::SMSService::Request, On 2017/01/23 21:13:57, msramek wrote: > WebHistoryService:Request is public. Couldn't we just inherit from it and > DesktopIOSPromotion::SMSService::Request to save all this duplication? > > We can also reuse the *OAuthToken* histograms in WebHistoryService:Request, > since we're retrieving the tokens in the same way, so the retrieval will succeed > or fail under the very same conditions. Wouldn't we need to reimplement that virtuals? Additionally, they use different oauth tokens and consumer ids. We could pull the Request class out into a parent class, perhaps? https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:258: request->SetPostData("{}"); On 2017/01/23 21:13:56, msramek wrote: > Is this a placeholder? In that case please add a TODO. Or if you only require a > non-empty input, explain that in a comment. added a comment. https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... File chrome/browser/ui/desktop_ios_promotion/sms_service.h (right): https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.h:73: void AddObserver(SMSServiceObserver* observer); On 2017/01/23 21:13:57, msramek wrote: > These public methods are neither used nor tested; therefore, as a reviewer, it > is difficult to evaluate that the design is correct and that it works :) > > Can we plumb them to at least one callsite, test them, or remove them for now? removed, done https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.h:76: void QueryPhoneNumber(const PhoneNumberCallback& callback); On 2017/01/23 21:13:57, msramek wrote: > Please document public methods. Done.
Add unit tests. https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... File chrome/browser/ui/desktop_ios_promotion/sms_service.cc (right): https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:285: if (value.get() && value.get()->IsType(base::Value::Type::DICTIONARY)) { On 2017/01/23 21:13:56, msramek wrote: > This looks complex enough to deserve a test. Done.
LGTM with comments. Thanks for adding the test (although it doesn't seem to compile now :) ). https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... File chrome/browser/ui/desktop_ios_promotion/sms_service.cc (right): https://codereview.chromium.org/2646823002/diff/1/chrome/browser/ui/desktop_i... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:42: class RequestImpl : public DesktopIOSPromotion::SMSService::Request, On 2017/01/25 22:52:38, justincohen wrote: > On 2017/01/23 21:13:57, msramek wrote: > > WebHistoryService:Request is public. Couldn't we just inherit from it and > > DesktopIOSPromotion::SMSService::Request to save all this duplication? > > > > We can also reuse the *OAuthToken* histograms in WebHistoryService:Request, > > since we're retrieving the tokens in the same way, so the retrieval will > succeed > > or fail under the very same conditions. > > Wouldn't we need to reimplement that virtuals? > > Additionally, they use different oauth tokens and consumer ids. We could pull > the Request class out into a parent class, perhaps? Ah, right. I think it would make sense to have a separate class for this, because I think that "I need to ping that server and analyse a JSON response." is sort of a common usecase. As your CL demonstrates. More importantly, if there are security or performance fixes, both classes would benefit from it. But since this is a different codebase, it's not unreasonable that you simply implement your own class that is inspired by unrelated to WebHistoryService, so we don't need to block on that. Please just add a TODO or file a cleanup bug for this. https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/sms_service.h (right): https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/sms_service.h:28: namespace DesktopIOSPromotion { Sorry, didn't notice this before. chrome/browser/ does not use feature namespaces. https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/sms_service_factory.cc (right): https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/sms_service_factory.cc:32: Profile* profile = static_cast<Profile*>(context); nit: Profile::FromBrowserContext() https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/sms_service_unittest.cc (right): https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/sms_service_unittest.cc:16: namespace DesktopIOSPromotion { Ditto here. https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/sms_service_unittest.cc:159: base::BindBlock(^(DesktopIOSPromotion::SMSService::Request* request, base::BindBlock is .mm code though, this doesn't look compilable. Use base::Bind instead, followed by spinning a RunLoop to ensure that MimicReturnFromFetch gets executed.
justincohen@chromium.org changed reviewers: + rkaplow@chromium.org
thanks for the review. over to sky@ for OWNERS and rkaplow@ for metrics. PTAL! https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/sms_service.h (right): https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/sms_service.h:28: namespace DesktopIOSPromotion { On 2017/01/27 16:38:57, msramek wrote: > Sorry, didn't notice this before. chrome/browser/ does not use feature > namespaces. Done. https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/sms_service_factory.cc (right): https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/sms_service_factory.cc:32: Profile* profile = static_cast<Profile*>(context); On 2017/01/27 16:38:57, msramek wrote: > nit: Profile::FromBrowserContext() Done. https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/sms_service_unittest.cc (right): https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/sms_service_unittest.cc:16: namespace DesktopIOSPromotion { On 2017/01/27 16:38:58, msramek wrote: > Ditto here. Done. https://codereview.chromium.org/2646823002/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/sms_service_unittest.cc:159: base::BindBlock(^(DesktopIOSPromotion::SMSService::Request* request, On 2017/01/27 16:38:58, msramek wrote: > base::BindBlock is .mm code though, this doesn't look compilable. > > Use base::Bind instead, followed by spinning a RunLoop to ensure that > MimicReturnFromFetch gets executed. Done.
The CQ bit was checked by justincohen@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sky@chromium.org changed reviewers: - sky@chromium.org
You've got both brett and myself as reviewer. You don't need us both. I'm removing myself and letting Brett review this.
apologies, over to brettw@ for OWNERS. PTAL!
The CQ bit was checked by justincohen@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by justincohen@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/2646823002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2646823002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10083: +<histogram name="DesktopIOSPromotion.OAuthTokenResponseCode" units="code"> you need to put the correct enum here
https://codereview.chromium.org/2646823002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2646823002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10083: +<histogram name="DesktopIOSPromotion.OAuthTokenResponseCode" units="code"> On 2017/01/30 16:36:31, rkaplow wrote: > you need to put the correct enum here I see a bunch of other histogram's with units="code" and no corresponding enum. Perhaps I should switch "code" -> "CombinedHttpResponseAndNetErrorCode" ?
lgtm yes, that would be best
https://codereview.chromium.org/2646823002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2646823002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10083: +<histogram name="DesktopIOSPromotion.OAuthTokenResponseCode" units="code"> On 2017/01/30 16:36:31, rkaplow wrote: > you need to put the correct enum here Done.
This project should have a design doc and it should be sent to the chrome-design-docs list. Can you make sure that happens soon? You want to leave plenty of time to incorporate any feedback before launch. Owners LGTM https://codereview.chromium.org/2646823002/diff/140001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/sms_service.cc (right): https://codereview.chromium.org/2646823002/diff/140001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:254: void SMSService::SendSMS(std::string promo_id, Seems like "promo_id" should be a const ref.
On 2017/01/31 00:12:25, brettw (ping after 24h) wrote: > This project should have a design doc and it should be sent to the > chrome-design-docs list. Can you make sure that happens soon? You want to leave > plenty of time to incorporate any feedback before launch. > > Owners LGTM > > https://codereview.chromium.org/2646823002/diff/140001/chrome/browser/ui/desk... > File chrome/browser/ui/desktop_ios_promotion/sms_service.cc (right): > > https://codereview.chromium.org/2646823002/diff/140001/chrome/browser/ui/desk... > chrome/browser/ui/desktop_ios_promotion/sms_service.cc:254: void > SMSService::SendSMS(std::string promo_id, > Seems like "promo_id" should be a const ref. There is a design document for the project, also design doc for each section (desktop,ios,tracking) This is the one for the desktop part: https://docs.google.com/document/d/1ah0VqnWcyvuyKZuBLASHvilyVl-GXom-LeDKmekIL... - it's still in progress.
Description was changed from ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. BUG=676655 ========== to ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. A general design doc is available here: https://docs.google.com/document/d/1IsjYNTHf-KnvTMQz1NJ4nub9C2FRBWud7S75mLIZP... BUG=676655 ==========
https://codereview.chromium.org/2646823002/diff/140001/chrome/browser/ui/desk... File chrome/browser/ui/desktop_ios_promotion/sms_service.cc (right): https://codereview.chromium.org/2646823002/diff/140001/chrome/browser/ui/desk... chrome/browser/ui/desktop_ios_promotion/sms_service.cc:254: void SMSService::SendSMS(std::string promo_id, On 2017/01/31 00:12:25, brettw (ping after 24h) wrote: > Seems like "promo_id" should be a const ref. Done.
The CQ bit was checked by justincohen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, rkaplow@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2646823002/#ps160001 (title: "Change to const ref std::string")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. A general design doc is available here: https://docs.google.com/document/d/1IsjYNTHf-KnvTMQz1NJ4nub9C2FRBWud7S75mLIZP... BUG=676655 ========== to ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. A general design doc is available here: https://docs.google.com/document/d/1IsjYNTHf-KnvTMQz1NJ4nub9C2FRBWud7S75mLIZP... and a specific doc available here: https://docs.google.com/document/d/1ah0VqnWcyvuyKZuBLASHvilyVl-GXom-LeDKmekIL... BUG=676655 ==========
Description was changed from ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. A general design doc is available here: https://docs.google.com/document/d/1IsjYNTHf-KnvTMQz1NJ4nub9C2FRBWud7S75mLIZP... and a specific doc available here: https://docs.google.com/document/d/1ah0VqnWcyvuyKZuBLASHvilyVl-GXom-LeDKmekIL... BUG=676655 ========== to ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. A design doc is available here: https://docs.google.com/document/d/1ah0VqnWcyvuyKZuBLASHvilyVl-GXom-LeDKmekIL... BUG=676655 ==========
also, notified chrome-design-docs@
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485969618309580,
"parent_rev": "c0d0fe8bdee208d5254479068a1167e6dcc2e611", "commit_rev":
"6b2002254c376c156bd5f7d49a6de9d352083dde"}
Message was sent while issue was closed.
Description was changed from ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. A design doc is available here: https://docs.google.com/document/d/1ah0VqnWcyvuyKZuBLASHvilyVl-GXom-LeDKmekIL... BUG=676655 ========== to ========== D2MIOS: Add plumbing to query verified number and send SMS from growth server. - Introduce SMSService class which communicates with the Google growth server. - Add API to query the logged in users verified phone number and send promotional desktop-to-mobile text message to SMS number. A design doc is available here: https://docs.google.com/document/d/1ah0VqnWcyvuyKZuBLASHvilyVl-GXom-LeDKmekIL... BUG=676655 Review-Url: https://codereview.chromium.org/2646823002 Cr-Commit-Position: refs/heads/master@{#447561} Committed: https://chromium.googlesource.com/chromium/src/+/6b2002254c376c156bd5f7d49a6d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/6b2002254c376c156bd5f7d49a6d... |
