1209699 - Push sample test for push service workers from about:debugging

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P1)

Description

[Axel Kratel]

For a push SW, there should be a way to do a simple push sample test from the SW listing on the about:debugging page.

Comment 1

[Axel Kratel]

See chrome://serviceworker-internals/ for an example of the simple push test.

Jan, is this covered in your about:debugging features?

Comment 2

[Jan Keromnes [:janx]]

Yes, I think this would make sense as an advanced about:debugging feature. Thanks for filing the follow-up bug.

Comment 3

[Jan Keromnes [:janx]]

Brian, did you mean to block the "(null)"-title bug on some other bug?

This sample-test-push feature-request seems unrelated.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Jan Keromnes [:janx] from comment #3)
> Brian, did you mean to block the "(null)"-title bug on some other bug?
> 
> This sample-test-push feature-request seems unrelated.

Yep, that was a mistake - thanks

Comment 5

[Jan Keromnes [:janx]]

I mean to work on this in a few weeks, self-assigning tentatively. (As always, feel free to steal if you have patches.)

Comment 6

[Jan Keromnes [:janx]]

Attachment: Push sample test for push service workers in about:debugging.

Comment 7

[Jan Keromnes [:janx]]

Attachment: Add a 'Push' button for service workers in about:debugging.

Comment 8

[Jan Keromnes [:janx]]

This patch depends on the patches in bug 1212797.

It adds a 'Push' button next to all service workers in about:debugging, regardless of whether the service workers actually subscribed to a real push service.

Comment 9

[Jan Keromnes [:janx]]

Attachment: Add a 'Push' button for service workers in about:debugging.

(I previously forgot to add the worker actor method.)

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8707912 [details] [diff] [review]
Add a 'Push' button for service workers in about:debugging.

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

Also need a test.

::: devtools/server/actors/worker.js
@@ +139,5 @@
> +    if (this._dbg.type !== Ci.nsIWorkerDebugger.TYPE_SERVICE) {
> +      return { error: "wrongType" };
> +    }
> +    let registration = this._getServiceWorkerRegistrationInfo();
> +    swm.sendPushEvent("", registration.scope);

Do you know what happens with an empty OriginAttributes?

185   [optional_argc] void sendPushEvent(in ACString aOriginAttributes,
186                                      in ACString aScope,
187                                      [optional] in uint32_t aDataLength,
188                                      [optional, array, size_is(aDataLength)] in uint8_t aDataBytes);

It looks like it should end up doing nothing when hitting this code:
http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#3184
3184   if (!swm->mRegistrationInfos.Get(aScopeKey, aData)) {
As aScopeKey derivates from originAttributes and should be empty here.

Comment 11

[Jan Keromnes [:janx]]

[Tracking Requested - why for this release]: Part of new Service Worker & Push features that we'd like to announce with Developer Edition 47.

Comment 12

[Benjamin Kerensa [:bkerensa]]

Tracking because this part of a new feature

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Jan Keromnes [:janx] from comment #11)
> [Tracking Requested - why for this release]: Part of new Service Worker &
> Push features that we'd like to announce with Developer Edition 47.

Welcome back Jan!  Any news on this? :)

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #13)
> (In reply to Jan Keromnes [:janx] from comment #11)
> > [Tracking Requested - why for this release]: Part of new Service Worker &
> > Push features that we'd like to announce with Developer Edition 47.
> 
> Welcome back Jan!  Any news on this? :)

Jan?

Comment 15

[Jan Keromnes [:janx]]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #13)
> (In reply to Jan Keromnes [:janx] from comment #11)
> > [Tracking Requested - why for this release]: Part of new Service Worker &
> > Push features that we'd like to announce with Developer Edition 47.
> 
> Welcome back Jan!  Any news on this? :)

Thanks Bryan! Yes, the patch mostly works and I rebase it frequently, but it has to wait for bug 1212797 to land first, which itself has to wait for bug 1242482.

Comment 16

[Jan Keromnes [:janx]]

Attachment: Add a 'Push' button for service workers in about:debugging.

Hi Julian, could you please have a look at this patch?

It adds a 'Push' button next to Service Workers in about:debugging, and you can use it to test push notifications.

You can try to use it with https://simple-push-demo.appspot.com/ or https://goroost.com/try-web-push.

:ochameau already pre-reviewed this patch, but it now has a test + a real originAttributes suffix.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=548c2f9f730b

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8724075 [details] [diff] [review]
Add a 'Push' button for service workers in about:debugging.

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

Code change looks good to me! Mostly nits about logs/comments in tests.

As you can see on try, browser_service_workers_push.js fails in e10s. 
It can be reproduced locally, and looks like an issue/race condition with the sw-claimed message.

r+ as long as the test is fixed.

::: devtools/client/aboutdebugging/components/target.js
@@ +41,5 @@
> +        React.createElement("button", {
> +          className: "push-button",
> +          onClick: this.push
> +        }, Strings.GetStringFromName("push")) :
> +        null),

Now that Bug 1207997 has landed (sorry :) ), would be nice to rebase and use dom.button() here.

::: devtools/client/aboutdebugging/test/browser_service_workers_push.js
@@ +4,5 @@
> +/* eslint-disable mozilla/no-cpows-in-tests */
> +/* global sendAsyncMessage */
> +
> +"use strict";
> +

Most devtools tests include a short summary as a top-level comment. 
Only a few about:debugging tests follow this (probably only the ones I added), so up to you to add one here.

@@ +13,5 @@
> +const SERVICE_WORKER = HTTP_ROOT + "service-workers/push-sw.js";
> +const TAB_URL = HTTP_ROOT + "service-workers/push-sw.html";
> +
> +add_task(function* () {
> +  yield new Promise(done => {

Maybe add an info() before this line ?

@@ +26,5 @@
> +
> +  let swTab = yield addTab(TAB_URL);
> +
> +  let serviceWorkersElement = document.getElementById("service-workers");
> +  yield waitForMutation(serviceWorkersElement, { childList: true });

Does not seem to fail for now, but I think it would be safer to :

> // Listen for mutations.
> let serviceWorkersElement = document.getElementById("service-workers");
> let onMutation = waitForMutation(serviceWorkersElement, { childList: true });
> // Add the tab containing a service worker. 
> let swTab = yield addTab(TAB_URL);
> // Wait for the expected mutation to resolve.
> yield onMutation;

@@ +32,5 @@
> +  // Check that the service worker appears in the UI.
> +  assertHasTarget(true, document, "service-workers", SERVICE_WORKER);
> +
> +  // Ensure that the registration resolved before trying to interact with the
> +  // service worker.

This comment could be turned into an info() (makes logs more useful when investigating test failures)

@@ +44,5 @@
> +  let targetElement = name.parentNode.parentNode;
> +  let pushBtn = targetElement.querySelector(".push-button");
> +  ok(pushBtn, "Found its push button");
> +
> +  // Make the test page notify us when the service worker sends a message.

This comment would be a useful info()

@@ +55,5 @@
> +  };
> +  let mm = swTab.linkedBrowser.messageManager;
> +  mm.loadFrameScript("data:,(" + encodeURIComponent(frameScript) + ")()", true);
> +
> +  // Wait for the service worker to claim the test window before proceeding.

This comment would be a useful info()

@@ +57,5 @@
> +  mm.loadFrameScript("data:,(" + encodeURIComponent(frameScript) + ")()", true);
> +
> +  // Wait for the service worker to claim the test window before proceeding.
> +  yield new Promise(done => {
> +    mm.addMessageListener("sw-claimed", function listener() {

Test times out on e10s because "sw-claimed" is never received.
FWIW, moving the listener right after `let swTab = yield addTab(TAB_URL);` fixes the issue, but I didn't check if the test was still valid when doing that.

@@ +63,5 @@
> +      done();
> +    });
> +  });
> +
> +  // Click on the Push button and wait for the service worker to receive a push

This comment would be a useful info()

@@ +75,5 @@
> +  pushBtn.click();
> +  yield onPushNotification;
> +  ok(true, "Service worker received a push notification");
> +
> +  // Finally, unregister the service worker itself.

This comment would be a useful info()

::: devtools/client/aboutdebugging/test/head.js
@@ +149,5 @@
>      observer.observe(target, mutationOptions);
>    });
>  }
> +
> +function assertHasTarget(expected, document, type, name) {

Can you add a JS doc for this method ?

@@ +156,5 @@
> +  is(names.includes(name), expected,
> +    "The " + type + " url appears in the list: " + names);
> +}
> +
> +function waitForServiceWorkerRegistered(tab) {

JS doc

@@ +159,5 @@
> +
> +function waitForServiceWorkerRegistered(tab) {
> +  // Make the test page notify us when the service worker is registered.
> +  let frameScript = function() {
> +    // Retrieve the `sw` promise created in the html page

nit: punctuation : dot at the end of line.

@@ +176,5 @@
> +    });
> +  });
> +}
> +
> +function unregisterServiceWorker(tab) {

JS doc

Comment 18

[Jan Keromnes [:janx]]

Attachment: Add a 'Push' button for service workers in about:debugging.

Rebased, addressed nits.

Comment 19

[Jan Keromnes [:janx]]

Comment on attachment 8724669 [details] [diff] [review]
Add a 'Push' button for service workers in about:debugging.

Carrying over r+.

Julian, could you please send this patch to try for me? I have configuration issues that will take some time to resolve.

try: -b do -p linux,linux64,macosx64,win64 -u reftest,reftest-e10s,mochitest-dt,mochitest-e10s-dt -t none

Comment 20

[Julian Descottes [:jdescottes]]

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

Comment 21

[Jan Keromnes [:janx]]

Thanks Julian! Try push looks green to me.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7f9ea43e7bf1

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7f9ea43e7bf1

Comment 24

[Jan Keromnes [:janx]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Part of new Devtools for Service Workers, adds a button to trigger a "push" event on a SW in about:debugging#workers
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 25

[Jan Keromnes [:janx]]

dev-doc-needed: The relevant MDN page is here https://developer.mozilla.org/en-US/docs/Tools/about:debugging

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx47 (Aurora) release notes.

Comment 27

[Will Bamberg [:wbamberg]]

This is done: https://developer.mozilla.org/en-US/docs/Tools/about:debugging