Closed Bug 1455695 Opened 2 years ago Closed 2 years ago

make ServiceWorkerRegistration::Inner only deal with MozPromise and cross-process types

Categories

(Core :: DOM: Service Workers, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 22 obsolete files)

18.44 KB, patch
baku
: review+
Details | Diff | Splinter Review
5.98 KB, patch
baku
: review+
Details | Diff | Splinter Review
8.44 KB, patch
baku
: review+
Details | Diff | Splinter Review
6.09 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.46 KB, patch
baku
: review+
Details | Diff | Splinter Review
In order to make use of the inner classes to implement remoting the inner classes must not have dependencies on content process types.  So I need to remove stuff like promise, JSContext, etc from ServiceWorkerRegistration::Inner.
Priority: -- → P2
(In reply to Ben Kelly [:bkelly] from comment #0)
> In order to make use of the inner classes to implement remoting the inner
> classes must not have dependencies on content process types.  So I need to
> remove stuff like promise, JSContext, etc from
> ServiceWorkerRegistration::Inner.

Any long term implications to removing these?
(In reply to Marion Daly [:mdaly] from comment #1)
> Any long term implications to removing these?

Not really.  It just means we have to do some work in ServiceWorkerRegistration.cpp instead of in lower code modules.  Its necessary because we can't pass these types across the IPC boundary.
There is a spec incompatibility in our update() method:

https://github.com/w3c/ServiceWorker/issues/1304

I think I should probably fix that while I'm touching this code.
Comment on attachment 8970290 [details] [diff] [review]
P0 Expose a GetPrincipal() convenience method on service worker descriptor classes. r=baku

Andrea, this patch adds some convenience methods on the ServiceWorker*Descriptor classes that return an nsIPrincipal* instead of PrincipalInfo.  This is consistent with what we have on ClientInfo::GetPrincipal().
Attachment #8970290 - Flags: review?(amarchesini)
Comment on attachment 8970291 [details] [diff] [review]
P1 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku

This patch moves the binding-level details for ServiceWorkerRegistration.update() into ServiceWorkerRegistration.cpp.  The SWM and inner class only deal with MozPromise and portable internal types.

Note, this patch does change some behavior.  We were previously resolving undefined from an update() call, but the spec requires us to resolve the registration.
Attachment #8970291 - Flags: review?(amarchesini)
Comment on attachment 8970296 [details] [diff] [review]
P2 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku

This patch fixes a mochitest and adds a WPT for the new behavior where we resolve a registration from update().
Attachment #8970296 - Flags: review?(amarchesini)
Comment on attachment 8970290 [details] [diff] [review]
P0 Expose a GetPrincipal() convenience method on service worker descriptor classes. r=baku

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

::: dom/serviceworkers/ServiceWorkerDescriptor.cpp
@@ +101,5 @@
> +nsCOMPtr<nsIPrincipal>
> +ServiceWorkerDescriptor::GetPrincipal() const
> +{
> +  AssertIsOnMainThread();
> +  nsCOMPtr<nsIPrincipal> ref =  PrincipalInfoToPrincipal(mData->principalInfo());

How often is this method going to be called? Asking because maybe we could cache the nsIPrincipal object.

::: dom/serviceworkers/ServiceWorkerRegistrationDescriptor.cpp
@@ +136,5 @@
> +nsCOMPtr<nsIPrincipal>
> +ServiceWorkerRegistrationDescriptor::GetPrincipal() const
> +{
> +  AssertIsOnMainThread();
> +  nsCOMPtr<nsIPrincipal> ref =  PrincipalInfoToPrincipal(mData->principalInfo());

same here.
Attachment #8970290 - Flags: review?(amarchesini) → review+
Attachment #8970291 - Flags: review?(amarchesini) → review+
Attachment #8970296 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #27)
> ::: dom/serviceworkers/ServiceWorkerDescriptor.cpp
> @@ +101,5 @@
> > +nsCOMPtr<nsIPrincipal>
> > +ServiceWorkerDescriptor::GetPrincipal() const
> > +{
> > +  AssertIsOnMainThread();
> > +  nsCOMPtr<nsIPrincipal> ref =  PrincipalInfoToPrincipal(mData->principalInfo());
> 
> How often is this method going to be called? Asking because maybe we could
> cache the nsIPrincipal object.

I don't think its very hot.  And hopefully in the nearish future nsIPrincipal will become threadsafe and we can nuke PrincipalInfo completely.  I'd rather not deal with the cached value for now.
I'm going to land the first three patches in a separate bug while I fix orange on the remaining bugs here.
Depends on: 1456466
Attachment #8970290 - Attachment is obsolete: true
Attachment #8970291 - Attachment is obsolete: true
Attachment #8970296 - Attachment is obsolete: true
Blocks: 1456981
No longer blocks: 1456981
Comment on attachment 8970992 [details] [diff] [review]
P1 Make ServiceWorkerRegistration::Inner::Unregister() use MozPromise. r=baku

Andrea, this makes ServiceWorkerRegistration::Unregister() use a MozPromise similar to the previous patches for Update().
Attachment #8970992 - Flags: review?(amarchesini)
Comment on attachment 8970993 [details] [diff] [review]
P2 Remove ServiceWorkerRegistration::Inner::GetPushManager(). r=baku

This moves the code to get the PushManager out of the Inner and just does it in the binding layer.
Attachment #8970993 - Flags: review?(amarchesini)
Comment on attachment 8971008 [details] [diff] [review]
P3 Remove ServiceWorkerRegistration::Inner::ShowNotification(). r=baku

This moves the logic for ShowNotification out of the inner and does it in the binding layer.

One tricky bit here was that there is a check for active worker.  We can only do this on the main thread for window right now because we have not exposed ServiceWorker objects on worker threads yet.
Attachment #8971008 - Flags: review?(amarchesini)
Comment on attachment 8971010 [details] [diff] [review]
P4 Remove ServiceWorkerRegistration::Inner::GetNotifications(). r=baku

This moves GetNotification() logic out of inner and back to the binding layer.
Attachment #8971010 - Flags: review?(amarchesini)
Comment on attachment 8971013 [details] [diff] [review]
P5 Remove unused ErrorResult argument from ServiceWorkerRegistration::Inner::Update(). r=baku

This removes an unused variable from Update().  All errors reject the returned MozPromise, so we don't need to pass ErrorResult.
Attachment #8971013 - Flags: review?(amarchesini)
Attachment #8970992 - Flags: review?(amarchesini) → review+
Attachment #8970993 - Flags: review?(amarchesini) → review+
Attachment #8971008 - Flags: review?(amarchesini) → review+
Attachment #8971010 - Flags: review?(amarchesini) → review+
Attachment #8971013 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd48fe9eb3dd
P1 Make ServiceWorkerRegistration::Inner::Unregister() use MozPromise. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f25c38c4fd6
P2 Remove ServiceWorkerRegistration::Inner::GetPushManager(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c21114e606e2
P3 Remove ServiceWorkerRegistration::Inner::ShowNotification(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b31ab5129bfc
P4 Remove ServiceWorkerRegistration::Inner::GetNotifications(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d28c827d1e85
P5 Remove unused ErrorResult argument from ServiceWorkerRegistration::Inner::Update(). r=baku
Depends on: 1458756
You need to log in before you can comment on or make changes to this bug.