Add mozilla::services Getter for nsIServiceWorkerManager

RESOLVED FIXED in mozilla34

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nsm, Assigned: lynn_tran, Mentored)

Tracking

(Blocks 1 bug)

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=nsm.nikhil@gmail.com][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

Whiteboard: [mentor=nsm.nikhil@gmail.com][lang=c++]

Updated

5 years ago
Mentor: nsm.nikhil → ehsan
Assignee

Comment 1

5 years ago
Hi, I'm interested in this bug, can I have some more information about this bug to tackle it?

Comment 2

5 years ago
Thanks for your interest, Lynn!

In order to fix this bug, you need to add nsIServiceWorkerManager to <http://mxr.mozilla.org/mozilla-central/source/xpcom/build/ServiceList.h>.  This file defines the list of XPCOM services available from the mozilla::services namespace.  Once you have done that, you want to convert the existing consumers of this service which use do_GetService() to get access to it to use the newly added mozilla::services::ServiceWorkerManager() function instead.  Please see <http://hg.mozilla.org/mozilla-central/rev/dfe5ab77a706> which is an example of how this has been done for another XPCOM service.
Assignee: nobody → lynn_tran
Assignee

Comment 3

5 years ago
Posted patch bug-1041335-fix.patch (obsolete) — Splinter Review
Attachment #8470353 - Flags: feedback?(ehsan)
Assignee

Updated

5 years ago
Flags: needinfo?(ehsan)

Comment 4

5 years ago
Comment on attachment 8470353 [details] [diff] [review]
bug-1041335-fix.patch

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

This is a good start!  Please see my comment below.  Note that you should probably change all of the occurrences in this list: <http://mxr.mozilla.org/mozilla-central/search?string=do_GetService%28SERVICEWORKERMANAGER_CONTRACTID>

::: dom/workers/ServiceWorkerContainer.cpp
@@ -43,5 @@
>    nsCOMPtr<nsISupports> promise;
>  
> -  nsresult rv;
> -  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID, &rv);
> -  if (NS_WARN_IF(NS_FAILED(rv))) {

You should not omit the error checking paths here and in the other place below.  You should probably do something like:

  if (!swm) {
    aRv.Throw(NS_ERROR_FAILURE);
    return nullptr;
  }
Attachment #8470353 - Flags: feedback?(ehsan) → feedback+

Updated

5 years ago
Flags: needinfo?(ehsan)
Assignee

Comment 5

5 years ago
Posted patch bug-1041335-fix.patch (obsolete) — Splinter Review
Attachment #8470353 - Attachment is obsolete: true
Attachment #8470384 - Flags: feedback?(ehsan)
Assignee

Updated

5 years ago
Flags: needinfo?(ehsan)
Assignee

Comment 6

5 years ago
Some of the lines that the search result yields don't exist

Comment 7

5 years ago
Comment on attachment 8470384 [details] [diff] [review]
bug-1041335-fix.patch

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

This looks good, but I think your tree is out of date, and that is why you're not seeing the rest of the occurrences of SERVICEWORKERMANAGER_CONTRACTID.  Here's what I get on my local tree if I grep SERVICEWORKERMANAGER_CONTRACTID:

content/base/src/nsDocument.cpp:4472:    nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
content/base/src/nsDocument.cpp:8496:  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
dom/interfaces/base/nsIServiceWorkerManager.idl:50:#define SERVICEWORKERMANAGER_CONTRACTID "@mozilla.org/serviceworkers/manager;1"
dom/workers/ServiceWorkerContainer.cpp:64:  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID, &rv);
dom/workers/ServiceWorkerContainer.cpp:87:  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID, &rv);
dom/workers/ServiceWorkerContainer.cpp:141:    nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID, &rv);
dom/workers/ServiceWorkerContainer.cpp:180:  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
dom/workers/ServiceWorkerContainer.cpp:189:  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
dom/workers/ServiceWorkerContainer.cpp:215:  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID, &rv);
dom/workers/ServiceWorkerContainer.cpp:258:  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID, &rv);
dom/workers/ServiceWorkerManager.cpp:612:    do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
layout/build/nsLayoutModule.cpp:1169:  { SERVICEWORKERMANAGER_CONTRACTID, &kSERVICEWORKERMANAGER_CID },

Can you please try updating your tree and convert the rest of these?  Thanks!
Attachment #8470384 - Flags: feedback?(ehsan) → feedback+

Updated

5 years ago
Flags: needinfo?(ehsan)
Assignee

Comment 8

5 years ago
Attachment #8470384 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Flags: needinfo?(ehsan)

Comment 9

5 years ago
Comment on attachment 8474897 [details] [diff] [review]
bug-1041335-fix.patch

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

Thanks a lot, Lynn!  Your patch looks great.
Attachment #8474897 - Flags: review+

Comment 10

5 years ago
I landed the patch for you: https://hg.mozilla.org/integration/mozilla-inbound/rev/33fcf51e6d13
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/33fcf51e6d13
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.