E10: Remote desktop notification prompt

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
For Desktop Notification (bug 573588), we need to remote the prompt.
(Assignee)

Comment 1

8 years ago
Created attachment 474340 [details] [diff] [review]
patch v.1
Assignee: nobody → doug.turner
Attachment #474340 - Flags: review?(jones.chris.g)
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.
(Assignee)

Comment 3

8 years ago
Created attachment 474809 [details] [diff] [review]
patch v.2

rolled in fixes

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

yup.. good call. small optimization.  follow up bug, okay?
Attachment #474340 - Attachment is obsolete: true
Attachment #474340 - Flags: review?(jones.chris.g)
(Assignee)

Updated

8 years ago
Attachment #474809 - Flags: review?(jones.chris.g)
Comment on attachment 474809 [details] [diff] [review]
patch v.2

Sure, followup is fine.
Attachment #474809 - Flags: review?(jones.chris.g) → review+
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 #?
Attachment #474809 - Flags: approval2.0+
(Assignee)

Comment 6

8 years ago
mfinkle: 596015
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.