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

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: content/browser/push_messaging/push_messaging_message_filter.cc

Issue 938123002: Push API: Add and cleanup UMA logging for unregister/get/delivery. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@incognito
Patch Set: Update tests Created 5 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« chrome/browser/services/gcm/push_messaging_service_impl.cc ('K') | « chrome/browser/services/gcm/push_messaging_service_impl.cc ('k') | content/public/common/push_messaging_status.h » ('j') | content/public/common/push_messaging_status.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: content/browser/push_messaging/push_messaging_message_filter.cc
diff --git a/content/browser/push_messaging/push_messaging_message_filter.cc b/content/browser/push_messaging/push_messaging_message_filter.cc
index 4f9345ec8057d69e654314fd0b65e33d41841766..0ba0be20d6d86e79f9bedc3df24207f3f837166c 100644
--- a/content/browser/push_messaging/push_messaging_message_filter.cc
+++ b/content/browser/push_messaging/push_messaging_message_filter.cc
@@ -20,6 +20,7 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/push_messaging_service.h"
#include "content/public/common/child_process_host.h"
+#include "content/public/common/push_messaging_status.h"
#include "third_party/WebKit/public/platform/modules/push_messaging/WebPushPermissionStatus.h"
namespace content {
@@ -29,15 +30,27 @@ const char kPushRegistrationIdServiceWorkerKey[] = "push_registration_id";
namespace {
+// These methods are only called from IO thread, but it would be acceptable
+// (even though slightly racy) to call them from UI thread as well, see
+// https://groups.google.com/a/chromium.org/d/msg/chromium-dev/FNzZRJtN2aw/Aw0CWAXJJ1kJ
void RecordRegistrationStatus(PushRegistrationStatus status) {
- // Only called from IO thread, but would be acceptable (even though slightly
- // racy) to call from UI thread as well, see
- // https://groups.google.com/a/chromium.org/d/msg/chromium-dev/FNzZRJtN2aw/Aw0CWAXJJ1kJ
DCHECK_CURRENTLY_ON(BrowserThread::IO);
UMA_HISTOGRAM_ENUMERATION("PushMessaging.RegistrationStatus",
status,
PUSH_REGISTRATION_STATUS_LAST + 1);
}
Ilya Sherman 2015/02/19 21:01:45 nit: Please leave a blank line after this one.
johnme 2015/02/20 11:34:19 Done.
+void RecordUnregistrationStatus(PushUnregistrationStatus status) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ UMA_HISTOGRAM_ENUMERATION("PushMessaging.UnregistrationStatus",
+ status,
+ PUSH_UNREGISTRATION_STATUS_LAST + 1);
+}
Ilya Sherman 2015/02/19 21:01:45 nit: Please leave a blank line after this one.
johnme 2015/02/20 11:34:19 Done.
+void RecordGetRegistrationStatus(PushGetRegistrationStatus status) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ UMA_HISTOGRAM_ENUMERATION("PushMessaging.GetRegistrationStatus",
+ status,
+ PUSH_GETREGISTRATION_STATUS_LAST + 1);
+}
} // namespace
@@ -337,7 +350,8 @@ void PushMessagingMessageFilter::Core::RegisterOnUI(
BrowserThread::IO, FROM_HERE,
base::Bind(&PushMessagingMessageFilter::SendRegisterError,
io_parent_,
- data, PUSH_REGISTRATION_STATUS_PERMISSION_DENIED));
+ data,
+ PUSH_REGISTRATION_STATUS_INCOGNITO_SERVICE_NOT_AVAILABLE_PERMISSION_DENIED));
}
// Else leave the promise hanging forever, to simulate a user ignoring the
// infobar. TODO(johnme): Simulate the user dismissing the infobar after a
@@ -456,7 +470,7 @@ void PushMessagingMessageFilter::OnUnregister(
service_worker_context_->context()->GetLiveRegistration(
service_worker_registration_id);
if (!service_worker_registration) {
- DidUnregister(request_id, PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR);
+ DidUnregister(request_id, PUSH_UNREGISTRATION_STATUS_NO_SERVICE_WORKER);
return;
}
@@ -523,7 +537,7 @@ void PushMessagingMessageFilter::UnregisterHavingGottenSenderId(
return;
case SERVICE_WORKER_ERROR_FAILED:
DidUnregister(request_id,
- PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR);
+ PUSH_UNREGISTRATION_STATUS_STORAGE_ERROR);
return;
case SERVICE_WORKER_ERROR_ABORT:
case SERVICE_WORKER_ERROR_START_WORKER_FAILED:
@@ -538,7 +552,7 @@ void PushMessagingMessageFilter::UnregisterHavingGottenSenderId(
case SERVICE_WORKER_ERROR_STATE:
NOTREACHED() << "Got unexpected error code: " << service_worker_status
<< " " << ServiceWorkerStatusToString(service_worker_status);
- DidUnregister(request_id, PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR);
+ DidUnregister(request_id, PUSH_UNREGISTRATION_STATUS_STORAGE_ERROR);
return;
}
}
@@ -557,7 +571,8 @@ void PushMessagingMessageFilter::Core::UnregisterFromService(
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&PushMessagingMessageFilter::DidUnregister, io_parent_,
- request_id, PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR));
+ request_id,
+ PUSH_UNREGISTRATION_STATUS_SERVICE_NOT_AVAILABLE));
return;
}
@@ -576,26 +591,26 @@ void PushMessagingMessageFilter::Core::DidUnregisterFromService(
DCHECK_CURRENTLY_ON(BrowserThread::UI);
switch (unregistration_status) {
- case PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTER:
- break;
+ case PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTERED:
case PUSH_UNREGISTRATION_STATUS_SUCCESS_WILL_RETRY_NETWORK_ERROR:
- NOTREACHED();
- break;
case PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED:
+ BrowserThread::PostTask(
+ BrowserThread::IO, FROM_HERE,
+ base::Bind(&PushMessagingMessageFilter::ClearRegistrationData,
+ io_parent_, request_id, service_worker_registration_id,
+ unregistration_status));
+ break;
+ case PUSH_UNREGISTRATION_STATUS_NO_SERVICE_WORKER:
+ case PUSH_UNREGISTRATION_STATUS_SERVICE_NOT_AVAILABLE:
+ case PUSH_UNREGISTRATION_STATUS_SERVICE_ERROR:
+ case PUSH_UNREGISTRATION_STATUS_STORAGE_ERROR:
case PUSH_UNREGISTRATION_STATUS_NETWORK_ERROR:
- case PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR:
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&PushMessagingMessageFilter::DidUnregister, io_parent_,
request_id, unregistration_status));
- return;
+ break;
}
-
- BrowserThread::PostTask(
- BrowserThread::IO, FROM_HERE,
- base::Bind(&PushMessagingMessageFilter::ClearRegistrationData, io_parent_,
- request_id, service_worker_registration_id,
- unregistration_status));
}
void PushMessagingMessageFilter::ClearRegistrationData(
@@ -618,9 +633,12 @@ void PushMessagingMessageFilter::DidClearRegistrationData(
ServiceWorkerStatusCode service_worker_status) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
- DCHECK(service_worker_status == SERVICE_WORKER_OK)
- << "Got unexpected error code: " << service_worker_status
- << " " << ServiceWorkerStatusToString(service_worker_status);
+ if (service_worker_status != SERVICE_WORKER_OK &&
+ service_worker_status != SERVICE_WORKER_ERROR_NOT_FOUND) {
+ unregistration_status = PUSH_UNREGISTRATION_STATUS_STORAGE_ERROR;
+ DLOG(WARNING) << "Got unexpected error code: " << service_worker_status
+ << " " << ServiceWorkerStatusToString(service_worker_status);
+ }
DidUnregister(request_id, unregistration_status);
}
@@ -630,25 +648,31 @@ void PushMessagingMessageFilter::DidUnregister(
PushUnregistrationStatus unregistration_status) {
// Only called from IO thread, but would be safe to call from UI thread.
DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ RecordUnregistrationStatus(unregistration_status);
+ blink::WebPushError::ErrorType blinkError;
switch (unregistration_status) {
- case PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTER:
- case PUSH_UNREGISTRATION_STATUS_SUCCESS_WILL_RETRY_NETWORK_ERROR:
+ case PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTERED:
Send(new PushMessagingMsg_UnregisterSuccess(request_id, true));
return;
+ case PUSH_UNREGISTRATION_STATUS_SUCCESS_WILL_RETRY_NETWORK_ERROR:
+ NOTREACHED();
+ return;
case PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED:
Send(new PushMessagingMsg_UnregisterSuccess(request_id, false));
return;
+ case PUSH_UNREGISTRATION_STATUS_NO_SERVICE_WORKER:
+ case PUSH_UNREGISTRATION_STATUS_SERVICE_NOT_AVAILABLE:
+ case PUSH_UNREGISTRATION_STATUS_SERVICE_ERROR:
+ case PUSH_UNREGISTRATION_STATUS_STORAGE_ERROR:
+ blinkError = blink::WebPushError::ErrorTypeAbort;
+ break;
case PUSH_UNREGISTRATION_STATUS_NETWORK_ERROR:
- Send(new PushMessagingMsg_UnregisterError(
- request_id, blink::WebPushError::ErrorTypeNetwork,
- "Failed to connect to the push server."));
- return;
- case PUSH_UNREGISTRATION_STATUS_UNKNOWN_ERROR:
- Send(new PushMessagingMsg_UnregisterError(
- request_id, blink::WebPushError::ErrorTypeUnknown,
- "Unexpected error while trying to unregister from the push server."));
- return;
+ blinkError = blink::WebPushError::ErrorTypeNetwork;
+ break;
}
+ Send(new PushMessagingMsg_UnregisterError(
+ request_id, blinkError,
+ PushUnregistrationStatusToString(unregistration_status)));
}
// GetRegistration methods on both IO and UI threads, merged in order of use
@@ -672,26 +696,27 @@ void PushMessagingMessageFilter::DidGetRegistration(
const std::string& push_registration_id,
ServiceWorkerStatusCode service_worker_status) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
- PushGetRegistrationStatus get_status =
- PUSH_GETREGISTRATION_STATUS_SERVICE_WORKER_ERROR;
+ PushGetRegistrationStatus get_status;
switch (service_worker_status) {
case SERVICE_WORKER_OK:
if (push_endpoint_.is_empty()) {
// Return not found in incognito mode, so websites can't detect it.
- get_status = ui_core_->is_incognito()
- ? PUSH_GETREGISTRATION_STATUS_REGISTRATION_NOT_FOUND
- : PUSH_GETREGISTRATION_STATUS_SERVICE_WORKER_ERROR;
+ get_status =
+ ui_core_->is_incognito()
+ ? PUSH_GETREGISTRATION_STATUS_INCOGNITO_SERVICE_NOT_AVAILABLE_REGISTRATION_NOT_FOUND
+ : PUSH_GETREGISTRATION_STATUS_SERVICE_NOT_AVAILABLE;
break;
}
Send(new PushMessagingMsg_GetRegistrationSuccess(request_id,
push_endpoint_,
push_registration_id));
+ RecordGetRegistrationStatus(PUSH_GETREGISTRATION_STATUS_SUCCESS);
return;
case SERVICE_WORKER_ERROR_NOT_FOUND:
get_status = PUSH_GETREGISTRATION_STATUS_REGISTRATION_NOT_FOUND;
break;
case SERVICE_WORKER_ERROR_FAILED:
- get_status = PUSH_GETREGISTRATION_STATUS_SERVICE_WORKER_ERROR;
+ get_status = PUSH_GETREGISTRATION_STATUS_STORAGE_ERROR;
break;
case SERVICE_WORKER_ERROR_ABORT:
case SERVICE_WORKER_ERROR_START_WORKER_FAILED:
@@ -706,11 +731,11 @@ void PushMessagingMessageFilter::DidGetRegistration(
case SERVICE_WORKER_ERROR_STATE:
NOTREACHED() << "Got unexpected error code: " << service_worker_status
<< " " << ServiceWorkerStatusToString(service_worker_status);
- get_status = PUSH_GETREGISTRATION_STATUS_SERVICE_WORKER_ERROR;
+ get_status = PUSH_GETREGISTRATION_STATUS_STORAGE_ERROR;
break;
}
Send(new PushMessagingMsg_GetRegistrationError(request_id, get_status));
- // TODO(johnme): RecordGetRegistrationStatus(status); ?
+ RecordGetRegistrationStatus(get_status);
}
// GetPermission methods on both IO and UI threads, merged in order of use from
« chrome/browser/services/gcm/push_messaging_service_impl.cc ('K') | « chrome/browser/services/gcm/push_messaging_service_impl.cc ('k') | content/public/common/push_messaging_status.h » ('j') | content/public/common/push_messaging_status.h » ('J')

Powered by Google App Engine
This is Rietveld 408576698