Skip to content

Commit

Permalink
[GCM] Propagating SendError details all the way to the event.
Browse files Browse the repository at this point in the history 
OnSendError allows to specify more message content than simply and error message
and message ID, but it currently is ignored. This patch fixes that problem by
introducing SendErrorDetails structure and updating the whole stack to handle it
properly.

BUG=350026

Review URL: https://codereview.chromium.org/191443002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@256187 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
fgorski@chromium.org committed Mar 11, 2014
1 parent 4c062e6 commit c6fe36b
Show file tree
Hide file tree
Showing 15 changed files with 229 additions and 131 deletions.
11 changes: 6 additions & 5 deletions chrome/browser/extensions/api/gcm/gcm_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,13 @@ void GcmJsEventRouter::OnMessagesDeleted(const std::string& app_id) {
app_id, event.Pass());
}

void GcmJsEventRouter::OnSendError(const std::string& app_id,
const std::string& message_id,
gcm::GCMClient::Result result) {
void GcmJsEventRouter::OnSendError(
const std::string& app_id,
const gcm::GCMClient::SendErrorDetails& send_error_details) {
api::gcm::OnSendError::Error error;
error.message_id.reset(new std::string(message_id));
error.error_message = GcmResultToError(result);
error.message_id.reset(new std::string(send_error_details.message_id));
error.error_message = GcmResultToError(send_error_details.result);
error.details.additional_properties = send_error_details.additional_data;

scoped_ptr<Event> event(new Event(
api::gcm::OnSendError::kEventName,
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/extensions/api/gcm/gcm_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ class GcmJsEventRouter : public gcm::GCMEventRouter,
const std::string& app_id,
const gcm::GCMClient::IncomingMessage& message) OVERRIDE;
virtual void OnMessagesDeleted(const std::string& app_id) OVERRIDE;
virtual void OnSendError(const std::string& app_id,
const std::string& message_id,
gcm::GCMClient::Result result) OVERRIDE;
virtual void OnSendError(
const std::string& app_id,
const gcm::GCMClient::SendErrorDetails& send_error_details) OVERRIDE;

// EventRouter::Observer:
virtual void OnListenerAdded(const EventListenerInfo& details) OVERRIDE;
Expand Down
62 changes: 52 additions & 10 deletions chrome/browser/extensions/api/gcm/gcm_apitest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,37 @@ namespace {

const char kEventsExtension[] = "gcm/events";

gcm::GCMClient::SendErrorDetails CreateErrorDetails(
const std::string& message_id,
const gcm::GCMClient::Result result,
const std::string& total_messages) {
gcm::GCMClient::SendErrorDetails error;
error.message_id = message_id;
error.result = result;
error.additional_data["expectedMessageId"] = message_id;
switch (result) {
case gcm::GCMClient::ASYNC_OPERATION_PENDING:
error.additional_data["expectedErrorMessage"] =
"Asynchronous operation is pending.";
break;
case gcm::GCMClient::SERVER_ERROR:
error.additional_data["expectedErrorMessage"] = "Server error occurred.";
break;
case gcm::GCMClient::NETWORK_ERROR:
error.additional_data["expectedErrorMessage"] = "Network error occurred.";
break;
case gcm::GCMClient::TTL_EXCEEDED:
error.additional_data["expectedErrorMessage"] = "Time-to-live exceeded.";
break;
case gcm::GCMClient::UNKNOWN_ERROR:
default: // Default case is the same as UNKNOWN_ERROR
error.additional_data["expectedErrorMessage"] = "Unknown error occurred.";
break;
}
error.additional_data["totalMessages"] = total_messages;
return error;
}

} // namespace

namespace extensions {
Expand Down Expand Up @@ -181,17 +212,28 @@ IN_PROC_BROWSER_TEST_F(GcmApiTest, OnSendError) {
LoadTestExtension(kEventsExtension, "on_send_error.html");
ASSERT_TRUE(extension);

std::string total_expected_messages = "5";
GcmJsEventRouter router(profile());
router.OnSendError(extension->id(), "error_message_1",
gcm::GCMClient::ASYNC_OPERATION_PENDING);
router.OnSendError(extension->id(), "error_message_2",
gcm::GCMClient::SERVER_ERROR);
router.OnSendError(extension->id(), "error_message_3",
gcm::GCMClient::NETWORK_ERROR);
router.OnSendError(extension->id(), "error_message_4",
gcm::GCMClient::UNKNOWN_ERROR);
router.OnSendError(extension->id(), "error_message_5",
gcm::GCMClient::TTL_EXCEEDED);
router.OnSendError(extension->id(),
CreateErrorDetails("error_message_1",
gcm::GCMClient::ASYNC_OPERATION_PENDING,
total_expected_messages));
router.OnSendError(extension->id(),
CreateErrorDetails("error_message_2",
gcm::GCMClient::SERVER_ERROR,
total_expected_messages));
router.OnSendError(extension->id(),
CreateErrorDetails("error_message_3",
gcm::GCMClient::NETWORK_ERROR,
total_expected_messages));
router.OnSendError(extension->id(),
CreateErrorDetails("error_message_4",
gcm::GCMClient::UNKNOWN_ERROR,
total_expected_messages));
router.OnSendError(extension->id(),
CreateErrorDetails("error_message_5",
gcm::GCMClient::TTL_EXCEEDED,
total_expected_messages));

EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
}
Expand Down
21 changes: 13 additions & 8 deletions chrome/browser/services/gcm/gcm_client_mock.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ void GCMClientMock::Send(const std::string& app_id,
base::Bind(&GCMClientMock::SendFinished,
weak_ptr_factory_.GetWeakPtr(),
app_id,
message.id));
message));
}

GCMClient::GCMStatistics GCMClientMock::GetStatistics() const {
Expand Down Expand Up @@ -165,17 +165,21 @@ void GCMClientMock::RegisterFinished(const std::string& app_id,
}

void GCMClientMock::SendFinished(const std::string& app_id,
const std::string& message_id) {
delegate_->OnSendFinished(app_id, message_id, SUCCESS);
const OutgoingMessage& message) {
delegate_->OnSendFinished(app_id, message.id, SUCCESS);

// Simulate send error if message id contains a hint.
if (message_id.find("error") != std::string::npos) {
if (message.id.find("error") != std::string::npos) {
SendErrorDetails send_error_details;
send_error_details.message_id = message.id;
send_error_details.result = NETWORK_ERROR;
send_error_details.additional_data = message.data;
base::MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::Bind(&GCMClientMock::MessageSendError,
weak_ptr_factory_.GetWeakPtr(),
app_id,
message_id),
send_error_details),
base::TimeDelta::FromMilliseconds(200));
}
}
Expand All @@ -191,10 +195,11 @@ void GCMClientMock::MessagesDeleted(const std::string& app_id) {
delegate_->OnMessagesDeleted(app_id);
}

void GCMClientMock::MessageSendError(const std::string& app_id,
const std::string& message_id) {
void GCMClientMock::MessageSendError(
const std::string& app_id,
const GCMClient::SendErrorDetails& send_error_details) {
if (delegate_)
delegate_->OnMessageSendError(app_id, message_id, NETWORK_ERROR);
delegate_->OnMessageSendError(app_id, send_error_details);
}

} // namespace gcm
4 changes: 2 additions & 2 deletions chrome/browser/services/gcm/gcm_client_mock.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,12 @@ class GCMClientMock : public GCMClient {
void CheckinFinished();
void RegisterFinished(const std::string& app_id,
const std::string& registrion_id);
void SendFinished(const std::string& app_id, const std::string& message_id);
void SendFinished(const std::string& app_id, const OutgoingMessage& message);
void MessageReceived(const std::string& app_id,
const IncomingMessage& message);
void MessagesDeleted(const std::string& app_id);
void MessageSendError(const std::string& app_id,
const std::string& message_id);
const SendErrorDetails& send_error_details);

Delegate* delegate_;
Status status_;
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/services/gcm/gcm_event_router.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ class GCMEventRouter {
virtual void OnMessage(const std::string& app_id,
const GCMClient::IncomingMessage& message) = 0;
virtual void OnMessagesDeleted(const std::string& app_id) = 0;
virtual void OnSendError(const std::string& app_id,
const std::string& message_id,
GCMClient::Result result) = 0;
virtual void OnSendError(
const std::string& app_id,
const GCMClient::SendErrorDetails& send_error_details) = 0;
};

} // namespace gcm
Expand Down
20 changes: 9 additions & 11 deletions chrome/browser/services/gcm/gcm_profile_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,9 @@ class GCMProfileService::IOWorker
const std::string& app_id,
const GCMClient::IncomingMessage& message) OVERRIDE;
virtual void OnMessagesDeleted(const std::string& app_id) OVERRIDE;
virtual void OnMessageSendError(const std::string& app_id,
const std::string& message_id,
GCMClient::Result result) OVERRIDE;
virtual void OnMessageSendError(
const std::string& app_id,
const GCMClient::SendErrorDetails& send_error_details) OVERRIDE;
virtual void OnGCMReady() OVERRIDE;

// Called on IO thread.
Expand Down Expand Up @@ -398,8 +398,7 @@ void GCMProfileService::IOWorker::OnMessagesDeleted(const std::string& app_id) {

void GCMProfileService::IOWorker::OnMessageSendError(
const std::string& app_id,
const std::string& message_id,
GCMClient::Result result) {
const GCMClient::SendErrorDetails& send_error_details) {
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));

content::BrowserThread::PostTask(
Expand All @@ -408,8 +407,7 @@ void GCMProfileService::IOWorker::OnMessageSendError(
base::Bind(&GCMProfileService::MessageSendError,
service_,
app_id,
message_id,
result));
send_error_details));
}

void GCMProfileService::IOWorker::OnGCMReady() {
Expand Down Expand Up @@ -1026,16 +1024,16 @@ void GCMProfileService::MessagesDeleted(const std::string& app_id) {
GetEventRouter(app_id)->OnMessagesDeleted(app_id);
}

void GCMProfileService::MessageSendError(const std::string& app_id,
const std::string& message_id,
GCMClient::Result result) {
void GCMProfileService::MessageSendError(
const std::string& app_id,
const GCMClient::SendErrorDetails& send_error_details) {
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

// Drop the event if signed out.
if (username_.empty())
return;

GetEventRouter(app_id)->OnSendError(app_id, message_id, result);
GetEventRouter(app_id)->OnSendError(app_id, send_error_details);
}

void GCMProfileService::GCMClientReady() {
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/services/gcm/gcm_profile_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,7 @@ class GCMProfileService : public BrowserContextKeyedService,
GCMClient::IncomingMessage message);
void MessagesDeleted(const std::string& app_id);
void MessageSendError(const std::string& app_id,
const std::string& message_id,
GCMClient::Result result);
const GCMClient::SendErrorDetails& send_error_details);
void GCMClientReady();

// Returns the event router to fire the event for the given app.
Expand Down
47 changes: 27 additions & 20 deletions chrome/browser/services/gcm/gcm_profile_service_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,7 @@ class FakeGCMEventRouter : public GCMEventRouter {
};

explicit FakeGCMEventRouter(Waiter* waiter)
: waiter_(waiter),
received_event_(NO_EVENT),
send_error_result_(GCMClient::SUCCESS) {
}
: waiter_(waiter), received_event_(NO_EVENT) {}

virtual ~FakeGCMEventRouter() {
}
Expand All @@ -196,38 +193,45 @@ class FakeGCMEventRouter : public GCMEventRouter {
waiter_->SignalCompleted();
}

virtual void OnSendError(const std::string& app_id,
const std::string& message_id,
GCMClient::Result result) OVERRIDE {
virtual void OnSendError(
const std::string& app_id,
const GCMClient::SendErrorDetails& send_error_details) OVERRIDE {
clear_results();
received_event_ = SEND_ERROR_EVENT;
app_id_ = app_id;
send_error_message_id_ = message_id;
send_error_result_ = result;
send_error_details_ = send_error_details;
waiter_->SignalCompleted();
}

Event received_event() const { return received_event_; }
const std::string& app_id() const { return app_id_; }
const GCMClient::IncomingMessage& message() const { return message_; }
const std::string& send_message_id() const { return send_error_message_id_; }
GCMClient::Result send_result() const { return send_error_result_; }
const GCMClient::SendErrorDetails& send_error_details() const {
return send_error_details_;
}
const std::string& send_error_message_id() const {
return send_error_details_.message_id;
}
GCMClient::Result send_error_result() const {
return send_error_details_.result;
}
const GCMClient::MessageData& send_error_data() const {
return send_error_details_.additional_data;
}

void clear_results() {
received_event_ = NO_EVENT;
app_id_.clear();
message_.data.clear();
send_error_message_id_.clear();
send_error_result_ = GCMClient::UNKNOWN_ERROR;
send_error_details_ = GCMClient::SendErrorDetails();
}

private:
Waiter* waiter_;
Event received_event_;
std::string app_id_;
GCMClient::IncomingMessage message_;
std::string send_error_message_id_;
GCMClient::Result send_error_result_;
GCMClient::SendErrorDetails send_error_details_;
};

class FakeGCMClientFactory : public GCMClientFactory {
Expand Down Expand Up @@ -1076,9 +1080,10 @@ TEST_F(GCMProfileServiceSingleProfileTest, SendError) {
consumer()->gcm_event_router()->received_event());
EXPECT_EQ(kTestingAppId, consumer()->gcm_event_router()->app_id());
EXPECT_EQ(consumer()->send_message_id(),
consumer()->gcm_event_router()->send_message_id());
consumer()->gcm_event_router()->send_error_message_id());
EXPECT_NE(GCMClient::SUCCESS,
consumer()->gcm_event_router()->send_result());
consumer()->gcm_event_router()->send_error_result());
EXPECT_EQ(message.data, consumer()->gcm_event_router()->send_error_data());
}

TEST_F(GCMProfileServiceSingleProfileTest, MessageReceived) {
Expand Down Expand Up @@ -1488,10 +1493,12 @@ TEST_F(GCMProfileServiceMultiProfileTest, Combined) {
EXPECT_EQ(FakeGCMEventRouter::SEND_ERROR_EVENT,
consumer2()->gcm_event_router()->received_event());
EXPECT_EQ(kTestingAppId, consumer2()->gcm_event_router()->app_id());
EXPECT_EQ(consumer2()->send_message_id(),
consumer2()->gcm_event_router()->send_message_id());
EXPECT_EQ(out_message4.id,
consumer2()->gcm_event_router()->send_error_message_id());
EXPECT_NE(GCMClient::SUCCESS,
consumer2()->gcm_event_router()->send_result());
consumer2()->gcm_event_router()->send_error_result());
EXPECT_EQ(out_message4.data,
consumer2()->gcm_event_router()->send_error_data());

// Sign in with a different user.
consumer()->SignIn(kTestingUsername3);
Expand Down
32 changes: 15 additions & 17 deletions chrome/test/data/extensions/api_test/gcm/events/on_send_error.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,24 @@
onload = function() {
chrome.test.runTests([
function onSendError() {
var errorMessages = [
'Asynchronous operation is pending.',
'Server error occurred.',
'Network error occurred.',
'Unknown error occurred.',
'Time-to-live exceeded.'
];
var messageIds = [
'error_message_1',
'error_message_2',
'error_message_3',
'error_message_4',
'error_message_5'
];
var currentError = 0;
var totalMessages = 0;
var eventHandler = function(error) {
chrome.test.assertEq(errorMessages[currentError], error.errorMessage);
chrome.test.assertEq(messageIds[currentError], error.messageId);
chrome.test.assertEq(3, Object.keys(error.details).length);
chrome.test.assertTrue(
error.details.hasOwnProperty("expectedMessageId"));
chrome.test.assertTrue(
error.details.hasOwnProperty("expectedErrorMessage"));
chrome.test.assertEq(error.details.expectedMessageId, error.messageId);
chrome.test.assertEq(error.details.expectedErrorMessage,
error.errorMessage);
currentError += 1;
if (currentError == messageIds.length) {
var tempTotalMessages = +error.details.totalMessages;
if (totalMessages == 0)
totalMessages = tempTotalMessages;
else
chrome.test.assertEq(totalMessages, tempTotalMessages);
if (currentError == totalMessages) {
chrome.gcm.onSendError.removeListener(eventHandler);
chrome.test.succeed();
}
Expand Down
Loading

0 comments on commit c6fe36b

Please sign in to comment.