Closed Bug 1156397 Opened 5 years ago Closed 5 years ago

Small bugs in webpush PushService

Categories

(Core :: DOM: Push Notifications, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

Attachments

(1 file, 3 obsolete files)

The line:
http://mxr.mozilla.org/mozilla-central/source/dom/push/PushService.jsm#385

does not look wright. Should be record

I will use this bug to fix if i find other problems.
Blocks: 1153499
Attached patch bug_1156397_v1.patch (obsolete) — Splinter Review
Call:
http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1543

does not work because the first argument cannot be nullptr it must be nsIDocument.

Suggestion from:
https://bugzilla.mozilla.org/show_bug.cgi?id=1038811#c64
 target->DispatchTrustedEvent(NS_LITERAL_STRING("..."));

does not work because  DOMEventTargetHelper is not thread safe....

I am not familiar with this code, I will leave the ExtendableEvent for now
Comment on attachment 8595444 [details] [diff] [review]
bug_1156397_v1.patch

Review of attachment 8595444 [details] [diff] [review]:
-----------------------------------------------------------------

nsm, can you look at the swm changes?

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ -122,5 @@
>    // of nsIServiceWorkerInfo.
>    nsIArray getAllRegistrations();
>  
>    void sendPushEvent(in ACString scope, in DOMString data);
> -  void sendPushSubscriptionChangedEvent(in ACString scope);

update the uuid of this interface.

::: dom/push/PushService.jsm
@@ +1330,2 @@
>  
> +        // TODO -- test needed.  E10s support needed.

while you're here, you can drop the "E10s support needed." of this comment.  It should work in e10s.
Attachment #8595444 - Flags: review?(nsm.nikhil)
Comment on attachment 8595444 [details] [diff] [review]
bug_1156397_v1.patch

Review of attachment 8595444 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +1536,5 @@
>    WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
>    {
>      MOZ_ASSERT(aWorkerPrivate);
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());
> +    nsCOMPtr<EventTarget> owner = do_QueryInterface(globalObj.GetAsSupports());

If the intent is to capture https://github.com/w3c/push-api/issues/132, please add a link to the issue, since the spec still keeps this a simple event. Also file a followup for implementing the part where we respect waitUntil() and put the bug number here. Thanks!

There is some duplication here, and globalObj is not used. How about:
nsRefPtr<EventTarget> target = aWorkerPrivate->GlobalScope();
and pass target to ExtendableEvent::Constructor and also use it below for DispatchDOMEvent.

@@ +1548,2 @@
>  
>      nsRefPtr<EventTarget> target = do_QueryObject(aWorkerPrivate->GlobalScope());

Not required.
Attachment #8595444 - Flags: review?(nsm.nikhil) → review+
Attached patch interdiff (obsolete) — Splinter Review
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Attachment #8596206 - Flags: review?(bobbyholley)
Comment on attachment 8596206 [details] [diff] [review]
interdiff

sorry, wrong bug.
Attachment #8596206 - Attachment is obsolete: true
Attachment #8596206 - Flags: review?(bobbyholley)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #4)
> Comment on attachment 8595444 [details] [diff] [review]
> bug_1156397_v1.patch
> 
> Review of attachment 8595444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +1536,5 @@
> >    WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
> >    {
> >      MOZ_ASSERT(aWorkerPrivate);
> > +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());
> > +    nsCOMPtr<EventTarget> owner = do_QueryInterface(globalObj.GetAsSupports());
> 
> If the intent is to capture https://github.com/w3c/push-api/issues/132,
> please add a link to the issue, since the spec still keeps this a simple
> event. Also file a followup for implementing the part where we respect
> waitUntil() and put the bug number here. Thanks!


Actually that was not my intention, I just did not know how to do it properly, because I am very new to dom code.
But I have figured it out.
Attached patch bug_1156397_v1.patch (obsolete) — Splinter Review
Attachment #8595444 - Attachment is obsolete: true
Attachment #8596658 - Flags: review?(nsm.nikhil)
Assignee: nsm.nikhil → dd.mozilla
Comment on attachment 8596658 [details] [diff] [review]
bug_1156397_v1.patch

Review of attachment 8596658 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments once you explain or remove the Throw() call.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1537,5 @@
>    {
>      MOZ_ASSERT(aWorkerPrivate);
>  
> +    WorkerGlobalScope* globalScope = aWorkerPrivate->GlobalScope();
> +    

Nit: Trailing whitespace.

@@ +1541,5 @@
> +    
> +    nsCOMPtr<nsIDOMEvent> event;
> +    nsresult rv =
> +      NS_NewDOMEvent(getter_AddRefs(event), globalScope, nullptr, nullptr);
> +    if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv)) {

@@ +1542,5 @@
> +    nsCOMPtr<nsIDOMEvent> event;
> +    nsresult rv =
> +      NS_NewDOMEvent(getter_AddRefs(event), globalScope, nullptr, nullptr);
> +    if (NS_FAILED(rv)) {
> +      Throw(aCx, rv);

See comment below about throw.

@@ +1547,5 @@
> +      return false;
> +    }
> +
> +    rv = event->InitEvent(NS_LITERAL_STRING("pushsubscriptionchange"), false, false);
> +    if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv)) {

@@ +1548,5 @@
> +    }
> +
> +    rv = event->InitEvent(NS_LITERAL_STRING("pushsubscriptionchange"), false, false);
> +    if (NS_FAILED(rv)) {
> +      Throw(aCx, rv);

Where does this Throw() function come from and what does it do? If there was a failure, just returning is fine.
Attachment #8596658 - Flags: review?(nsm.nikhil) → review+
Attachment #8596658 - Attachment is obsolete: true
Attachment #8597470 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5fa88d413c4f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.