For Desktop Notification (bug 573588), we need to remote the prompt.

Comment on attachment 474340 [details] [diff] [review] patch v.1 >diff --git a/dom/src/notification/nsDesktopNotification.cpp b/dom/src/notification/nsDesktopNotification.cpp >--- a/dom/src/notification/nsDesktopNotification.cpp >+++ b/dom/src/notification/nsDesktopNotification.cpp >@@ -166,24 +177,46 @@ nsDOMDesktopNotification::HandleAlertSer > } else if (!strcmp("alertfinished", aTopic)) { > DispatchNotificationEvent(NS_LITERAL_STRING("close")); > } > } > > NS_IMETHODIMP > nsDOMDesktopNotification::Show() > { >- nsCOMPtr<nsIRunnable> request; > if (nsContentUtils::GetBoolPref("notification.prompt.testing", PR_FALSE) && > nsContentUtils::GetBoolPref("notification.prompt.testing.allow", PR_TRUE)) { >- request = new NotificationRequestAllowEvent(this); >- } else { >- request = new nsDesktopNotificationRequest(this); >+ nsCOMPtr<nsIRunnable> request = new NotificationRequestAllowEvent(this); >+ NS_DispatchToMainThread(request); >+ return NS_OK; > } Comment plz. Took me a bit to figure out what's going on here. > >+ nsRefPtr<nsDesktopNotificationRequest> request = new nsDesktopNotificationRequest(this); >+ >+#ifdef MOZ_IPC >+ if (XRE_GetProcessType() == GeckoProcessType_Content) { >+ if (!mOwner) >+ return NS_OK; >+ When might |mOwner| be NULL? Is this effectively going to ignore the notification request? What might the implications of that be? >+ // because owner implements nsITabChild, we can assume that it is >+ // the one and only TabChild. >+ TabChild* child = GetTabChildFrom(mOwner->GetDocShell()); >+ "one and only TabChild *for this docshell*." >+ // Retain a reference so the object isn't deleted without IPDL's knowledge. >+ // Corresponding release occurs in DeallocPContentPermissionRequest. >+ request->AddRef(); >+ >+ nsCString type = NS_LITERAL_CSTRING("desktop-notification"); >+ child->SendPContentPermissionRequestConstructor(request, type, IPC::URI(mURI)); >+ >+ request->Sendprompt(); I didn't notice this in the PContentPermissionRequest patch because of the |hg mv|, but why do you have a separate prompt() message for PContentPermissionRequest? It looks to me like you can fold SendPContentPermissionRequestConstructor() and prompt() into a single operation.

rolled in fixes >fold SendPContentPermissionRequestConstructor() and prompt() into a single operation. yup.. good call. small optimization. follow up bug, okay?

Comment on attachment 474809 [details] [diff] [review] patch v.2 Sure, followup is fine.

Comment on attachment 474809 [details] [diff] [review] patch v.2 >>fold SendPContentPermissionRequestConstructor() and prompt() into a single operation. >yup.. good call. small optimization. follow up bug, okay? What is the bug #?

mfinkle: 596015