595456 - E10: Remote desktop notification prompt

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Doug Turner (:dougt)]

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

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.2

rolled in fixes

>fold SendPContentPermissionRequestConstructor() and prompt() into a single operation.

yup.. good call. small optimization.  follow up bug, okay?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 474809 [details] [diff] [review]
patch v.2

Sure, followup is fine.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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 #?

Comment 6

[Doug Turner (:dougt)]

mfinkle: 596015

Comment 7

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/df5f653ea413