Show Push service connection state in `about:debugging`

RESOLVED FIXED in Firefox 50

Status

DevTools
about:debugging
P2
normal
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: lina, Assigned: janx)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments, 8 obsolete attachments)

565.03 KB, image/png
helenvholmes
: feedback+
Details
9.59 KB, patch
janx
: review+
Details | Diff | Splinter Review
26.12 KB, patch
janx
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
We've had several reports on IRC where a service worker claims to have a subscription, but `PushService.jsm` isn't actually connected to the Push service. We should add an indicator for whether the client is connected, and show the device ID for debugging.
(Reporter)

Updated

3 years ago
Depends on: 1189998
Since we'd like to transition from about:serviceworkers to about:debugging I'm moving this bug there and altering the summary appropriately.
Component: DOM: Push Notifications → Developer Tools: about:debugging
Product: Core → Firefox
Summary: Show Push service connection state in `about:serviceworkers` → Show Push service connection state in `about:debugging`
(Assignee)

Updated

3 years ago
Blocks: 1220747
(Assignee)

Comment 2

2 years ago
This would be an interesting feature for about:debugging's workers panel (bumping the bug so it stays on my radar).
(Assignee)

Updated

2 years ago
No longer blocks: 1220747
Mentor: janx
Priority: -- → P3
(Assignee)

Comment 3

2 years ago
Before allowing Push Service subscription changes, we should first show Push Service subscription details in about:debugging.

Bumping priority accordingly.
Assignee: nobody → janx
Blocks: 1221965
Mentor: janx
Status: NEW → ASSIGNED
Priority: P3 → P2
See Also: → bug 1260568
(Assignee)

Comment 4

2 years ago
Created attachment 8740036 [details] [diff] [review]
Show Push Service subscription details for Service Workers in about:debugging.
(Assignee)

Comment 5

2 years ago
Comment on attachment 8740036 [details] [diff] [review]
Show Push Service subscription details for Service Workers in about:debugging.

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

Ryan, I'd love your opinion on this proof of concept, and especially on the following topics:
- Should we create a dedicated PushSubscriptionActor?
- What do you think of pushSubscription living in ServiceWorkerActor's state?

::: devtools/client/aboutdebugging/components/service-worker-target.js
@@ +59,5 @@
> +          ),
> +          (pushSubscription ?
> +            dom.li({ className: "target-detail" },
> +              dom.strong(null, Strings.GetStringFromName("pushService")),
> +              dom.span({ className: "service-worker-push-url" },

Note: This className will be used in an upcoming test.

::: devtools/client/aboutdebugging/components/workers-tab.js
@@ +87,5 @@
>      ));
>    },
>  
>    update() {
> +    let { client } = this.props;

I'll remove this leftover, sorry.

::: devtools/server/actors/worker.js
@@ +370,5 @@
> +    let registration = this._registration;
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(registration.scope, registration.principal,
> +        (result, subscription) => {
> +          if (!subscription /* Components.isSuccessCode(result) */) {

I saw some other code use `Components.isSuccessCode`, but it's probably OK to just check for `subscription`, right?

@@ +371,5 @@
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(registration.scope, registration.principal,
> +        (result, subscription) => {
> +          if (!subscription /* Components.isSuccessCode(result) */) {
> +            reject({ error: "noPushSubscription" });

There errors are logged, maybe that's not the right way to say there is no subscription for this registration. Maybe return `{subscription: null}`?
Attachment #8740036 - Flags: feedback?(jryans)
(Assignee)

Comment 6

2 years ago
Created attachment 8740042 [details]
push-service.png
(Assignee)

Comment 7

2 years ago
Comment on attachment 8740042 [details]
push-service.png

Hi Helen!

I'm adding another detail to some Service Worker registrations: a "Push Service" URL (and probably an "unsubscribe" or "clear" link later).

The screeshot is here: https://bug1188981.bmoattachments.org/attachment.cgi?id=8740042

For Service Workers subscribed to a Push Service (for Push Notifications), it shows the subscription's "endpoint" (URL of the remote server that will send the notifications to your Service Worker). Do you think it makes sense to display it like that?
Attachment #8740042 - Flags: feedback?(hholmes)
(Assignee)

Comment 8

2 years ago
(In reply to Jan Keromnes [:janx] from comment #7)
> The screeshot is here:
> https://bug1188981.bmoattachments.org/attachment.cgi?id=8740042

(Sorry for the double screenshot link, I originally planned to post this comment in bug 1260568, but then decided to leave it for more general UX discussion).
(Assignee)

Updated

2 years ago
Blocks: 1156702
(In reply to Jan Keromnes [:janx] from comment #7)
> Comment on attachment 8740042 [details]
> push-service.png
> 
> Hi Helen!
> 
> I'm adding another detail to some Service Worker registrations: a "Push
> Service" URL (and probably an "unsubscribe" or "clear" link later).
> 
> The screeshot is here:
> https://bug1188981.bmoattachments.org/attachment.cgi?id=8740042
> 
> For Service Workers subscribed to a Push Service (for Push Notifications),
> it shows the subscription's "endpoint" (URL of the remote server that will
> send the notifications to your Service Worker). Do you think it makes sense
> to display it like that?

It seems like it makes sense to me—I don't have any issues with the layout from what I see!
Attachment #8740042 - Flags: feedback?(hholmes) → feedback+
Comment on attachment 8740036 [details] [diff] [review]
Show Push Service subscription details for Service Workers in about:debugging.

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

::: devtools/server/actors/worker.js
@@ +365,5 @@
>      request: {},
>      response: RetVal("json")
>    }),
> +
> +  getPushSubscription: method(function () {

I think it depends a bit on what the future looks like... are there other actions you want to perform on push subscriptions after getting them, like to modify them in some way?  Or you are just making a list of them, and there's nothing else to do?

If there are future actions to perform on push subscriptions, a separate actor would seem like a better fit.

@@ +370,5 @@
> +    let registration = this._registration;
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(registration.scope, registration.principal,
> +        (result, subscription) => {
> +          if (!subscription /* Components.isSuccessCode(result) */) {

Reading the IDL, your version seems okay, since you only care about the case where a subscription exists.

@@ +371,5 @@
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(registration.scope, registration.principal,
> +        (result, subscription) => {
> +          if (!subscription /* Components.isSuccessCode(result) */) {
> +            reject({ error: "noPushSubscription" });

I think your intuition is right, something like `{subscription: null}` sounds better, since it's not really an error exactly.  You are asking about a push subscription when you don't know ahead of time if there is one or not.  A protocol error would make more sense to me if you are passed some arguments that are invalid in some way.
Attachment #8740036 - Flags: feedback?(jryans) → feedback+
(Assignee)

Updated

2 years ago
Priority: P2 → P1
(Assignee)

Comment 11

2 years ago
Created attachment 8743802 [details] [diff] [review]
WIP Show Push Service subscription details for Service Workers in about:debugging.

Thanks Helen and Ryan for your feedback!

New work-in-progress patch addressing comments (separate PushSubscriptionActor, resolve to `null` instead of rejecting ro `error`).
(Assignee)

Updated

2 years ago
Attachment #8740036 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1266433
(Assignee)

Comment 12

2 years ago
(Skipping the Dev Edition 48 branch point because this needs more time to get right.)
Priority: P1 → P2
(Assignee)

Updated

2 years ago
Depends on: 1269385
(Assignee)

Comment 13

2 years ago
Created attachment 8754951 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

This took surprizingly long for such a simple feature, but this is ready for another round!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> I think it depends a bit on what the future looks like... are there other
> actions you want to perform on push subscriptions after getting them, like
> to modify them in some way?  Or you are just making a list of them, and
> there's nothing else to do?
> 
> If there are future actions to perform on push subscriptions, a separate
> actor would seem like a better fit.

I agree. There is now a separate PushSubscriptionActor, which already gives more information than this patch displays in about:debugging (in the future, we could easily show push count, time of last push, and quota information, which are all included in the actor's form).

I would also like to show a light indicator of Push Service status (e.g. a green dot if Firefox is connected to Mozilla's Push Service, or orange/red if there are network troubles / the user disabled the Push Service, with additional status information in the dot's tooltip).
Attachment #8754951 - Flags: review?(jryans)
(Assignee)

Updated

2 years ago
Attachment #8743802 - Attachment is obsolete: true
Comment on attachment 8754951 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

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

Thanks for working on this!  Getting closer, but a few actor issues that I'd like to see another round for.

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js
@@ +5,5 @@
>  /* eslint-env browser */
>  
>  "use strict";
>  
> +loader.lazyImporter(this, "setTimeout",

`setTimeout` should already be defined as a loader global[1].

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/builtin-modules.js#287

::: devtools/client/aboutdebugging/test/browser_service_workers_push_service.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* eslint-disable mozilla/no-cpows-in-tests */

Hmm, can you avoid CPOWs instead?  Do you have to use them?  Which lines are triggering this?  If they must be present, can you wrap them in eslint-disable / eslint-enable block so the rest of the test is covered by the rule?

@@ +10,5 @@
> +// in about:debugging if it exists, and disappears when unregistered.
> +
> +// Service workers can't be loaded from chrome://, but http:// is ok with
> +// dom.serviceWorkers.testing.enabled turned on.
> +const HTTP_ROOT = CHROME_ROOT.replace(

Do you import `shared-head`?  Can you use this instead:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#35

@@ +29,5 @@
> +      ["dom.serviceWorkers.testing.enabled", true],
> +      // Enable the push service.
> +      ["dom.push.connection.enabled", true],
> +    ]};
> +    SpecialPowers.pushPrefEnv(options, done);

`pushPrefEnv` was recently upgraded to return a promise itself, let's use that (may need to rebase first).

::: devtools/server/actors/worker.js
@@ +316,5 @@
> +
> +  initialize(conn, subscription) {
> +    protocol.Actor.prototype.initialize.call(this, conn);
> +    this._subscription = subscription;
> +    this.manage(this);

This is not quite right.  The parent actor that creates this child actor should itself call `this.manage(child)` after the `new ...Actor()` step.

It's harder to find examples of child actors, so the expected pattern is less obvious, see[1] for example.

[1]: https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/devtools/server/actors/inspector.js#966-970

@@ +333,5 @@
> +      quota: subscription.quota
> +    };
> +  },
> +
> +  destroy() {

You need to also call the super class destroy, like in `initialize`:

Actor.prototype.destroy.call(this);

See best practices[1] page.

[1]: https://wiki.mozilla.org/DevTools/Actor_Best_Practices

@@ +342,4 @@
>  // Lazily load the service-worker-child.js process script only once.
>  let _serviceWorkerProcessScriptLoaded = false;
>  
>  let ServiceWorkerRegistrationActor = protocol.ActorClass({

This actor is the first actor in a tree to use protocol.js.  For its `destroy` to be called in all cases, you must also implement `disconnect` which calls destroy.  See the wiki[1].

[1]: https://wiki.mozilla.org/DevTools/Actor_Best_Practices

@@ +367,1 @@
>      this.manage(this);

`this.manage(this)` should only be needed on _fronts_ for types that begin a protocol.js actor tree.  It should not be needed on this actor, please remove it.

@@ +379,4 @@
>      };
>    },
>  
> +  destroy() {

You need to also call the super class destroy, like in `initialize`:

Actor.prototype.destroy.call(this);

See best practices[1] page.

[1]: https://wiki.mozilla.org/DevTools/Actor_Best_Practices

@@ +471,5 @@
> +      );
> +    });
> +  }, {
> +    request: {},
> +    response: RetVal("json")

You're returning an actor, so your response should probably be `RetVal("pushSubscription")`.  Then can resolve the actor object itself.  protocol.js takes care of calling form() and writing it out.  You shouldn't need to wrap it in an object either.  Just `resolve(pushSubscriptionActor)` or similar.
Attachment #8754951 - Flags: review?(jryans) → feedback+
(Assignee)

Comment 15

2 years ago
Created attachment 8760344 [details] [diff] [review]
Switch about:debugging tests to devtools shared-head.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> 
> devtools/client/aboutdebugging/test/browser_service_workers_push_service.js
> @@ +1,4 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +/* eslint-disable mozilla/no-cpows-in-tests */
> 
> Hmm, can you avoid CPOWs instead?  Do you have to use them?  Which lines are
> triggering this?  If they must be present, can you wrap them in
> eslint-disable / eslint-enable block so the rest of the test is covered by
> the rule?

This exception was left over from before our ContentTask switch. I was able to remove it everywhere without issue.

> @@ +10,5 @@
> > +// in about:debugging if it exists, and disappears when unregistered.
> > +
> > +// Service workers can't be loaded from chrome://, but http:// is ok with
> > +// dom.serviceWorkers.testing.enabled turned on.
> > +const HTTP_ROOT = CHROME_ROOT.replace(
> 
> Do you import `shared-head`?  Can you use this instead:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> test/shared-head.js#35

I didn't know "http://example.com/" was equivalent to "http://mochi.test:8888/"! I imported shared-head.js into about:debugging's head.js, and switched all tests over to `URL_ROOT` in this separate patch (rest coming up in a second).
Attachment #8760344 - Flags: review?(jryans)
(Assignee)

Comment 16

2 years ago
Created attachment 8760353 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> Thanks for working on this!  Getting closer, but a few actor issues that I'd
> like to see another round for.

Thanks! This is the next round.

> devtools/client/aboutdebugging/components/workers/service-worker-target.js
> > +loader.lazyImporter(this, "setTimeout",
> 
> `setTimeout` should already be defined as a loader global[1].
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/builtin-
> modules.js#287

Indeed, removed the duplicate import.

> @@ +29,5 @@
> > +      ["dom.serviceWorkers.testing.enabled", true],
> > +      // Enable the push service.
> > +      ["dom.push.connection.enabled", true],
> > +    ]};
> > +    SpecialPowers.pushPrefEnv(options, done);
> 
> `pushPrefEnv` was recently upgraded to return a promise itself, let's use
> that (may need to rebase first).

Switched to the promise version (only in this test, for now).

> ::: devtools/server/actors/worker.js
> @@ +316,5 @@
> > +
> > +  initialize(conn, subscription) {
> > +    protocol.Actor.prototype.initialize.call(this, conn);
> > +    this._subscription = subscription;
> > +    this.manage(this);
> 
> This is not quite right.  The parent actor that creates this child actor
> should itself call `this.manage(child)` after the `new ...Actor()` step.
> 
> It's harder to find examples of child actors, so the expected pattern is
> less obvious, see[1] for example.
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/rev/
> 16663eb3dcfa759f25b5e27b101bc79270c156f2/devtools/server/actors/inspector.
> js#966-970

Thanks for catching this! I wasn't quite sure what `this.manage()` was doing. I moved the manage call next to the creation of the actor.

> > +  destroy() {
> 
> You need to also call the super class destroy, like in `initialize`:
> 
> Actor.prototype.destroy.call(this);

Done (in `ServiceWorkerRegistrationActor.destroy()` and `PushSubscriptionActor.destroy()`).

> @@ +342,4 @@
> >  // Lazily load the service-worker-child.js process script only once.
> >  let _serviceWorkerProcessScriptLoaded = false;
> >  
> >  let ServiceWorkerRegistrationActor = protocol.ActorClass({
> 
> This actor is the first actor in a tree to use protocol.js.  For its
> `destroy` to be called in all cases, you must also implement `disconnect`
> which calls destroy.  See the wiki[1].
> 
> [1]: https://wiki.mozilla.org/DevTools/Actor_Best_Practices

Indeed. Added the `disconnect` method.

Late thought: It's probably also the case for the `WorkerActor` class.

> @@ +367,1 @@
> >      this.manage(this);
> 
> `this.manage(this)` should only be needed on _fronts_ for types that begin a
> protocol.js actor tree.  It should not be needed on this actor, please
> remove it.

Fair enough, removed all occurences of "this.manage(this)" in worker.js, since presumably none of the classes in there are fronts.

> @@ +471,5 @@
> > +      );
> > +    });
> > +  }, {
> > +    request: {},
> > +    response: RetVal("json")
> 
> You're returning an actor, so your response should probably be
> `RetVal("pushSubscription")`.  Then can resolve the actor object itself. 
> protocol.js takes care of calling form() and writing it out.  You shouldn't
> need to wrap it in an object either.  Just `resolve(pushSubscriptionActor)`
> or similar.

Changed to `response: { subscription: RetVal("nullable:pushSubscription") }`.
Attachment #8760353 - Flags: review?(jryans)
(Assignee)

Updated

2 years ago
Attachment #8754951 - Attachment is obsolete: true
Comment on attachment 8760344 [details] [diff] [review]
Switch about:debugging tests to devtools shared-head.

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

::: devtools/client/aboutdebugging/test/browser_service_workers.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +/* import-globals-from ../../framework/test/shared-head.js */

Do you actually need this in each test?  It is necessary in head.js, but individual tests are meant to transitively pick up things from their head.js file.

Keep it if you must, but try to remove it from the tests first.
Attachment #8760344 - Flags: review?(jryans) → review+
Comment on attachment 8760353 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

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

Looks like bug 1277951 landed which decoupled this actor into a separate spec file (like is being done with all actors for devtools.html).  It's probably worth one more review after you rebase and account for this.

It seems good overall!

::: devtools/client/aboutdebugging/test/browser_service_workers_push_service.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* import-globals-from ../../framework/test/shared-head.js */

Shouldn't be needed in each test, as mentioned on previous patch.

::: devtools/client/aboutdebugging/test/service-workers/push-sw.html
@@ +13,5 @@
>  sw.then(
>    function (registration) {
>      dump("SW registered\n");
> +    registration.pushManager.subscribe().then(
> +        function (subscription) {

Nit: strange formatting, why 4 spaces?

::: devtools/server/actors/worker.js
@@ +51,3 @@
>    typeName: "worker",
>  
>    initialize: function (conn, dbg) {

Can you either implement `disconnect` for this one as well, or file a follow up for it?

@@ +393,5 @@
> +
> +  /**
> +   * Standard observer interface to listen to push messages and changes.
> +   *
> +   * @param subject

Probably don't need to list these params in the comment, it's a common Gecko API.

@@ +464,5 @@
> +            return;
> +          }
> +          pushSubscriptionActor = new PushSubscriptionActor(this._conn, subscription);
> +          this._pushSubscriptionActor = pushSubscriptionActor;
> +          this.manage(pushSubscriptionActor);

My apologies... it looks like you do not need `this.manage` here for the case where the parent actor that returns the child actor is the one that should own it, which is what you are doing here.

The protocol.js docs help to explain this[1] and the code seems to confirm this behavior[2].

Sorry for the confusion... it's hard to remember all the rules. :)

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/protocol.js.md#550-574
[2]: https://dxr.mozilla.org/mozilla-central/rev/1828937da9493b2cd54862b9c520b2ba5c7db92b/devtools/shared/protocol.js#292
Attachment #8760353 - Flags: review?(jryans) → feedback+
(Assignee)

Comment 19

2 years ago
Created attachment 8761540 [details] [diff] [review]
Switch about:debugging tests to devtools shared-head.

Thanks Ryan!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17)
> ::: devtools/client/aboutdebugging/test/browser_service_workers.js
> @@ +1,4 @@
> >  /* Any copyright is dedicated to the Public Domain.
> >     http://creativecommons.org/publicdomain/zero/1.0/ */
> >  
> > +/* import-globals-from ../../framework/test/shared-head.js */
> 
> Do you actually need this in each test?  It is necessary in head.js, but
> individual tests are meant to transitively pick up things from their head.js
> file.
> 
> Keep it if you must, but try to remove it from the tests first.

Indeed, they were unnecessary, so I removed them.

(Carrying over r+.)
Attachment #8761540 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8760344 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8761541 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> Looks like bug 1277951 landed which decoupled this actor into a separate
> spec file (like is being done with all actors for devtools.html).  It's
> probably worth one more review after you rebase and account for this.

Indeed, I rebased my patch and added a spec for the new pushSubscriptionActor.

> > +/* import-globals-from ../../framework/test/shared-head.js */
> 
> Shouldn't be needed in each test, as mentioned on previous patch.

Removed.

> > +    registration.pushManager.subscribe().then(
> > +        function (subscription) {
> 
> Nit: strange formatting, why 4 spaces?

Fixed (Cloud9 defaults to 4 spaces, which is annoying).

> >    typeName: "worker",
> >  
> >    initialize: function (conn, dbg) {
> 
> Can you either implement `disconnect` for this one as well, or file a follow
> up for it?

Implemented `disconnect()` and `destroy()` methods on workerActor (which calls `_detach()` if necessary).

> > +  /**
> > +   * Standard observer interface to listen to push messages and changes.
> > +   *
> > +   * @param subject
> 
> Probably don't need to list these params in the comment, it's a common Gecko
> API.

Granted. Removed.

> > +          pushSubscriptionActor = new PushSubscriptionActor(this._conn, subscription);
> > +          this._pushSubscriptionActor = pushSubscriptionActor;
> > +          this.manage(pushSubscriptionActor);
> 
> My apologies... it looks like you do not need `this.manage` here for the
> case where the parent actor that returns the child actor is the one that
> should own it, which is what you are doing here.

Removed `this.manage(pushSubscriptionActor)`. Thanks for checking and linking to the docs!
Attachment #8761541 - Flags: review?(jryans)
(Assignee)

Updated

2 years ago
Attachment #8760353 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
Created attachment 8761543 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

(In reply to Jan Keromnes [:janx] from comment #20)
> > > +/* import-globals-from ../../framework/test/shared-head.js */
> > 
> > Shouldn't be needed in each test, as mentioned on previous patch.
> 
> Removed.

Now actually removed.
Attachment #8761543 - Flags: review?(jryans)
(Assignee)

Updated

2 years ago
Attachment #8761541 - Attachment is obsolete: true
Attachment #8761541 - Flags: review?(jryans)
Comment on attachment 8761543 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

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

Okay, looks good to me!
Attachment #8761543 - Flags: review?(jryans) → review+
(Assignee)

Comment 24

2 years ago
Thanks Ryan! It seems I misspelled a try job on the last push, here is a new one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88d6adfa187d
(Assignee)

Comment 25

2 years ago
Well, it seems that browser_service_workers_push_service.js is now leaking 2 windows. This is going to be fun.
(Assignee)

Comment 26

2 years ago
Created attachment 8764994 [details] [diff] [review]
Switch about:debugging tests to devtools shared-head.

Rebased, carried over.
Attachment #8764994 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8761540 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
Created attachment 8764996 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

Rebased, carried over.

Fixed leaks with:

>   info("Unmock the push service");
>   PushService.service = null;

in browser_service_workers_push_service.js.
Attachment #8764996 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8761543 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/17a89f9b5d55
Switch about:debugging tests to devtools shared-head. r=jryans
https://hg.mozilla.org/integration/fx-team/rev/110c7ad7d399
Show Push Service subscription endpoint URL for Service Workers in about:debugging. r=jryans
Keywords: checkin-needed

Comment 30

2 years ago
Landed this morning:
https://hg.mozilla.org/mozilla-central/rev/17a89f9b5d55
https://hg.mozilla.org/mozilla-central/rev/110c7ad7d399
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1274106

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.