Closed Bug 1209699 Opened 4 years ago Closed 4 years ago

Push sample test for push service workers from about:debugging

Categories

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

defect

Tracking

(firefox47+ fixed, relnote-firefox 47+)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 + fixed
relnote-firefox --- 47+

People

(Reporter: akratel, Assigned: janx)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

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.
See chrome://serviceworker-internals/ for an example of the simple push test.

Jan, is this covered in your about:debugging features?
Depends on: about:debugging
Flags: needinfo?(janx)
Blocks: 1188675
Yes, I think this would make sense as an advanced about:debugging feature. Thanks for filing the follow-up bug.
Flags: needinfo?(janx)
Summary: Push sample test from about:debugging → Push sample test for push Service Workers from about:debugging
Summary: Push sample test for push Service Workers from about:debugging → Push sample test for push service workers from about:debugging
Brian, did you mean to block the "(null)"-title bug on some other bug?

This sample-test-push feature-request seems unrelated.
Flags: needinfo?(bgrinstead)
(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
No longer blocks: 1211017
Flags: needinfo?(bgrinstead)
I mean to work on this in a few weeks, self-assigning tentatively. (As always, feel free to steal if you have patches.)
Assignee: nobody → janx
Blocks: 1214248
Blocks: 1209369
Component: Developer Tools → Developer Tools: about:debugging
Depends on: 1216309
Depends on: 1212797
Attachment #8681427 - Attachment is obsolete: true
No longer depends on: 1216309
Attachment #8707903 - Flags: review?(poirot.alex)
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.
(I previously forgot to add the worker actor method.)
Attachment #8707912 - Flags: review?(poirot.alex)
Attachment #8707903 - Attachment is obsolete: true
Attachment #8707903 - Flags: review?(poirot.alex)
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.
Attachment #8707912 - Flags: review?(poirot.alex)
Keywords: dev-doc-needed
[Tracking Requested - why for this release]: Part of new Service Worker & Push features that we'd like to announce with Developer Edition 47.
Tracking because this part of a new feature
(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? :)
(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?
Flags: needinfo?(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.
Flags: needinfo?(janx)
Priority: -- → P1
Depends on: 1250902
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
Attachment #8724075 - Flags: review?(jdescottes)
Attachment #8707912 - Attachment is obsolete: true
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
Attachment #8724075 - Flags: review?(jdescottes) → review+
Rebased, addressed nits.
Attachment #8724075 - Attachment is obsolete: true
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
Flags: needinfo?(jdescottes)
Attachment #8724669 - Flags: review+
Thanks Julian! Try push looks green to me.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7f9ea43e7bf1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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)]:
relnote-firefox: --- → ?
dev-doc-needed: The relevant MDN page is here https://developer.mozilla.org/en-US/docs/Tools/about:debugging
Added to Fx47 (Aurora) release notes.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.