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

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Marion Daly [:mdaly]]

(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?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06af4c51de7bcfbb18d03c254f66ca9d7a9eda5a

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Expose a GetPrincipal() convenience method on service worker descriptor classes. r=baku

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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().

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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().

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make ServiceWorkerRegistration::Inner::Unregister() use MozPromise. r=baku

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove ServiceWorkerRegistration::Inner::GetPushManager(). r=baku

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove ServiceWorkerRegistration::Inner::ShowNotification(). r=baku

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Remove ServiceWorkerRegistration::Inner::GetNotifications(). r=baku

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make ServiceWorkerRegistration::Inner::Unregister() use MozPromise. r=baku

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove ServiceWorkerRegistration::Inner::GetPushManager(). r=baku

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove ServiceWorkerRegistration::Inner::ShowNotification(). r=baku

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Remove ServiceWorkerRegistration::Inner::GetNotifications(). r=baku

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=736ab12baa124c1d7bc995860b3d8ad402bf774a

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Remove unused ErrorResult argument from ServiceWorkerRegistration::Inner::Update(). r=baku

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27e4b3d1f4db6392d1df383d0afc200c5c19137e

Comment 27

[Andrea Marchesini [:baku]]

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.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

I'm going to land the first three patches in a separate bug while I fix orange on the remaining bugs here.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistration::Inner::Unregister() use MozPromise. r=baku

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove ServiceWorkerRegistration::Inner::GetPushManager(). r=baku

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove ServiceWorkerRegistration::Inner::ShowNotification(). r=baku

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove ServiceWorkerRegistration::Inner::GetNotifications(). r=baku

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove unused ErrorResult argument from ServiceWorkerRegistration::Inner::Update(). r=baku

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistration::Inner::Unregister() use MozPromise. r=baku

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove ServiceWorkerRegistration::Inner::GetPushManager(). r=baku

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove ServiceWorkerRegistration::Inner::ShowNotification(). r=baku

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove ServiceWorkerRegistration::Inner::GetNotifications(). r=baku

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove unused ErrorResult argument from ServiceWorkerRegistration::Inner::Update(). r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b77074f6a441b1d4f2a9e032439baf2fd78e9743

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

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().

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
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

Comment 46

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cd48fe9eb3dd
https://hg.mozilla.org/mozilla-central/rev/8f25c38c4fd6
https://hg.mozilla.org/mozilla-central/rev/c21114e606e2
https://hg.mozilla.org/mozilla-central/rev/b31ab5129bfc
https://hg.mozilla.org/mozilla-central/rev/d28c827d1e85