Closed
Bug 1436812
Opened 6 years ago
Closed 6 years ago
Split ServiceWorkerContainer into an inner and outer object
Categories
(Core :: DOM: Service Workers, enhancement, P2)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(9 files, 38 obsolete files)
4.85 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
P2 Don't try to use the document's nsILoadGroup when registering a new service worker script. r=baku
1.75 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
3.82 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
5.55 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
10.11 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
4.43 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
15.29 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
5.62 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
7.50 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
Previous bugs have split ServiceWorker and ServiceWorkerRegistration into inner/outer objects. The next step is to split ServiceWorkerContainer into inner/outer objects as well.
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
I think the trickiest part of this bug will be the ready promise. The spec has some weirdness: https://github.com/w3c/ServiceWorker/issues/1278 And our implementation mixes ServiceWorkerContainer/ServiceWorkerManager/nsPIDOMWindowInner together. I'll need to disentangle all that to make a clean interface that can be remoted.
Assignee | ||
Comment 4•6 years ago
|
||
More ready promise weirdness: https://github.com/w3c/ServiceWorker/issues/1279
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8949864 -
Attachment is obsolete: true
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #8949882 -
Attachment is obsolete: true
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #8950397 -
Attachment is obsolete: true
Assignee | ||
Comment 9•6 years ago
|
||
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88db04557e3ac81a6bb7b592e7e2d369a64a3eb8
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #8960298 -
Attachment is obsolete: true
Assignee | ||
Comment 12•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9cda798dd153a0c19b82338d1daf1c9157bff4b
Attachment #8960299 -
Attachment is obsolete: true
Assignee | ||
Comment 13•6 years ago
|
||
Since I keep getting interrupted by stuff at home I'm going to split this bug up a bit so I can land it in smaller chunks. These 4 patches look good, so I'll move them to a new bug and ask for review there.
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 8960239 [details] [diff] [review] P1 Add ServiceWorkerContainer::Create() factory method. r=asuth This patch is just a small refactor to change ServiceWorkerContainer to use a factory method for creation. We will use this in the future to select the kind of inner object (same-process vs remote) based on a pref.
Attachment #8960239 -
Flags: review?(bugmail)
Assignee | ||
Comment 15•6 years ago
|
||
Comment on attachment 8960241 [details] [diff] [review] P2 Eagerly create the ServiceWorkerContainer.controller. r=asuth This patch makes the container eagerly allocate the controller ServiceWorker binding object. This is necessary for a few reasons: 1. We need it available synchonously for the .controller getter. 2. Once we are remote we cannot just ask the SWM for it on demand. 3. We want to make sure the binding object gets updates if the state of the ServiceWorker changes, etc. The easiest way to do this is to create the binding object and let it request these updates from the parent process.
Attachment #8960241 -
Flags: review?(bugmail)
Assignee | ||
Comment 16•6 years ago
|
||
Whoops... I am flagging for review on the wrong bug. Sorry.
Assignee | ||
Updated•6 years ago
|
Attachment #8960239 -
Attachment is obsolete: true
Attachment #8960239 -
Flags: review?(bugmail)
Assignee | ||
Updated•6 years ago
|
Attachment #8960241 -
Attachment is obsolete: true
Attachment #8960241 -
Flags: review?(bugmail)
Assignee | ||
Updated•6 years ago
|
Attachment #8961156 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8961157 -
Attachment is obsolete: true
Assignee | ||
Comment 17•6 years ago
|
||
Assignee | ||
Comment 18•6 years ago
|
||
Attachment #8967487 -
Attachment is obsolete: true
Assignee | ||
Comment 19•6 years ago
|
||
Assignee | ||
Comment 20•6 years ago
|
||
This currently causes a crash in detached-context.https.html. I'll have to investigate.
Assignee | ||
Comment 21•6 years ago
|
||
Attachment #8967498 -
Attachment is obsolete: true
Assignee | ||
Comment 22•6 years ago
|
||
Attachment #8967544 -
Attachment is obsolete: true
Assignee | ||
Comment 23•6 years ago
|
||
Attachment #8968076 -
Attachment is obsolete: true
Assignee | ||
Comment 24•6 years ago
|
||
Assignee | ||
Comment 25•6 years ago
|
||
Assignee | ||
Comment 26•6 years ago
|
||
Attachment #8967865 -
Attachment is obsolete: true
Assignee | ||
Comment 27•6 years ago
|
||
Attachment #8968246 -
Attachment is obsolete: true
Assignee | ||
Comment 28•6 years ago
|
||
Attachment #8968247 -
Attachment is obsolete: true
Assignee | ||
Comment 29•6 years ago
|
||
Attachment #8968248 -
Attachment is obsolete: true
Assignee | ||
Comment 30•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b975831e109085150fdb0d4867523b59b16ae959
Assignee | ||
Comment 31•6 years ago
|
||
I still need to do the register() implementation, but I can start requesting/landing the first patches here. Going to flag for review since try is green.
Assignee | ||
Comment 32•6 years ago
|
||
Comment on attachment 8967497 [details] [diff] [review] P1 Add a ServiceWorkerContainer::Inner scaffold. r=baku Andrea, this patch just stubs out an "inner" class for ServiceWorkerContainer. The overall goal of this bug is to migrate all the ServiceWorkerContainer methods over to an implementation that calls through the inner class. Then we can swap out the inner implementation in a later bug to remote to the parent process. Note, since we want to be able to remote to another process we need to make sure the inner class neither takes arguments or returns values directly tied to the binding layer. So part of this bug will be switching SWM over to use MozPromise internally instead of Promise for these operations, etc.
Attachment #8967497 -
Flags: review?(amarchesini)
Assignee | ||
Comment 33•6 years ago
|
||
Comment on attachment 8968249 [details] [diff] [review] P2 Implement ServiceWorkerContainer::GetReady() via the Inner class. r=baku This implements the GetReady() method using the inner class. This is pretty simple since I already refactored SWM to use a MozPromise for the ready stuff.
Attachment #8968249 -
Flags: review?(amarchesini)
Assignee | ||
Comment 34•6 years ago
|
||
Comment on attachment 8968250 [details] [diff] [review] P3 Implement ServiceWorkerContainer::GetRegistration() via the inner clas. r=baku This migrates getRegistration() over to an inner using a MozPromise.
Attachment #8968250 -
Flags: review?(amarchesini)
Assignee | ||
Comment 35•6 years ago
|
||
Comment on attachment 8968251 [details] [diff] [review] P4 Factor out a method for checking global validity in ServiceWorkerContainer to reduce code duplication. r=baku I realized that a lot of this code will have to verify that storage access is allowed for the window. I decided to move this into a helper method that could be re-used in ServiceWorkerContainer.
Attachment #8968251 -
Flags: review?(amarchesini)
Assignee | ||
Comment 36•6 years ago
|
||
Comment on attachment 8968252 [details] [diff] [review] P5 Implement ServiceWorkerContainer::GetRegistrations() via the inner class. r=baku This implements getRegistrations() using the inner class.
Attachment #8968252 -
Flags: review?(amarchesini)
Assignee | ||
Comment 37•6 years ago
|
||
This is my work-in-progress on moving register(). Its taking a bit longer because I have to disentangle some tight coupling between the container and SWM.
Comment 38•6 years ago
|
||
Comment on attachment 8967497 [details] [diff] [review] P1 Add a ServiceWorkerContainer::Inner scaffold. r=baku Review of attachment 8967497 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/serviceworkers/ServiceWorkerContainer.cpp @@ +62,5 @@ > ServiceWorkerContainer::Create(nsIGlobalObject* aGlobal) > { > + RefPtr<Inner> inner = new ServiceWorkerContainerImpl(); > + RefPtr<ServiceWorkerContainer> ref = > + new ServiceWorkerContainer(aGlobal, inner); inner.forget() @@ +67,5 @@ > return ref.forget(); > } > > +ServiceWorkerContainer::ServiceWorkerContainer(nsIGlobalObject* aGlobal, > + ServiceWorkerContainer::Inner* aInner) already_AddRefed<ServiceWorkerContainer::Inner> aInner @@ +72,2 @@ > : DOMEventTargetHelper(aGlobal) > + , mInner(aInner) Move() ::: dom/serviceworkers/ServiceWorkerContainerImpl.cpp @@ +8,5 @@ > + > +namespace mozilla { > +namespace dom { > + > +ServiceWorkerContainerImpl::~ServiceWorkerContainerImpl() = default; but I guess it will change in the next patches.
Attachment #8967497 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8968249 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8968250 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8968251 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8968252 -
Flags: review?(amarchesini) → review+
Assignee | ||
Updated•6 years ago
|
Attachment #8967497 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8968249 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8968250 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8968251 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8968252 -
Attachment is obsolete: true
Assignee | ||
Comment 39•6 years ago
|
||
This passes mochitests, but still fails some WPT tests: /service-workers/service-worker/registration-security-error.https.html FAIL Registering script outside domain - assert_throws: Registration script outside domain should fail with SecurityError. function "function() { throw e }" threw object "[Exception... "<no message>" nsresult: "0x805e0006 (<unknown>)" location: "JS frame :: https://web-platform.test:8443/service-workers/service-worker/registration-security-error.https.html :: <TOP_LEVEL> :: line 8" data: no]" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18 registration_tests_security_error/<@https://web-platform.test:8443/service-workers/service-worker/resources/registration-tests-security-error.js:35:14 Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1501:20 promise_test/tests.promise_tests<@https://web-platform.test:8443/resources/testharness.js:543:27 promise callback*promise_test@https://web-platform.test:8443/resources/testharness.js:539:31 registration_tests_security_error@https://web-platform.test:8443/service-workers/service-worker/resources/registration-tests-security-error.js:32:3 @https://web-platform.test:8443/service-workers/service-worker/registration-security-error.https.html:8:1 FAIL Script URL is same-origin filesystem: URL - assert_throws: Registering a script which has same-origin filesystem: URL should fail with SecurityError. function "function() { throw e }" threw object "[Exception... "<no message>" nsresult: "0x805e0006 (<unknown>)" location: "JS frame :: https://web-platform.test:8443/service-workers/service-worker/registration-security-error.https.html :: <TOP_LEVEL> :: line 8" data: no]" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18 registration_tests_security_error/<@https://web-platform.test:8443/service-workers/service-worker/resources/registration-tests-security-error.js:72:14 Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1501:20 promise_test/tests.promise_tests<@https://web-platform.test:8443/resources/testharness.js:543:27 promise callback*promise_test@https://web-platform.test:8443/resources/testharness.js:539:31 registration_tests_security_error@https://web-platform.test:8443/service-workers/service-worker/resources/registration-tests-security-error.js:69:3 @https://web-platform.test:8443/service-workers/service-worker/registration-security-error.https.html:8:1 /service-workers/service-worker/rejections.https.html FAIL Rejections are DOMExceptions - assert_true: expected true got false @https://web-platform.test:8443/service-workers/service-worker/rejections.https.html:14:17 Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1501:20 Test.prototype.step_func/<@https://web-platform.test:8443/resources/testharness.js:1525:20 promise callback*@https://web-platform.test:8443/service-workers/service-worker/rejections.https.html:11:9 Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1501:20 @https://web-platform.test:8443/service-workers/service-worker/rejections.https.html:9:5 @https://web-platform.test:8443/service-workers/service-worker/rejections.https.html:7:2
Attachment #8968327 -
Attachment is obsolete: true
Assignee | ||
Comment 40•6 years ago
|
||
Also, there is a good chance I will need bug 1357463 to do this right.
Depends on: 1357463
Assignee | ||
Comment 41•6 years ago
|
||
This passes tests locally but still needs some cleanup and patch splitting.
Attachment #8968666 -
Attachment is obsolete: true
Assignee | ||
Comment 42•6 years ago
|
||
Actually, I won't need bug 1357463 yet. We might need it when we add the IPC layer to the service worker binding code.
No longer depends on: 1357463
Assignee | ||
Comment 43•6 years ago
|
||
Assignee | ||
Comment 44•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d0e47778e0e836de6272ef7de3e1a01b09aaba
Attachment #8968906 -
Attachment is obsolete: true
Assignee | ||
Comment 45•6 years ago
|
||
Attachment #8968956 -
Attachment is obsolete: true
Assignee | ||
Comment 46•6 years ago
|
||
Attachment #8969036 -
Attachment is obsolete: true
Assignee | ||
Comment 47•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=578f6aa121fb25c5ea4a3a1d38e556a298238f98
Assignee | ||
Comment 48•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=289590fbac2961e36b6fd1402a94a854bc4afe62
Attachment #8969057 -
Attachment is obsolete: true
Assignee | ||
Comment 49•6 years ago
|
||
Assignee | ||
Comment 50•6 years ago
|
||
Assignee | ||
Comment 51•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47393aa0498a452cac51151592e9ab6cf39a4da5
Attachment #8969067 -
Attachment is obsolete: true
Assignee | ||
Comment 52•6 years ago
|
||
Assignee | ||
Comment 53•6 years ago
|
||
Attachment #8969092 -
Attachment is obsolete: true
Assignee | ||
Comment 54•6 years ago
|
||
Assignee | ||
Comment 55•6 years ago
|
||
Attachment #8969383 -
Attachment is obsolete: true
Assignee | ||
Comment 56•6 years ago
|
||
Assignee | ||
Comment 57•6 years ago
|
||
Attachment #8969412 -
Attachment is obsolete: true
Assignee | ||
Comment 58•6 years ago
|
||
Assignee | ||
Comment 59•6 years ago
|
||
Assignee | ||
Comment 60•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a87cb456867186660c187287a3e0cd9878fdc4b7
Attachment #8969416 -
Attachment is obsolete: true
Assignee | ||
Comment 61•6 years ago
|
||
It seems I broke something while patch splitting. I'll have to investigate the orange.
Assignee | ||
Comment 62•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de98c10811a906c01081660488486e4f7de127c6
Attachment #8969437 -
Attachment is obsolete: true
Assignee | ||
Comment 63•6 years ago
|
||
Attachment #8969438 -
Attachment is obsolete: true
Assignee | ||
Comment 64•6 years ago
|
||
Attachment #8969442 -
Attachment is obsolete: true
Assignee | ||
Comment 65•6 years ago
|
||
Comment on attachment 8968939 [details] [diff] [review] P1 Make ServiceWorker MozPromise types exclusive so they can reject with ErrorResult values. r=baku Andrea, the ServiceWorkerContainer::Register() method is a bit of a mess, so I'm going to do the split in a series of patches. The first step is to make the MozPromise types for service worker registrations reject with ErrorResult. We need this in order to preserve TypeError messages that are produces in the ServiceWorkerManager job code. Its a bit tricky using ErrorResult with MozPromise because: 1. ErrorResult does not have a copy constructor. 2. ErrorResult will assert in its destructor if its failed, but not consumed. I've discovered I can mostly make this work by using the MozPromise exclusive flag and by always allowing the MozPromise to resolve/reject fully. We can't use its Disconnect() feature.
Attachment #8968939 -
Flags: review?(amarchesini)
Assignee | ||
Comment 66•6 years ago
|
||
Comment on attachment 8969076 [details] [diff] [review] P2 Don't try to use the document's nsILoadGroup when registering a new service worker script. r=baku This removes some code that attached the document's nsILoadGroup to the initial service worker fetch and execution. I believe this was originally needed to deal with firefox os security checking, but we don't need it any more. I also think its wrong if this load group cancels the register operation when the window is closed. It should really complete.
Attachment #8969076 -
Flags: review?(amarchesini)
Assignee | ||
Comment 67•6 years ago
|
||
Comment on attachment 8969082 [details] [diff] [review] P3 Refactor ServiceWorkerContainer::GetGlobalIfValid() to accomodate Register() usage. r=baku While I previously factored out the storage permission check in GetGlobalIfValid() I forgot that the exact console report message on failure needs to be different depending on where it is used. There are tests that verify this is correct (thankfully). To support the different message lets provide a lambda hook to perform the error report instead of hard coding it. This also changes the chrome script assertion in GetGlobalIfValid() to a runtime check to match what Register() needs.
Attachment #8969082 -
Flags: review?(amarchesini)
Assignee | ||
Comment 68•6 years ago
|
||
Comment on attachment 8969382 [details] [diff] [review] P4 Add the ServiceWorkerContainer::GetBaseURIFromGlobal() method. r=baku While splitting this stuff apart I have been trying to remove as many window-specific pieces of code as I can. In the near future we will need to support worker globals as well. Getting the base URL is one thing we'll have to correct as part of that effort. We have bug 1432481 filed to make this easier. For now, this patch moves the base URL stuff off into a separate static function to make it easier to see what else needs the window/doc references.
Attachment #8969382 -
Flags: review?(amarchesini)
Assignee | ||
Comment 69•6 years ago
|
||
Comment on attachment 8969411 [details] [diff] [review] P5 Move the IsFromAuthenticatedOrigin() check from the ServiceWorkerManager to the ServiceWorkerContainer. r=baku One of the recurring issues in this bug are security checks performed in ServiceWorkerManager that require the nsIDocument or window pointer. We can't do that in the future when SWM is in the parent process. Whenever possible I am converting it to a non-window-specific check, but some cannot. The IsFromAuthenticatedOrigin() code is one such check. It wants to walk the iframe parent tree. So this patch moves the check from SWM to ServiceWorkerContainer. This whole bit of code is kind of bad, though. We should really move this into a [Func] to better reflect how [SecureContext] is supposed to work in webidl. The interface should not even be exposed if its not secure. See bug 1455078.
Attachment #8969411 -
Flags: review?(amarchesini)
Assignee | ||
Comment 70•6 years ago
|
||
Comment on attachment 8969415 [details] [diff] [review] P6 Move the NS_CheckContentLoadPolicy() from ServiceWorkerManager to ServiceWorkerContainer. r=baku This patch moves another security check, NS_CheckContentLoadPolicy(), from SWM to the binding object. We must do this in the binding layer so that the check can use the nsIDocument's exact principal to get the CSP value. Once bug 965637 is fixed this can be changed to use ClientInfo for CSP and performed in the validation function I add in a later patch here.
Attachment #8969415 -
Flags: review?(amarchesini)
Assignee | ||
Comment 71•6 years ago
|
||
Comment on attachment 8969638 [details] [diff] [review] P7 Move many checks into ServiceWorkerScopeAndScriptAreValid() utility method. r=baku This patch takes the remaining validity checks from both the container and SWM and wraps them into a utility method. It: 1. Checks for http/https schemes only. 2. Checks for illegal escaped characters. 3. Verifies any URL ref has already been stripped. 4. Verifies the client's principal can load the URL. (Effectively same-origin check.) For now this patch calls this only in ServiceWorkerContainer. The last patch in this bug will also make SWM call this method. When we are in e10s mode we want the SWM to double-check the validity to avoid a content process lying to us.
Attachment #8969638 -
Flags: review?(amarchesini)
Assignee | ||
Comment 72•6 years ago
|
||
Comment on attachment 8969677 [details] [diff] [review] P8 Move storage permission check and other window specific code to ServiceWorkerContainer. r=baku Andrea, this removes the storage permission check from SWM and makes ServiceWorkerContainer::Register() do it via GetGlobalIfValid().
Attachment #8969677 -
Flags: review?(amarchesini)
Assignee | ||
Comment 73•6 years ago
|
||
Comment on attachment 8969678 [details] [diff] [review] P9 Implement the ServiceWorkerContainer::Register() method using the inner class. r=baku Finally, this implements the remainder of the changes. It: 1. Makes ServiceWorkerContainer::Register() call through the inner class to SWM. 2. Removes the xpcom register() method. 3. Calls the validation function in the SWM. 4. Makes the job callback trigger the MozPromise, etc.
Attachment #8969678 -
Flags: review?(amarchesini)
Updated•6 years ago
|
Attachment #8968939 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8969076 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8969382 -
Flags: review?(amarchesini) → review+
Comment 74•6 years ago
|
||
Comment on attachment 8969411 [details] [diff] [review] P5 Move the IsFromAuthenticatedOrigin() check from the ServiceWorkerManager to the ServiceWorkerContainer. r=baku Review of attachment 8969411 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/serviceworkers/ServiceWorkerContainer.cpp @@ +276,5 @@ > + aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); > + return nullptr; > + } > + > + // Next, implement a lame version of [SecureContext[ with an [SecureContext]
Attachment #8969411 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8969415 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8969638 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8969678 -
Flags: review?(amarchesini) → review+
Comment 75•6 years ago
|
||
Comment on attachment 8969082 [details] [diff] [review] P3 Refactor ServiceWorkerContainer::GetGlobalIfValid() to accomodate Register() usage. r=baku Review of attachment 8969082 [details] [diff] [review]: ----------------------------------------------------------------- I still like GetGlobalIfValid() written in this way. For sure, would be better to rename it. Plus, all the uses here and in part 8 are related to nsContentUtils::ReportToConsole(). Wondering if you can just pass an optional "params" argument instead having a callback. Up to you if you want this method with or without callback, but rename it and write a comment related to when the callback is executed. ::: dom/serviceworkers/ServiceWorkerContainer.cpp @@ +229,5 @@ > > already_AddRefed<Promise> > ServiceWorkerContainer::GetRegistrations(ErrorResult& aRv) > { > + nsIGlobalObject* global = GetGlobalIfValid(aRv, [](nsIDocument* aDoc) { Maybe rename the method to something else? I don't have a better name to suggest, but it's important to have a reason for this callback. @@ +233,5 @@ > + nsIGlobalObject* global = GetGlobalIfValid(aRv, [](nsIDocument* aDoc) { > + nsContentUtils::ReportToConsole(nsIScriptError::errorFlag, > + NS_LITERAL_CSTRING("Service Workers"), aDoc, > + nsContentUtils::eDOM_PROPERTIES, > + "ServiceWorkerGetRegistrationStorageError"); Why ServiceWorkerGetRegistrationStorageError?
Attachment #8969082 -
Flags: review?(amarchesini) → review+
Comment 76•6 years ago
|
||
Comment on attachment 8969677 [details] [diff] [review] P8 Move storage permission check and other window specific code to ServiceWorkerContainer. r=baku Review of attachment 8969677 [details] [diff] [review]: ----------------------------------------------------------------- You are probably going to rename GetGlobalIfValid() in the previous patches.
Attachment #8969677 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 77•6 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #75) > @@ +233,5 @@ > > + nsIGlobalObject* global = GetGlobalIfValid(aRv, [](nsIDocument* aDoc) { > > + nsContentUtils::ReportToConsole(nsIScriptError::errorFlag, > > + NS_LITERAL_CSTRING("Service Workers"), aDoc, > > + nsContentUtils::eDOM_PROPERTIES, > > + "ServiceWorkerGetRegistrationStorageError"); > > Why ServiceWorkerGetRegistrationStorageError? We have always used this same error string for both getRegistration() and getRegistrations().
Assignee | ||
Comment 78•6 years ago
|
||
Added a comment, but did not rename per our IRC discussion.
Attachment #8969082 -
Attachment is obsolete: true
Attachment #8970160 -
Flags: review+
Assignee | ||
Comment 79•6 years ago
|
||
Attachment #8969411 -
Attachment is obsolete: true
Attachment #8970161 -
Flags: review+
Comment 80•6 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/915fd6a149d2 P1 Make ServiceWorker MozPromise types exclusive so they can reject with ErrorResult values. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/25a7296ac1fc P2 Don't try to use the document's nsILoadGroup when registering a new service worker script. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/84c69758ca15 P3 Refactor ServiceWorkerContainer::GetGlobalIfValid() to accomodate Register() usage. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/2f32e0d8a75f P4 Add the ServiceWorkerContainer::GetBaseURIFromGlobal() method. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/06082fbb04ce P5 Move the IsFromAuthenticatedOrigin() check from the ServiceWorkerManager to the ServiceWorkerContainer. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/2f27625fd511 P6 Move the NS_CheckContentLoadPolicy() from ServiceWorkerManager to ServiceWorkerContainer. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/16ddf190ca51 P7 Move many checks into ServiceWorkerScopeAndScriptAreValid() utility method. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/bfadc112f9f7 P8 Move storage permission check and other window specific code to ServiceWorkerContainer. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f62b226f25 P9 Implement the ServiceWorkerContainer::Register() method using the inner class. r=baku
Comment 81•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/915fd6a149d2 https://hg.mozilla.org/mozilla-central/rev/25a7296ac1fc https://hg.mozilla.org/mozilla-central/rev/84c69758ca15 https://hg.mozilla.org/mozilla-central/rev/2f32e0d8a75f https://hg.mozilla.org/mozilla-central/rev/06082fbb04ce https://hg.mozilla.org/mozilla-central/rev/2f27625fd511 https://hg.mozilla.org/mozilla-central/rev/16ddf190ca51 https://hg.mozilla.org/mozilla-central/rev/bfadc112f9f7 https://hg.mozilla.org/mozilla-central/rev/f0f62b226f25
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•