1275434 - Refactor `PushNotifier` to clarify remoting

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

Currently, all `PushNotifier` methods checked the process type, and either call `Notify*{Workers, Observers}` or send an IPDL message. The message handlers then called back in to `PushNotifier` from the remote process.

This was clearer when we only sent worker event notifications to the content process, and fired all observer notifications in the parent. It became more complicated once we started notifying observers for all subscriptions in both processes (bug 1266433).

This makes it harder to see what's missing. For example, "push-subscription-modified" isn't currently forwarded to the child, but "push-subscription-change" and "push-message" are.

Comment 1

[Lina Butler [:lina]]

Attachment: 1.patch

I moved the remoting code into PushNotifier::Dispatch(), and added some Dispatcher classes for the different notification types. This cleans up the duplicated e10s checks in NotifyPushWorkers and NotifySubscriptionChangeWorkers, and I think Dispatch() makes it easier to see what's called where.

Dragana, please let me know if this makes sense, or if there's a better approach. In JS, I'd use functions instead of a class, but it seems like I can't pass a C++ lambda as a function pointer if it captures variables.

Comment 2

[Lina Butler [:lina]]

Attachment: 2.patch

Send "push-subscription-modified" to the child, to match "push-message" and "push-subscription-change".

Comment 3

[Lina Butler [:lina]]

Attachment: 3.patch

xpcshell tests.

Comment 4

[Dragana Damjanovic [:dragana]]

(In reply to Kit Cambridge [:kitcambridge] from comment #1)
> Created attachment 8756136 [details] [diff] [review]
> 1.patch
> 
> I moved the remoting code into PushNotifier::Dispatch(), and added some
> Dispatcher classes for the different notification types. This cleans up the
> duplicated e10s checks in NotifyPushWorkers and
> NotifySubscriptionChangeWorkers, and I think Dispatch() makes it easier to
> see what's called where.
> 
> Dragana, please let me know if this makes sense, or if there's a better
> approach. In JS, I'd use functions instead of a class, but it seems like I
> can't pass a C++ lambda as a function pointer if it captures variables.

you could use function pointers, but maybe it is better this way different behaviors are grouped into separate classes.

Comment 5

[Dragana Damjanovic [:dragana]]

Comment on attachment 8756136 [details] [diff] [review]
1.patch

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

::: dom/push/PushNotifier.cpp
@@ +103,5 @@
> +      // At least one content process is active, so e10s must be enabled.
> +      // Broadcast a message to notify observers and service workers.
> +      bool ok = true;
> +      for (uint32_t i = 0; i < contentActors.Length(); ++i) {
> +        ok &= aDispatcher.SendToChild(contentActors[i]);

use ok = ok && aDispatcher.SendToChild(contentActors[i]);
The result will be the same, but logically you want && and not bitwise and (&)

Comment 6

[Lina Butler [:lina]]

(In reply to Dragana Damjanovic [:dragana] from comment #5)
> use ok = ok && aDispatcher.SendToChild(contentActors[i]);
> The result will be the same, but logically you want && and not bitwise and
> (&)

Oops, thanks for catching that! Changed to `NS_WARN_IF` instead, to match `SendToParent`.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23080ad39e30
Refactor `PushNotifier` to clarify remoting logic. r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/80fbbee5de1f
Notify "push-subscription-modified" observers in the child. r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/332caaad198f
Add remote push observer tests. r=dragana

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23080ad39e30
https://hg.mozilla.org/mozilla-central/rev/80fbbee5de1f
https://hg.mozilla.org/mozilla-central/rev/332caaad198f