Commit 72b58157 authored by Ayu Ishii's avatar Ayu Ishii Committed by Commit Bot

WebOTP: Mojo cleanup for SmsReceiver

This change removes the mojo `message` parameter[1] from SmsReceiver
Receive because it is no longer used since the API shape change to be
a part of credential manager.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2098499/7/third_party/blink/public/mojom/sms/sms_receiver.mojom

Bug: 1045526
Change-Id: I1c49a71ca8c07fab2dd7008d0e755cc4f7d7de4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2098499Reviewed-by: default avatarVictor Costan <pwnall@chromium.org>
Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750476}
parent b0e0f347
...@@ -47,10 +47,8 @@ SmsFetchRequestHandler::Request::~Request() { ...@@ -47,10 +47,8 @@ SmsFetchRequestHandler::Request::~Request() {
} }
void SmsFetchRequestHandler::Request::OnReceive( void SmsFetchRequestHandler::Request::OnReceive(
const std::string& one_time_code, const std::string& one_time_code) {
const std::string& sms) {
auto response = std::make_unique<chrome_browser_sharing::ResponseMessage>(); auto response = std::make_unique<chrome_browser_sharing::ResponseMessage>();
response->mutable_sms_fetch_response()->set_sms(sms);
response->mutable_sms_fetch_response()->set_one_time_code(one_time_code); response->mutable_sms_fetch_response()->set_one_time_code(one_time_code);
std::move(respond_callback_).Run(std::move(response)); std::move(respond_callback_).Run(std::move(response));
......
...@@ -42,8 +42,7 @@ class SmsFetchRequestHandler : public SharingMessageHandler { ...@@ -42,8 +42,7 @@ class SmsFetchRequestHandler : public SharingMessageHandler {
SharingMessageHandler::DoneCallback respond_callback); SharingMessageHandler::DoneCallback respond_callback);
~Request() override; ~Request() override;
void OnReceive(const std::string& one_time_code, void OnReceive(const std::string& one_time_code) override;
const std::string& sms) override;
private: private:
SmsFetchRequestHandler* handler_; SmsFetchRequestHandler* handler_;
......
...@@ -66,12 +66,10 @@ TEST(SmsFetchRequestHandlerTest, Basic) { ...@@ -66,12 +66,10 @@ TEST(SmsFetchRequestHandlerTest, Basic) {
BindLambdaForTesting([&loop](std::unique_ptr<ResponseMessage> response) { BindLambdaForTesting([&loop](std::unique_ptr<ResponseMessage> response) {
EXPECT_TRUE(response->has_sms_fetch_response()); EXPECT_TRUE(response->has_sms_fetch_response());
EXPECT_EQ("123", response->sms_fetch_response().one_time_code()); EXPECT_EQ("123", response->sms_fetch_response().one_time_code());
EXPECT_EQ("hello", response->sms_fetch_response().sms());
loop.Quit(); loop.Quit();
})); }));
subscriber->OnReceive("123", "hello"); subscriber->OnReceive("123");
loop.Run(); loop.Run();
} }
...@@ -91,7 +89,7 @@ TEST(SmsFetchRequestHandlerTest, OutOfOrder) { ...@@ -91,7 +89,7 @@ TEST(SmsFetchRequestHandlerTest, OutOfOrder) {
message, message,
BindLambdaForTesting([&loop1](std::unique_ptr<ResponseMessage> response) { BindLambdaForTesting([&loop1](std::unique_ptr<ResponseMessage> response) {
EXPECT_TRUE(response->has_sms_fetch_response()); EXPECT_TRUE(response->has_sms_fetch_response());
EXPECT_EQ("first", response->sms_fetch_response().sms()); EXPECT_EQ("1", response->sms_fetch_response().one_time_code());
loop1.Quit(); loop1.Quit();
})); }));
...@@ -104,16 +102,14 @@ TEST(SmsFetchRequestHandlerTest, OutOfOrder) { ...@@ -104,16 +102,14 @@ TEST(SmsFetchRequestHandlerTest, OutOfOrder) {
message, message,
BindLambdaForTesting([&loop2](std::unique_ptr<ResponseMessage> response) { BindLambdaForTesting([&loop2](std::unique_ptr<ResponseMessage> response) {
EXPECT_TRUE(response->has_sms_fetch_response()); EXPECT_TRUE(response->has_sms_fetch_response());
EXPECT_EQ("second", response->sms_fetch_response().sms()); EXPECT_EQ("2", response->sms_fetch_response().one_time_code());
loop2.Quit(); loop2.Quit();
})); }));
request2->OnReceive("2", "second"); request2->OnReceive("2");
loop2.Run(); loop2.Run();
request1->OnReceive("1", "first"); request1->OnReceive("1");
loop1.Run(); loop1.Run();
} }
......
...@@ -63,7 +63,8 @@ void FetchRemoteSms( ...@@ -63,7 +63,8 @@ void FetchRemoteSms(
DCHECK(response); DCHECK(response);
DCHECK(response->has_sms_fetch_response()); DCHECK(response->has_sms_fetch_response());
std::move(callback).Run(response->sms_fetch_response().sms()); std::move(callback).Run(
response->sms_fetch_response().one_time_code());
}, },
std::move(callback))); std::move(callback)));
} }
...@@ -108,7 +108,7 @@ TEST(SmsRemoteFetcherTest, OneDevice) { ...@@ -108,7 +108,7 @@ TEST(SmsRemoteFetcherTest, OneDevice) {
chrome_browser_sharing::SharingMessage message, chrome_browser_sharing::SharingMessage message,
SharingMessageSender::ResponseCallback callback) { SharingMessageSender::ResponseCallback callback) {
auto response = std::make_unique<ResponseMessage>(); auto response = std::make_unique<ResponseMessage>();
response->mutable_sms_fetch_response()->set_sms("hello"); response->mutable_sms_fetch_response()->set_one_time_code("ABC");
std::move(callback).Run(SharingSendMessageResult::kSuccessful, std::move(callback).Run(SharingSendMessageResult::kSuccessful,
std::move(response)); std::move(response));
})); }));
...@@ -117,7 +117,7 @@ TEST(SmsRemoteFetcherTest, OneDevice) { ...@@ -117,7 +117,7 @@ TEST(SmsRemoteFetcherTest, OneDevice) {
&profile, GetOriginForURL("a.com"), &profile, GetOriginForURL("a.com"),
BindLambdaForTesting([&loop](base::Optional<std::string> result) { BindLambdaForTesting([&loop](base::Optional<std::string> result) {
ASSERT_TRUE(result); ASSERT_TRUE(result);
ASSERT_EQ("hello", result); ASSERT_EQ("ABC", result);
loop.Quit(); loop.Quit();
})); }));
......
...@@ -22,8 +22,7 @@ void SmsClientImpl::Subscribe() { ...@@ -22,8 +22,7 @@ void SmsClientImpl::Subscribe() {
fetcher_->Subscribe(origin_, this); fetcher_->Subscribe(origin_, this);
} }
void SmsClientImpl::OnReceive(const std::string& one_time_code, void SmsClientImpl::OnReceive(const std::string& one_time_code) {
const std::string& sms) {
one_time_code_ = one_time_code; one_time_code_ = one_time_code;
} }
......
...@@ -31,8 +31,7 @@ class SmsClientImpl : public SmsClient, public content::SmsFetcher::Subscriber { ...@@ -31,8 +31,7 @@ class SmsClientImpl : public SmsClient, public content::SmsFetcher::Subscriber {
const std::string& GetOTP() const override; const std::string& GetOTP() const override;
// content::SmsFetcher::Subscriber // content::SmsFetcher::Subscriber
void OnReceive(const std::string& one_time_code, void OnReceive(const std::string& one_time_code) override;
const std::string& sms) override;
content::SmsFetcher* GetFetcherForTesting(); content::SmsFetcher* GetFetcherForTesting();
......
...@@ -45,8 +45,7 @@ TEST_F(SmsClientImplTest, SaveOtpOnReceive) { ...@@ -45,8 +45,7 @@ TEST_F(SmsClientImplTest, SaveOtpOnReceive) {
sms_client()->Subscribe(); sms_client()->Subscribe();
std::string otp = "123"; std::string otp = "123";
std::string sms = "For: https://www.google.com?otp=" + otp; sms_client()->OnReceive(otp);
sms_client()->OnReceive(otp, sms);
EXPECT_EQ(sms_client()->GetOTP(), otp); EXPECT_EQ(sms_client()->GetOTP(), otp);
} }
......
...@@ -154,7 +154,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, Receive) { ...@@ -154,7 +154,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, Receive) {
)"; )";
EXPECT_CALL(*mock_provider_ptr, Retrieve()).WillOnce(Invoke([&]() { EXPECT_CALL(*mock_provider_ptr, Retrieve()).WillOnce(Invoke([&]() {
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello");
ConfirmPrompt(); ConfirmPrompt();
})); }));
...@@ -204,7 +204,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, AtMostOneSmsRequestPerOrigin) { ...@@ -204,7 +204,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, AtMostOneSmsRequestPerOrigin) {
EXPECT_CALL(*mock_provider_ptr, Retrieve()) EXPECT_CALL(*mock_provider_ptr, Retrieve())
.WillOnce(Return()) .WillOnce(Return())
.WillOnce(Invoke([&]() { .WillOnce(Invoke([&]() {
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello");
ConfirmPrompt(); ConfirmPrompt();
})); }));
...@@ -284,7 +284,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, ...@@ -284,7 +284,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest,
ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName, ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName,
ukm_loop.QuitClosure()); ukm_loop.QuitClosure());
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello1", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello1");
ConfirmPrompt(); ConfirmPrompt();
ukm_loop.Run(); ukm_loop.Run();
...@@ -308,7 +308,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, ...@@ -308,7 +308,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest,
ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName, ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName,
ukm_loop.QuitClosure()); ukm_loop.QuitClosure());
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello2", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello2");
ConfirmPrompt(); ConfirmPrompt();
ukm_loop.Run(); ukm_loop.Run();
...@@ -440,7 +440,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_TwoTabsSameOrigin) { ...@@ -440,7 +440,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_TwoTabsSameOrigin) {
ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName, ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName,
ukm_loop.QuitClosure()); ukm_loop.QuitClosure());
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello1", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello1");
ConfirmPrompt(); ConfirmPrompt();
ukm_loop.Run(); ukm_loop.Run();
...@@ -464,7 +464,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_TwoTabsSameOrigin) { ...@@ -464,7 +464,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_TwoTabsSameOrigin) {
ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName, ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName,
ukm_loop.QuitClosure()); ukm_loop.QuitClosure());
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello2", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello2");
ConfirmPrompt(); ConfirmPrompt();
ukm_loop.Run(); ukm_loop.Run();
...@@ -522,7 +522,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_TwoTabsDifferentOrigin) { ...@@ -522,7 +522,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_TwoTabsDifferentOrigin) {
// capture and evaluation. // capture and evaluation.
ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName, ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName,
ukm_loop.QuitClosure()); ukm_loop.QuitClosure());
mock_provider_ptr->NotifyReceive(url::Origin::Create(url1), "hello1", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url1), "hello1");
ConfirmPrompt(); ConfirmPrompt();
ukm_loop.Run(); ukm_loop.Run();
} }
...@@ -538,7 +538,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_TwoTabsDifferentOrigin) { ...@@ -538,7 +538,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_TwoTabsDifferentOrigin) {
// capture and evaluation. // capture and evaluation.
ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName, ukm_recorder()->SetOnAddEntryCallback(Entry::kEntryName,
ukm_loop.QuitClosure()); ukm_loop.QuitClosure());
mock_provider_ptr->NotifyReceive(url::Origin::Create(url2), "hello2", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url2), "hello2");
ConfirmPrompt(); ConfirmPrompt();
ukm_loop.Run(); ukm_loop.Run();
} }
...@@ -574,7 +574,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, SmsReceivedAfterTabIsClosed) { ...@@ -574,7 +574,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, SmsReceivedAfterTabIsClosed) {
shell()->Close(); shell()->Close();
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello");
ExpectNoOutcomeUKM(); ExpectNoOutcomeUKM();
} }
...@@ -595,7 +595,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, Cancels) { ...@@ -595,7 +595,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, Cancels) {
ExpectSmsPrompt(); ExpectSmsPrompt();
EXPECT_CALL(*mock_provider_ptr, Retrieve()).WillOnce(Invoke([&]() { EXPECT_CALL(*mock_provider_ptr, Retrieve()).WillOnce(Invoke([&]() {
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello");
DismissPrompt(); DismissPrompt();
})); }));
...@@ -634,7 +634,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, AbortAfterSmsRetrieval) { ...@@ -634,7 +634,7 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, AbortAfterSmsRetrieval) {
EXPECT_CALL(*mock_provider_ptr, Retrieve()) EXPECT_CALL(*mock_provider_ptr, Retrieve())
.WillOnce(Invoke([&mock_provider_ptr, &url]() { .WillOnce(Invoke([&mock_provider_ptr, &url]() {
mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello", ""); mock_provider_ptr->NotifyReceive(url::Origin::Create(url), "hello");
})); }));
EXPECT_TRUE(ExecJs(shell(), R"( EXPECT_TRUE(ExecJs(shell(), R"(
......
...@@ -66,15 +66,14 @@ void SmsFetcherImpl::Unsubscribe(const url::Origin& origin, ...@@ -66,15 +66,14 @@ void SmsFetcherImpl::Unsubscribe(const url::Origin& origin,
} }
bool SmsFetcherImpl::Notify(const url::Origin& origin, bool SmsFetcherImpl::Notify(const url::Origin& origin,
const std::string& one_time_code, const std::string& one_time_code) {
const std::string& sms) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
auto* subscriber = subscribers_.Pop(origin); auto* subscriber = subscribers_.Pop(origin);
if (!subscriber) if (!subscriber)
return false; return false;
subscriber->OnReceive(one_time_code, sms); subscriber->OnReceive(one_time_code);
return true; return true;
} }
...@@ -89,14 +88,13 @@ void SmsFetcherImpl::OnRemote(base::Optional<std::string> sms) { ...@@ -89,14 +88,13 @@ void SmsFetcherImpl::OnRemote(base::Optional<std::string> sms) {
if (!result) if (!result)
return; return;
Notify(result->origin, result->one_time_code, *sms); Notify(result->origin, result->one_time_code);
} }
bool SmsFetcherImpl::OnReceive(const url::Origin& origin, bool SmsFetcherImpl::OnReceive(const url::Origin& origin,
const std::string& one_time_code, const std::string& one_time_code) {
const std::string& sms) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return Notify(origin, one_time_code, sms); return Notify(origin, one_time_code);
} }
bool SmsFetcherImpl::HasSubscribers() { bool SmsFetcherImpl::HasSubscribers() {
......
...@@ -40,8 +40,7 @@ class CONTENT_EXPORT SmsFetcherImpl : public content::SmsFetcher, ...@@ -40,8 +40,7 @@ class CONTENT_EXPORT SmsFetcherImpl : public content::SmsFetcher,
// content::SmsProvider::Observer: // content::SmsProvider::Observer:
bool OnReceive(const url::Origin& origin, bool OnReceive(const url::Origin& origin,
const std::string& one_time_code, const std::string& one_time_code) override;
const std::string& sms) override;
bool HasSubscribers() override; bool HasSubscribers() override;
...@@ -50,9 +49,7 @@ class CONTENT_EXPORT SmsFetcherImpl : public content::SmsFetcher, ...@@ -50,9 +49,7 @@ class CONTENT_EXPORT SmsFetcherImpl : public content::SmsFetcher,
private: private:
void OnRemote(base::Optional<std::string> sms); void OnRemote(base::Optional<std::string> sms);
bool Notify(const url::Origin& origin, bool Notify(const url::Origin& origin, const std::string& one_time_code);
const std::string& one_time_code,
const std::string& sms);
// |context_| is safe because all instances of SmsFetcherImpl are owned by // |context_| is safe because all instances of SmsFetcherImpl are owned by
// the BrowserContext itself. // the BrowserContext itself.
......
...@@ -41,8 +41,7 @@ class MockSubscriber : public SmsFetcher::Subscriber { ...@@ -41,8 +41,7 @@ class MockSubscriber : public SmsFetcher::Subscriber {
MockSubscriber() = default; MockSubscriber() = default;
~MockSubscriber() override = default; ~MockSubscriber() override = default;
MOCK_METHOD2(OnReceive, MOCK_METHOD1(OnReceive, void(const std::string& one_time_code));
void(const std::string& one_time_code, const std::string& sms));
private: private:
DISALLOW_COPY_AND_ASSIGN(MockSubscriber); DISALLOW_COPY_AND_ASSIGN(MockSubscriber);
...@@ -84,10 +83,10 @@ TEST_F(SmsFetcherImplTest, ReceiveFromLocalSmsProvider) { ...@@ -84,10 +83,10 @@ TEST_F(SmsFetcherImplTest, ReceiveFromLocalSmsProvider) {
SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider())); SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
EXPECT_CALL(*provider(), Retrieve()).WillOnce(Invoke([&]() { EXPECT_CALL(*provider(), Retrieve()).WillOnce(Invoke([&]() {
provider()->NotifyReceive(kOrigin, "123", "hello"); provider()->NotifyReceive(kOrigin, "123");
})); }));
EXPECT_CALL(subscriber, OnReceive("123", "hello")); EXPECT_CALL(subscriber, OnReceive("123"));
fetcher.Subscribe(kOrigin, &subscriber); fetcher.Subscribe(kOrigin, &subscriber);
} }
...@@ -105,7 +104,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromRemoteProvider) { ...@@ -105,7 +104,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromRemoteProvider) {
std::move(callback).Run(sms); std::move(callback).Run(sms);
})); }));
EXPECT_CALL(subscriber, OnReceive("123", sms)); EXPECT_CALL(subscriber, OnReceive("123"));
fetcher.Subscribe(url::Origin::Create(GURL("https://a.com")), &subscriber); fetcher.Subscribe(url::Origin::Create(GURL("https://a.com")), &subscriber);
} }
...@@ -121,7 +120,7 @@ TEST_F(SmsFetcherImplTest, RemoteProviderTimesOut) { ...@@ -121,7 +120,7 @@ TEST_F(SmsFetcherImplTest, RemoteProviderTimesOut) {
std::move(callback).Run(base::nullopt); std::move(callback).Run(base::nullopt);
})); }));
EXPECT_CALL(subscriber, OnReceive(_, _)).Times(0); EXPECT_CALL(subscriber, OnReceive(_)).Times(0);
fetcher.Subscribe(url::Origin::Create(GURL("https://a.com")), &subscriber); fetcher.Subscribe(url::Origin::Create(GURL("https://a.com")), &subscriber);
} }
...@@ -137,7 +136,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromOtherOrigin) { ...@@ -137,7 +136,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromOtherOrigin) {
std::move(callback).Run("@b.com #123"); std::move(callback).Run("@b.com #123");
})); }));
EXPECT_CALL(subscriber, OnReceive(_, _)).Times(0); EXPECT_CALL(subscriber, OnReceive(_)).Times(0);
fetcher.Subscribe(url::Origin::Create(GURL("https://a.com")), &subscriber); fetcher.Subscribe(url::Origin::Create(GURL("https://a.com")), &subscriber);
} }
...@@ -160,7 +159,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromBothProviders) { ...@@ -160,7 +159,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromBothProviders) {
})); }));
// Expects subscriber to be notified just once. // Expects subscriber to be notified just once.
EXPECT_CALL(subscriber, OnReceive("123", sms)); EXPECT_CALL(subscriber, OnReceive("123"));
fetcher.Subscribe(url::Origin::Create(GURL("https://a.com")), &subscriber); fetcher.Subscribe(url::Origin::Create(GURL("https://a.com")), &subscriber);
} }
...@@ -176,11 +175,11 @@ TEST_F(SmsFetcherImplTest, OneOriginTwoSubscribers) { ...@@ -176,11 +175,11 @@ TEST_F(SmsFetcherImplTest, OneOriginTwoSubscribers) {
fetcher.Subscribe(kOrigin, &subscriber1); fetcher.Subscribe(kOrigin, &subscriber1);
fetcher.Subscribe(kOrigin, &subscriber2); fetcher.Subscribe(kOrigin, &subscriber2);
EXPECT_CALL(subscriber1, OnReceive("123", "foo")); EXPECT_CALL(subscriber1, OnReceive("123"));
provider()->NotifyReceive(kOrigin, "123", "foo"); provider()->NotifyReceive(kOrigin, "123");
EXPECT_CALL(subscriber2, OnReceive("456", "bar")); EXPECT_CALL(subscriber2, OnReceive("456"));
provider()->NotifyReceive(kOrigin, "456", "bar"); provider()->NotifyReceive(kOrigin, "456");
} }
TEST_F(SmsFetcherImplTest, TwoOriginsTwoSubscribers) { TEST_F(SmsFetcherImplTest, TwoOriginsTwoSubscribers) {
...@@ -194,11 +193,11 @@ TEST_F(SmsFetcherImplTest, TwoOriginsTwoSubscribers) { ...@@ -194,11 +193,11 @@ TEST_F(SmsFetcherImplTest, TwoOriginsTwoSubscribers) {
fetcher.Subscribe(kOrigin1, &subscriber1); fetcher.Subscribe(kOrigin1, &subscriber1);
fetcher.Subscribe(kOrigin2, &subscriber2); fetcher.Subscribe(kOrigin2, &subscriber2);
EXPECT_CALL(subscriber2, OnReceive("456", "bar")); EXPECT_CALL(subscriber2, OnReceive("456"));
provider()->NotifyReceive(kOrigin2, "456", "bar"); provider()->NotifyReceive(kOrigin2, "456");
EXPECT_CALL(subscriber1, OnReceive("123", "foo")); EXPECT_CALL(subscriber1, OnReceive("123"));
provider()->NotifyReceive(kOrigin1, "123", "foo"); provider()->NotifyReceive(kOrigin1, "123");
} }
TEST_F(SmsFetcherImplTest, SubscribeIsIdempotent) { TEST_F(SmsFetcherImplTest, SubscribeIsIdempotent) {
......
...@@ -38,14 +38,13 @@ void SmsProvider::RemoveObserver(const Observer* observer) { ...@@ -38,14 +38,13 @@ void SmsProvider::RemoveObserver(const Observer* observer) {
void SmsProvider::NotifyReceive(const std::string& sms) { void SmsProvider::NotifyReceive(const std::string& sms) {
base::Optional<SmsParser::Result> result = SmsParser::Parse(sms); base::Optional<SmsParser::Result> result = SmsParser::Parse(sms);
if (result) if (result)
NotifyReceive(result->origin, result->one_time_code, sms); NotifyReceive(result->origin, result->one_time_code);
} }
void SmsProvider::NotifyReceive(const url::Origin& origin, void SmsProvider::NotifyReceive(const url::Origin& origin,
const std::string& one_time_code, const std::string& one_time_code) {
const std::string& sms) {
for (Observer& obs : observers_) { for (Observer& obs : observers_) {
bool handled = obs.OnReceive(origin, one_time_code, sms); bool handled = obs.OnReceive(origin, one_time_code);
if (handled) { if (handled) {
break; break;
} }
......
...@@ -25,11 +25,10 @@ class CONTENT_EXPORT SmsProvider { ...@@ -25,11 +25,10 @@ class CONTENT_EXPORT SmsProvider {
public: public:
class Observer : public base::CheckedObserver { class Observer : public base::CheckedObserver {
public: public:
// Receive an |sms| from an origin. Return true if the message is // Receive an |one_time_code| from an origin. Return true if the message is
// handled, which stops its propagation to other observers. // handled, which stops its propagation to other observers.
virtual bool OnReceive(const url::Origin&, virtual bool OnReceive(const url::Origin&,
const std::string& one_time_code, const std::string& one_time_code) = 0;
const std::string& sms) = 0;
}; };
SmsProvider(); SmsProvider();
...@@ -43,9 +42,7 @@ class CONTENT_EXPORT SmsProvider { ...@@ -43,9 +42,7 @@ class CONTENT_EXPORT SmsProvider {
void AddObserver(Observer*); void AddObserver(Observer*);
void RemoveObserver(const Observer*); void RemoveObserver(const Observer*);
void NotifyReceive(const url::Origin&, void NotifyReceive(const url::Origin&, const std::string& one_time_code);
const std::string& one_time_code,
const std::string& sms);
void NotifyReceive(const std::string& sms); void NotifyReceive(const std::string& sms);
bool HasObservers(); bool HasObservers();
......
...@@ -29,10 +29,8 @@ class MockObserver : public SmsProvider::Observer { ...@@ -29,10 +29,8 @@ class MockObserver : public SmsProvider::Observer {
MockObserver() = default; MockObserver() = default;
~MockObserver() override = default; ~MockObserver() override = default;
MOCK_METHOD3(OnReceive, MOCK_METHOD2(OnReceive,
bool(const Origin&, bool(const Origin&, const std::string& one_time_code));
const std::string& one_time_code,
const std::string& sms));
private: private:
DISALLOW_COPY_AND_ASSIGN(MockObserver); DISALLOW_COPY_AND_ASSIGN(MockObserver);
...@@ -84,21 +82,18 @@ class SmsProviderAndroidTest : public RenderViewHostTestHarness { ...@@ -84,21 +82,18 @@ class SmsProviderAndroidTest : public RenderViewHostTestHarness {
TEST_F(SmsProviderAndroidTest, Retrieve) { TEST_F(SmsProviderAndroidTest, Retrieve) {
std::string test_url = "https://google.com"; std::string test_url = "https://google.com";
std::string expected_sms = "Hi\n@google.com #123";
EXPECT_CALL(*observer(), EXPECT_CALL(*observer(), OnReceive(Origin::Create(GURL(test_url)), "ABC123"));
OnReceive(Origin::Create(GURL(test_url)), _, expected_sms));
provider().Retrieve(); provider().Retrieve();
TriggerSms(expected_sms); TriggerSms("Hi\n@google.com #ABC123");
} }
TEST_F(SmsProviderAndroidTest, IgnoreBadSms) { TEST_F(SmsProviderAndroidTest, IgnoreBadSms) {
std::string test_url = "https://google.com"; std::string test_url = "https://google.com";
std::string good_sms = "Hi\n@google.com #123"; std::string good_sms = "Hi\n@google.com #ABC123";
std::string bad_sms = "Hi\n@b.com"; std::string bad_sms = "Hi\n@b.com";
EXPECT_CALL(*observer(), EXPECT_CALL(*observer(), OnReceive(Origin::Create(GURL(test_url)), "ABC123"));
OnReceive(Origin::Create(GURL(test_url)), _, good_sms));
provider().Retrieve(); provider().Retrieve();
TriggerSms(bad_sms); TriggerSms(bad_sms);
...@@ -106,17 +101,15 @@ TEST_F(SmsProviderAndroidTest, IgnoreBadSms) { ...@@ -106,17 +101,15 @@ TEST_F(SmsProviderAndroidTest, IgnoreBadSms) {
} }
TEST_F(SmsProviderAndroidTest, TaskTimedOut) { TEST_F(SmsProviderAndroidTest, TaskTimedOut) {
EXPECT_CALL(*observer(), OnReceive(_, _, _)).Times(0); EXPECT_CALL(*observer(), OnReceive(_, _)).Times(0);
provider().Retrieve(); provider().Retrieve();
TriggerTimeout(); TriggerTimeout();
} }
TEST_F(SmsProviderAndroidTest, OneObserverTwoTasks) { TEST_F(SmsProviderAndroidTest, OneObserverTwoTasks) {
std::string test_url = "https://google.com"; std::string test_url = "https://google.com";
std::string expected_sms = "Hi\n@google.com #123";
EXPECT_CALL(*observer(), EXPECT_CALL(*observer(), OnReceive(Origin::Create(GURL(test_url)), "ABC123"));
OnReceive(Origin::Create(GURL(test_url)), _, expected_sms));
// Two tasks for when 1 request gets aborted but the task is still triggered. // Two tasks for when 1 request gets aborted but the task is still triggered.
provider().Retrieve(); provider().Retrieve();
...@@ -124,7 +117,7 @@ TEST_F(SmsProviderAndroidTest, OneObserverTwoTasks) { ...@@ -124,7 +117,7 @@ TEST_F(SmsProviderAndroidTest, OneObserverTwoTasks) {
// First timeout should be ignored. // First timeout should be ignored.
TriggerTimeout(); TriggerTimeout();
TriggerSms(expected_sms); TriggerSms("Hi\n@google.com #ABC123");
} }
} // namespace content } // namespace content
...@@ -45,7 +45,7 @@ SmsService::SmsService( ...@@ -45,7 +45,7 @@ SmsService::SmsService(
SmsService::~SmsService() { SmsService::~SmsService() {
if (callback_) if (callback_)
Process(SmsStatus::kTimeout, base::nullopt, base::nullopt); Process(SmsStatus::kTimeout, base::nullopt);
} }
// static // static
...@@ -69,21 +69,19 @@ void SmsService::Receive(ReceiveCallback callback) { ...@@ -69,21 +69,19 @@ void SmsService::Receive(ReceiveCallback callback) {
WebContents* web_contents = WebContents* web_contents =
content::WebContents::FromRenderFrameHost(render_frame_host()); content::WebContents::FromRenderFrameHost(render_frame_host());
if (!web_contents->GetDelegate()) { if (!web_contents->GetDelegate()) {
std::move(callback).Run(SmsStatus::kCancelled, base::nullopt, std::move(callback).Run(SmsStatus::kCancelled, base::nullopt);
base::nullopt);
return; return;
} }
if (callback_) { if (callback_) {
std::move(callback_).Run(SmsStatus::kCancelled, base::nullopt, std::move(callback_).Run(SmsStatus::kCancelled, base::nullopt);
base::nullopt);
fetcher_->Unsubscribe(origin_, this); fetcher_->Unsubscribe(origin_, this);
} }
start_time_ = base::TimeTicks::Now(); start_time_ = base::TimeTicks::Now();
callback_ = std::move(callback); callback_ = std::move(callback);
// |one_time_code_|, |sms_| and prompt are still present from the previous // |one_time_code_| and prompt are still present from the previous
// request so a new subscription is unnecessary. // request so a new subscription is unnecessary.
if (prompt_open_) { if (prompt_open_) {
// TODO(crbug.com/1024598): Add UMA histogram. // TODO(crbug.com/1024598): Add UMA histogram.
...@@ -93,18 +91,15 @@ void SmsService::Receive(ReceiveCallback callback) { ...@@ -93,18 +91,15 @@ void SmsService::Receive(ReceiveCallback callback) {
fetcher_->Subscribe(origin_, this); fetcher_->Subscribe(origin_, this);
} }
void SmsService::OnReceive(const std::string& one_time_code, void SmsService::OnReceive(const std::string& one_time_code) {
const std::string& sms) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!one_time_code_); DCHECK(!one_time_code_);
DCHECK(!sms_);
DCHECK(!start_time_.is_null()); DCHECK(!start_time_.is_null());
RecordSmsReceiveTime(base::TimeTicks::Now() - start_time_); RecordSmsReceiveTime(base::TimeTicks::Now() - start_time_);
one_time_code_ = one_time_code; one_time_code_ = one_time_code;
sms_ = sms;
receive_time_ = base::TimeTicks::Now(); receive_time_ = base::TimeTicks::Now();
OpenInfoBar(one_time_code); OpenInfoBar(one_time_code);
...@@ -112,7 +107,7 @@ void SmsService::OnReceive(const std::string& one_time_code, ...@@ -112,7 +107,7 @@ void SmsService::OnReceive(const std::string& one_time_code,
void SmsService::Abort() { void SmsService::Abort() {
DCHECK(callback_); DCHECK(callback_);
Process(SmsStatus::kAborted, base::nullopt, base::nullopt); Process(SmsStatus::kAborted, base::nullopt);
} }
void SmsService::NavigationEntryCommitted( void SmsService::NavigationEntryCommitted(
...@@ -137,7 +132,7 @@ void SmsService::OpenInfoBar(const std::string& one_time_code) { ...@@ -137,7 +132,7 @@ void SmsService::OpenInfoBar(const std::string& one_time_code) {
WebContents* web_contents = WebContents* web_contents =
content::WebContents::FromRenderFrameHost(render_frame_host()); content::WebContents::FromRenderFrameHost(render_frame_host());
if (!web_contents->GetDelegate()) { if (!web_contents->GetDelegate()) {
Process(SmsStatus::kCancelled, base::nullopt, base::nullopt); Process(SmsStatus::kCancelled, base::nullopt);
return; return;
} }
...@@ -149,11 +144,10 @@ void SmsService::OpenInfoBar(const std::string& one_time_code) { ...@@ -149,11 +144,10 @@ void SmsService::OpenInfoBar(const std::string& one_time_code) {
} }
void SmsService::Process(blink::mojom::SmsStatus status, void SmsService::Process(blink::mojom::SmsStatus status,
base::Optional<std::string> one_time_code, base::Optional<std::string> one_time_code) {
base::Optional<std::string> sms) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(callback_); DCHECK(callback_);
std::move(callback_).Run(status, one_time_code, sms); std::move(callback_).Run(status, one_time_code);
CleanUp(); CleanUp();
} }
...@@ -161,7 +155,6 @@ void SmsService::OnConfirm() { ...@@ -161,7 +155,6 @@ void SmsService::OnConfirm() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(one_time_code_); DCHECK(one_time_code_);
DCHECK(sms_);
DCHECK(!receive_time_.is_null()); DCHECK(!receive_time_.is_null());
RecordContinueOnSuccessTime(base::TimeTicks::Now() - receive_time_); RecordContinueOnSuccessTime(base::TimeTicks::Now() - receive_time_);
...@@ -172,7 +165,7 @@ void SmsService::OnConfirm() { ...@@ -172,7 +165,7 @@ void SmsService::OnConfirm() {
CleanUp(); CleanUp();
return; return;
} }
Process(SmsStatus::kSuccess, one_time_code_, sms_); Process(SmsStatus::kSuccess, one_time_code_);
} }
void SmsService::OnCancel() { void SmsService::OnCancel() {
...@@ -189,7 +182,7 @@ void SmsService::OnCancel() { ...@@ -189,7 +182,7 @@ void SmsService::OnCancel() {
CleanUp(); CleanUp();
return; return;
} }
Process(SmsStatus::kCancelled, base::nullopt, base::nullopt); Process(SmsStatus::kCancelled, base::nullopt);
} }
void SmsService::CleanUp() { void SmsService::CleanUp() {
...@@ -198,7 +191,6 @@ void SmsService::CleanUp() { ...@@ -198,7 +191,6 @@ void SmsService::CleanUp() {
// upon prompt confirmation. // upon prompt confirmation.
if (!prompt_open_) { if (!prompt_open_) {
one_time_code_.reset(); one_time_code_.reset();
sms_.reset();
receive_time_ = base::TimeTicks(); receive_time_ = base::TimeTicks();
} }
start_time_ = base::TimeTicks(); start_time_ = base::TimeTicks();
......
...@@ -54,8 +54,7 @@ class CONTENT_EXPORT SmsService ...@@ -54,8 +54,7 @@ class CONTENT_EXPORT SmsService
void Abort() override; void Abort() override;
// content::SmsQueue::Subscriber // content::SmsQueue::Subscriber
void OnReceive(const std::string& one_time_code, void OnReceive(const std::string& one_time_code) override;
const std::string& sms) override;
protected: protected:
// content::WebContentsObserver: // content::WebContentsObserver:
...@@ -65,8 +64,7 @@ class CONTENT_EXPORT SmsService ...@@ -65,8 +64,7 @@ class CONTENT_EXPORT SmsService
private: private:
void OpenInfoBar(const std::string& one_time_code); void OpenInfoBar(const std::string& one_time_code);
void Process(blink::mojom::SmsStatus, void Process(blink::mojom::SmsStatus,
base::Optional<std::string> one_time_code, base::Optional<std::string> one_time_code);
base::Optional<std::string> sms);
void CleanUp(); void CleanUp();
// Called when the user manually clicks the 'Enter code' button. // Called when the user manually clicks the 'Enter code' button.
...@@ -84,7 +82,6 @@ class CONTENT_EXPORT SmsService ...@@ -84,7 +82,6 @@ class CONTENT_EXPORT SmsService
bool prompt_open_ = false; bool prompt_open_ = false;
ReceiveCallback callback_; ReceiveCallback callback_;
base::Optional<std::string> sms_;
base::Optional<std::string> one_time_code_; base::Optional<std::string> one_time_code_;
base::TimeTicks start_time_; base::TimeTicks start_time_;
base::TimeTicks receive_time_; base::TimeTicks receive_time_;
......
This diff is collapsed.
...@@ -27,11 +27,10 @@ class SmsFetcher { ...@@ -27,11 +27,10 @@ class SmsFetcher {
class Subscriber : public base::CheckedObserver { class Subscriber : public base::CheckedObserver {
public: public:
// Receive an |sms| and a |one_time_code| from subscribed origin. The // Receive a |one_time_code| from subscribed origin. The |one_time_code|
// |one_time_code| is parsed from |sms| as an alphanumeric value which the // is parsed from |sms| as an alphanumeric value which the origin uses
// origin uses to verify the ownership of the phone number. // to verify the ownership of the phone number.
virtual void OnReceive(const std::string& one_time_code, virtual void OnReceive(const std::string& one_time_code) = 0;
const std::string& sms) = 0;
}; };
// Idempotent function that subscribes to incoming SMSes from SmsProvider. // Idempotent function that subscribes to incoming SMSes from SmsProvider.
......
...@@ -20,12 +20,11 @@ enum SmsStatus { ...@@ -20,12 +20,11 @@ enum SmsStatus {
// associated: there is an origin associated with a request that is multiplexed // associated: there is an origin associated with a request that is multiplexed
// through one instance on a storage partition. // through one instance on a storage partition.
interface SmsReceiver { interface SmsReceiver {
// TODO(crbug.com/1045526): Remove |message| once we stop using nagivator.sms.
// Retrieves the next SMS message that arrives on the phone that is addressed // Retrieves the next SMS message that arrives on the phone that is addressed
// to the caller's origin. // to the caller's origin.
// Returns the raw content of the received SMS. // Returns the raw content of the received SMS.
// |otp|, |message| is only set if status == kSuccess. // |otp|, |message| is only set if status == kSuccess.
Receive() => (SmsStatus status, string? otp, string? message); Receive() => (SmsStatus status, string? otp);
// Aborts the current retrieval process and resolves it with an // Aborts the current retrieval process and resolves it with an
// kAborted SmsStatus. // kAborted SmsStatus.
Abort(); Abort();
......
...@@ -479,8 +479,7 @@ void OnGetAssertionComplete( ...@@ -479,8 +479,7 @@ void OnGetAssertionComplete(
void OnSmsReceive(ScriptPromiseResolver* resolver, void OnSmsReceive(ScriptPromiseResolver* resolver,
base::TimeTicks start_time, base::TimeTicks start_time,
mojom::blink::SmsStatus status, mojom::blink::SmsStatus status,
const WTF::String& otp, const WTF::String& otp) {
const WTF::String& sms) {
AssertSecurityRequirementsBeforeResponse( AssertSecurityRequirementsBeforeResponse(
resolver, RequiredOriginType::kSecureAndSameWithAncestors); resolver, RequiredOriginType::kSecureAndSameWithAncestors);
auto& document = auto& document =
......
...@@ -224,7 +224,7 @@ class MockSmsReceiver { ...@@ -224,7 +224,7 @@ class MockSmsReceiver {
// Mock functions: // Mock functions:
async receive() { async receive() {
return {status: this.status_, otp: this.otp_, message: this.message_}; return {status: this.status_, otp: this.otp_};
} }
async abort() {} async abort() {}
...@@ -232,7 +232,6 @@ class MockSmsReceiver { ...@@ -232,7 +232,6 @@ class MockSmsReceiver {
// Resets state of mock SmsReceiver. // Resets state of mock SmsReceiver.
reset() { reset() {
this.otp_ = ''; this.otp_ = '';
this.message_ = '';
this.status_ = blink.mojom.SmsStatus.kTimeout; this.status_ = blink.mojom.SmsStatus.kTimeout;
} }
...@@ -240,10 +239,6 @@ class MockSmsReceiver { ...@@ -240,10 +239,6 @@ class MockSmsReceiver {
this.otp_ = otp; this.otp_ = otp;
} }
setMessage(message) {
this.message_ = message;
}
setStatus(status) { setStatus(status) {
this.status_ = status; this.status_ = status;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment