1258902 - about:debugging doesn't have a way to unregister a service worker

Status: RESOLVED FIXED

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

Description

[Edwin Wong [:edwong]]

as a web dev, i want to uninstall a service worker to I can be sure it's updating properly (sometimes it doesn't) so that I can test it.  

1. install a service worker goto: 
https://serviceworke.rs/push-payload_demo.html and click 'request sending a notification'
2. goto: 
about:debugging#workers

actual: there isn't a button to unregister or uninstall SW

expected: there should be, in about:serviceworkers, there was this button

Comment 1

[Jan Keromnes [:janx]]

[Tracking Requested - why for this release]: Edwin would like this feature to be available in the next release.

Comment 2

[Jan Keromnes [:janx]]

Thanks Edwin for filing this bug!

Idea: Maybe we could show the SW's scope under its URL in about:debugging, and have an "unregister" link right next to it. That way it doesn't add yet another button, and it's probably an action you'd perform less often than "Debug".

Other idea: Maybe we could have a dropdown button next to "Debug", to hide buttons used a little less often (e.g. "Push", "Stop", "Unregister", etc.) This is what Chrome's remote debugging page does.

Comment 3

[Edwin Wong [:edwong]]

Just to compare
Chrome base state:
Unregister | Start

Chrome start state:
 Stop | Push | Inspect
Unregister (on a new line - i suspect this is WIP as this isn't fancy)

Fx base:
Start

Fx started:
Push | Debug

Comment 4

[Julian Descottes [:jdescottes]]

Taking this one.
I plan to implement this using a new line displaying the scope as well as an unregister link.

Comment 5

[Jan Keromnes [:janx]]

(In reply to Julian Descottes [:jdescottes] from comment #4)
> Taking this one.
> I plan to implement this using a new line displaying the scope as well as an
> unregister link.

Very cool, thanks Julian!

See also bug 1260568 for UX concepts and tracking.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part1.wip.patch

Jan: Let me know what you think about the implementation & ui.
I think we should have a second part in this bug in order to create a dedicated "serviceworker-target" component, similar to what you did with addon-target and worker-target.

I'll start working on this as well but let me know if you have any tips/guidelines.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part1.wip.patch

(forgot to include the test in the previous patch)

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part2.wip.patch

As mentioned in my previous comment, here is an attempt at extracting a serviceworker-target component.

The only major change here is that I extracted the debug method to a new "utils" module at the root level of aboutdebugging. I think this method is complex enough to avoid duplicating it. Another option would be to define it in worker-tab.js and pass it as a prop. Let me know which way you prefer.

I'm also starting to be a bit concerned by the code duplication in the render method (mainly icon & button rendering). It is low complexity, so I wouldn't change this immediately, but just keep an eye on it.

Comment 9

[Julian Descottes [:jdescottes]]

Here is a try push with the two patches above; https://treeherder.mozilla.org/#/jobs?repo=try&revision=40f234ebef51

Comment 10

[Jan Keromnes [:janx]]

Comment on attachment 8737542 [details] [diff] [review]
bug1258902.part1.wip.patch

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

Thanks a lot for adding service worker scope + unregister!

Looks good to me, and as usual I mostly have a few nits.

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ +84,5 @@
> +}
> +
> +.target-info-list {
> +  margin: 3px 0;
> +  padding: 0 0 0 20px;

Nit: Maybe add CSS rule `list-style-type: none` here?

@@ +100,5 @@
> +  content: "(";
> +}
> +.target-info a:after {
> +  content: ")";
> +}

Nit: Cool CSS trick, but the "(" and ")" don't feel useful. Maybe we can do without?

@@ +113,5 @@
> +
> +.target-info-title,
> +.target-info-content {
> +  margin-right: 5px;
> +}

Nit: I think this CSS block would be better as:

    .target-info > :not(:first-child) {
      margin-left: 5px;
    }

Nit: Also, I would like to suggest the following renames:

- target-info-list: target-details
- target-info: target-detail
- target-info-title: (no need for a class or a ":" here, you can just use a <strong>)
- target-info-content: (no need for this class)

But I don't feel too strongly about them, so feel free to ignore.

::: devtools/client/aboutdebugging/components/worker-target.js
@@ +20,5 @@
>  
>  const Strings = Services.strings.createBundle(
>    "chrome://devtools/locale/aboutdebugging.properties");
>  
> +const l10n = Strings.GetStringFromName;

Rant: I never liked shortcuts like "l10n", "i18n" or "a11y", because they're not transparent, and they're not that helpful in saving characters/keystrokes anyway. It's my opinion that the less background knowledge we require for someone to understand our code, the better, and there are better names for such functions e.g. "localize".

However, the rest of our codebase does use "l10n", so I'm not sure we'd actually do more good than bad using a different name here. Your call, I'm fine either way.

@@ +41,5 @@
> +        dom.div({ className: "target-name" }, target.name),
> +        isServiceWorker ? dom.ul({ className: "target-info-list" },
> +          dom.li({ className: "target-info target-info-scope" },
> +            dom.span({ className: "target-info-title" }, l10n("scope")),
> +            dom.span({ className: "target-info-content" }, target.scope),

Nit: If you follow my suggested CSS class renames, I think it would make more sense to have something like:

    dom.li({ className: "target-detail" },
      dom.strong({}, l10n("scope")),
      dom.span({ className: "serviceworker-scope" }, target.scope)

Note: in my suggestion, "target-info-scope" is gone from the <li>, but the scope span has a specific className that tests can use.

::: devtools/client/aboutdebugging/test/browser_service_workers_unregister.js
@@ +7,5 @@
> +"use strict";
> +
> +// Test that clicking on the unregister link in the Service Worker details works
> +// as intended in about:debugging.
> +// It should unregister the service-worker, which should trigger an update of

Nit: s/service-worker/service worker/.

::: devtools/client/locales/en-US/aboutdebugging.properties
@@ +5,5 @@
>  debug = Debug
>  push = Push
>  start = Start
>  
> +scope = scope

Nit: Maybe capitalize?

::: devtools/server/actors/worker.js
@@ +344,5 @@
>  
> +  unregister: method(function() {
> +    let unregisterCallback = {
> +      unregisterSucceeded: function() {},
> +      unregisterFailed: function() {},

Nit: Maybe we should at least log an error here?

Comment 11

[Jan Keromnes [:janx]]

Comment on attachment 8737544 [details] [diff] [review]
bug1258902.part2.wip.patch

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

As discussed on IRC, I'm fully OK splitting worker-target.js up further.

F- because of the isRunning() TypeError, and because I'm not fully convinced a utils.js is the best way to share code between our components.

My main gripe about such a file is that it acts like a collection of global functions available everywhere, but I suspect that most of the methods will actually be specific to only a few components. A single file will encourage the formation of incidental complexity (accidental dependencies between unrelated but co-located modules), and can grow very big / become hard to refactor and maintain.

Instead, I think it would be better to use something like a React Mixin[0], and have a WorkerMixin implementing the shared method `debugWorker()`. If you're unsure how they work, or if they're the right solution here, let's involve :jlongster or :linclark in this discussion.

[0] https://facebook.github.io/react/docs/reusable-components.html#mixins

::: devtools/client/aboutdebugging/components/moz.build
@@ +6,5 @@
>      'aboutdebugging.js',
>      'addon-target.js',
>      'addons-controls.js',
>      'addons-tab.js',
> +    'serviceworker-target.js',

Nit: Please keep the file names consistent by either renaming this to 'service-worker-target.js', or by renaming 'tab-menu-entry.js' to 'tab-menuentry.js'. I prefer the former.

::: devtools/client/aboutdebugging/components/worker-target.js
@@ +20,5 @@
>    displayName: "WorkerTarget",
>  
>    render() {
>      let { target, debugDisabled } = this.props;
>      let isRunning = this.isRunning();

You removed `isRunning()` from this file, so this line will fail.

Comment 12

[Julian Descottes [:jdescottes]]

(In reply to Jan Keromnes [:janx] from comment #10)
> Comment on attachment 8737542 [details] [diff] [review]
> bug1258902.part1.wip.patch
> 
> Review of attachment 8737542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks a lot for adding service worker scope + unregister!
> 
> Looks good to me, and as usual I mostly have a few nits.

Thanks for the review. Particularly, thanks a lot for classname changes suggestions. They all make sense and I feel the code is much better now!

> 
> ::: devtools/client/aboutdebugging/aboutdebugging.css
> @@ +84,5 @@
> > +}
> > +
> > +.target-info-list {
> > +  margin: 3px 0;
> > +  padding: 0 0 0 20px;
> 
> Nit: Maybe add CSS rule `list-style-type: none` here?
> 
> @@ +100,5 @@
> > +  content: "(";
> > +}
> > +.target-info a:after {
> > +  content: ")";
> > +}
> 
> Nit: Cool CSS trick, but the "(" and ")" don't feel useful. Maybe we can do
> without?
> 

I applied your proposed changes, and bumped the padding to isolate the title from the scope and from the link. Regarding the "()" around unregister, my goal was not to show off a cool CSS trick :) They should actually be outside of the link and not clickable. I just wanted to isolate the "unregister" action from the scope's text, a way to warn the user that this is an action.

> 
> ::: devtools/client/aboutdebugging/components/worker-target.js
> @@ +20,5 @@
> >  
> >  const Strings = Services.strings.createBundle(
> >    "chrome://devtools/locale/aboutdebugging.properties");
> >  
> > +const l10n = Strings.GetStringFromName;
> 
> Rant: I never liked shortcuts like "l10n", "i18n" or "a11y", because they're
> not transparent, and they're not that helpful in saving
> characters/keystrokes anyway. It's my opinion that the less background
> knowledge we require for someone to understand our code, the better, and
> there are better names for such functions e.g. "localize".
> 
> However, the rest of our codebase does use "l10n", so I'm not sure we'd
> actually do more good than bad using a different name here. Your call, I'm
> fine either way.

I decided to remove it for now, because it's not the purpose of this bug. When we make such a change it should be in all the aboutdebugging module at once. The other react modules in devtools seem to rely on "devtools/client/shared/l10n" which provides a getStr method. ( https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/utils.js#9 and https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/components/census-header.js#24 )

What about doing something similar here? I can add it as a separate commit in this bug.

> 
> ::: devtools/server/actors/worker.js
> @@ +344,5 @@
> >  
> > +  unregister: method(function() {
> > +    let unregisterCallback = {
> > +      unregisterSucceeded: function() {},
> > +      unregisterFailed: function() {},
> 
> Nit: Maybe we should at least log an error here?

Right, and I was thinking we should have a follow up to actually display something to the end user if unregisterFailed?

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part1.v1.patch

janx: This patch should address your feedback comments!

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part2.wip.patch

Lin: I am trying to add a new mixin in the aboutdebugging module in order to mutualize the code used to debug a worker. Can I get your feedback on this patch? Thanks!

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part2.wip.patch

Forgot one file in the previous patch :/

Comment 16

[Lin Clark [:linclark]]

Comment on attachment 8737884 [details] [diff] [review]
bug1258902.part2.wip.patch

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

Would it make sense to switch this from a mixin to a utility function that is imported? The trend in React has been against using mixins (https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-components-94a0d2f9e750#.6bbqwbsp9). Generally, we're trying to move as many functions off of the component as possible and into their own pure functions.

Comment 17

[Jan Keromnes [:janx]]

Comment on attachment 8737870 [details] [diff] [review]
bug1258902.part1.v1.patch

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

(In reply to Julian Descottes [:jdescottes] from comment #13)
> janx: This patch should address your feedback comments!

It does! Thanks a lot. R+ (and a few more nits).

(In reply to Julian Descottes [:jdescottes] from comment #12)
> Thanks for the review. Particularly, thanks a lot for classname changes
> suggestions. They all make sense and I feel the code is much better now!

Cool! I'm glad you liked them :)

> > Nit: Cool CSS trick, but the "(" and ")" don't feel useful. Maybe we can do
> > without?
> > 
> 
> I applied your proposed changes, and bumped the padding to isolate the title
> from the scope and from the link. Regarding the "()" around unregister, my
> goal was not to show off a cool CSS trick :) They should actually be outside
> of the link and not clickable. I just wanted to isolate the "unregister"
> action from the scope's text, a way to warn the user that this is an action.

I think that "unregister" being a link, it doesn't need further visual separation (e.g. parentheses). However, the added padding is a nice touch.

> > Rant: I never liked shortcuts like "l10n", "i18n" or "a11y", because they're
> > not transparent, and they're not that helpful in saving
> > characters/keystrokes anyway. It's my opinion that the less background
> > knowledge we require for someone to understand our code, the better, and
> > there are better names for such functions e.g. "localize".
> > 
> > However, the rest of our codebase does use "l10n", so I'm not sure we'd
> > actually do more good than bad using a different name here. Your call, I'm
> > fine either way.
> 
> I decided to remove it for now, because it's not the purpose of this bug.
> When we make such a change it should be in all the aboutdebugging module at
> once. The other react modules in devtools seem to rely on
> "devtools/client/shared/l10n" which provides a getStr method. (
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/utils.
> js#9 and
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/
> components/census-header.js#24 )
> 
> What about doing something similar here? I can add it as a separate commit
> in this bug.

Even though I don't like the naming, I agree about staying consistent with the rest of the code base. Separate commit welcome if you want to do that.
 
> > > +  unregister: method(function() {
> > > +    let unregisterCallback = {
> > > +      unregisterSucceeded: function() {},
> > > +      unregisterFailed: function() {},
> > 
> > Nit: Maybe we should at least log an error here?
> 
> Right, and I was thinking we should have a follow up to actually display
> something to the end user if unregisterFailed?

Actually displaying something in the UI would be a great follow-up to logging the error, except if we reasonably expect this error not to happen (i.e. if the cost of surfacing that error in the UI is not worth it).

When would an unregister fail? Can a SW developer cause such a thing, or would Firefox internals always be at fault? (e.g. brief disk failure when trying to save the registration DB).

Then again maybe at some point we'll route errors back to the UI for all actions (Debug, Push, Start, unregister, etc).

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ +59,5 @@
>  }
>  
> +#service-workers .target-container {
> +  align-items: flex-start;
> +}

Nit: Why restrict this rule to just #service-workers?

@@ +86,2 @@
>  .target-details {
> +  margin: 3px 0;

Nit: If you add the `margin-top: 5px` to `.target-detail` suggested below, I think `margin: 0;` would work fine here.

@@ +86,3 @@
>  .target-details {
> +  margin: 3px 0;
> +  padding: 0 0 0 20px;

Nit: Not too happy with the 20px indent. Don't you like `padding: 0;`?

@@ +89,5 @@
> +  list-style-type: none
> +}
> +
> +.target-detail {
> +  font-size: 12px;

Nit: Please add a `margin-top: 5px;` here to space

::: devtools/client/aboutdebugging/components/worker-target.js
@@ +38,5 @@
> +      dom.div({ className: "target" },
> +        dom.div({ className: "target-name" }, target.name),
> +        isServiceWorker ? dom.ul({ className: "target-details" },
> +          dom.li({ className: "target-detail" },
> +            dom.strong(null, (Strings.GetStringFromName("scope"))),

Nit: No need for parentheses around `Strings.GetStringFromName("scope")`.

Comment 18

[Jan Keromnes [:janx]]

(In reply to Lin Clark [:linclark] from comment #16)
> Would it make sense to switch this from a mixin to a utility function that
> is imported? The trend in React has been against using mixins
> (https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-
> components-94a0d2f9e750#.6bbqwbsp9). Generally, we're trying to move as many
> functions off of the component as possible and into their own pure functions.

I wasn't aware that mixins were on their way out. Julian, sorry for asking you to replace your original module with a mixin.

However, it's still not a good idea to have a single monolithic `utils.js` file, and as agreed on IRC we should probably create a "modules/" folder next to "components/".

This gets into follow-up territory, but here is a list of everything we'd like to extract from about:debugging components, in order to avoid custom methods and help shrink their state:

- debugWorker(client, workerActor) // currently WorkerTarget.debug()
- getWorkerForms() // currently WorkersTab.getWorkerForms()
- debugAddon(...) // currently AddonTarget.debug(), will be rewritten in bug 1243460

Additionally, there are two state-changing methods which would need to be refactored:

- WorkersTab.update()
- AddonsTab.updateAddonsList() // will also be rewritten in bug 1243460

Both of these transform devtools actor forms into an intermediate representation like {icon, name, [url, workerActor, registrationActor, addonID]}. We should extract form-getting code into separate modules, but maybe we should also get rid of the intermediate representation and make our components use actor forms directly?

This would simply "form > intermediate representation > target component" into "form > target component". (These representations were created with the assumption there would be a central Target component, but it stopped being valuable when we started having several unrelated Target-specific components).

Comment 19

[Jan Keromnes [:janx]]

Attachment: service-worker-scope.png

Helen, we'd like to add more details to a Service Worker item in about:debugging.

Julian is adding a "Scope" line to them, with an "unregister" action (a link, as you suggested for bug 1260568).

In bug 1188981, I'd like to add a similar "Push Service" line, with actions like "edit" or "unsubscribe".

What do you think of Julian's layout for "Scope"? Do you have any improvement suggestions?

Comment 20

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part1.v2.patch (applied review comments)

Thanks for the review Jan, new version of the patch here.

Addressed the review comments +

> Nit: Why restrict this rule to just #service-workers?

As mentioned on IRC, I used this to maintain the icon & buttons aligned with the first line of the .target element. I've gone back and forth since then on the implementation and I'm not too unhappy with the last one, so submitting for review.

Basically every item of the target container is aligned on baseline. The icon overrides this to flex-start and we add a margin top to make it look the same as it used to. Asking for r? because this part changed a bit.

Comment 21

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part2.v1.patch

Thanks for the feedback Lin! As Jan commented, we are moving this method to a utility module based on your suggestion. 

Jan: I only created the worker module for now and did not change the model created by the update methods. I think the rest can and should be done in follow-ups.

Comment 22

[Jan Keromnes [:janx]]

Comment on attachment 8738294 [details] [diff] [review]
bug1258902.part1.v2.patch (applied review comments)

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

Thanks for addressing my comments and re-flagging me!

We're very close to landable patches, but please wait for Helen's UX feedback before moving foward.

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ +46,5 @@
>  
>  /* Targets */
>  
>  .targets {
>    margin-bottom: 25px;

Nit: We're adding more stuff to targets, maybe bump this to at least 35px?

@@ +54,1 @@
>    margin-top: 5px;

Nit: Maybe bump this to at least 8px?

@@ +54,5 @@
>    margin-top: 5px;
>    min-height: 34px;
>    display: flex;
>    flex-direction: row;
> +  /* .target might expand vertically due to .targe-details, align on baseline */

Nit: I find this comment hard to understand, do we really need it? Also "target-details".

@@ +60,5 @@
>  }
>  
>  .target-icon {
>    height: 24px;
> +  margin: 6px 5px 0 0;

Nit: Maybe bump the second value to at least 6 or 7px? Also please keep an eye on the Add-on icons, which seem a wee bit off now.

@@ +85,5 @@
> +}
> +
> +.target-detail {
> +  font-size: 12px;
> +  margin-top: 5px;

Nit: 5px feels a little cramped, maybe at least 7px?

@@ +93,5 @@
> +  cursor: pointer;
> +}
> +
> +.target-detail > :not(:first-child) {
> +  margin-left: 10px;

Nit: 10px feels a little far, maybe not more than 8px?

Comment 23

[Jan Keromnes [:janx]]

Comment on attachment 8738296 [details] [diff] [review]
bug1258902.part2.v1.patch

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

This patch mostly look good, but there are a few leftovers in worker-target.js.

Additionally, I'm a bit worried about the `worker` module (unrelated co-located functions), but let's keep it like that for now. I think we will refactor it anyway when we extract more methods from components, and/or when we start launching toolboxes in tabs with "about:devtools-toolbox" URLs (see also bug 1233463 and bug 1262144).

::: devtools/client/aboutdebugging/components/service-worker-target.js
@@ +31,5 @@
> +        dom.div({ className: "target-name" }, target.name),
> +        dom.ul({ className: "target-details" },
> +          dom.li({ className: "target-detail" },
> +            dom.strong(null, Strings.GetStringFromName("scope")),
> +            dom.span({ className: "serviceworker-scope" }, target.scope),

Nit: Maybe rename this className to "service-worker-scope"?

@@ +73,5 @@
> +      type: "push"
> +    });
> +  },
> +
> +  unregister() {

Nit: Maybe locally sort the "{debug,push,start,unregister}" methods alphabetically? (And eventually they'll have to figure before "render" due to bug 1258353).

::: devtools/client/aboutdebugging/components/worker-target.js
@@ +37,5 @@
>            disabled: debugDisabled
>          }, Strings.GetStringFromName("debug")) :
>          dom.button({
>            className: "start-button",
>            onClick: this.start

There is no `this.start` here, maybe remove this button?

@@ -73,2 @@
>      if (!this.isRunning()) {
> -      // If the worker is not running, we can't debug it.

Nit: Why remove this comment? (Here and in service-worker-target.js).

@@ +55,5 @@
>  
>    isRunning() {
>      // We know the target is running if it has a worker actor.
>      return !!this.props.target.workerActor;
>    },

Nit: Actually, only service workers can exist without a workerActor today.

Workers and shared workers should (in theory) always be running. What about removing `isRunning` from worker-target.js altogether?

::: devtools/client/aboutdebugging/components/workers-tab.js
@@ +13,5 @@
>  const TargetList = createFactory(require("./target-list"));
>  const WorkerTarget = createFactory(require("./worker-target"));
> +const ServiceWorkerTarget = createFactory(require("./service-worker-target"));
> +
> +const { getWorkerForms } = require("../modules/worker");

Nit: Simple required modules are above factories, please put this before `const Services`.

::: devtools/client/aboutdebugging/modules/worker.js
@@ +22,5 @@
> + *        worker actor form to debug
> + */
> +exports.debugWorker = function(client, workerActor) {
> +  if (!workerActor) {
> +    return;

Nit: This shouldn't really happen, but maybe log an error here?

Comment 24

[Julian Descottes [:jdescottes]]

I have patches including your review comments, waiting for Helen's feedback before submitting for review.

(In reply to Jan Keromnes [:janx] from comment #22)
> Comment on attachment 8738294 [details] [diff] [review]
> bug1258902.part1.v2.patch (applied review comments)
> 
> Review of attachment 8738294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +54,5 @@
> >    margin-top: 5px;
> >    min-height: 34px;
> >    display: flex;
> >    flex-direction: row;
> > +  /* .target might expand vertically due to .targe-details, align on baseline */
> 
> Nit: I find this comment hard to understand, do we really need it? Also
> "target-details".
> 

Right, removed it.

> @@ +60,5 @@
> >  }
> >  
> >  .target-icon {
> >    height: 24px;
> > +  margin: 6px 5px 0 0;
> 
> Nit: Maybe bump the second value to at least 6 or 7px? Also please keep an
> eye on the Add-on icons, which seem a wee bit off now.
> 

I have the exact same icon positioning between Nightly and my local build (both for addon & worker tabs). Can you confirm?

Comment 25

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8738153 [details]
service-worker-scope.png

(In reply to Jan Keromnes [:janx] from comment #19)
> Created attachment 8738153 [details]
> service-worker-scope.png
> 
> Helen, we'd like to add more details to a Service Worker item in
> about:debugging.
> 
> Julian is adding a "Scope" line to them, with an "unregister" action (a
> link, as you suggested for bug 1260568).
> 
> In bug 1188981, I'd like to add a similar "Push Service" line, with actions
> like "edit" or "unsubscribe".
> 
> What do you think of Julian's layout for "Scope"? Do you have any
> improvement suggestions?

Looking at the mockup, it currently seems redundant (I see https://serviceworke.rs/immediate-claim/service-worker.js twice.) I have no issues with the layout, other than that it seems like we're saying the same thing twice.

Are there instances where those two things would be different?

Comment 26

[Julian Descottes [:jdescottes]]

Thanks for the feedback!

In the mockup, they are slightly different:
- "https://serviceworke.rs/immediate-claim/service-worker.js": url of the JS file for the registered worker
- "https://serviceworke.rs/immediate-claim/": scope of all pages affected by this worker

Both are set by the developer when registering the worker :
> navigator.serviceWorker.register('/sw-test/sw.js', { scope: '/sw-test/' })

So far most service worker demos have the service worker JS at the root of the scope, but nothing prevents the developer from diverging from this. So I would keep displaying both here.

Comment 27

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part1.v3.patch (addressed review comments)

Jan: carrying over r+, applied all your comments except for the icon. I can't see any difference between the current display and the one we have here. Could you attach a screenshot of the positioning issues you are seeing?

Comment 28

[Julian Descottes [:jdescottes]]

Attachment: bug1258902.part2.v2.patch

Thanks for the feedback Jan. Here is an updated version that should address your review comments.

(In reply to Jan Keromnes [:janx] from comment #23)
> Comment on attachment 8738296 [details] [diff] [review]
> bug1258902.part2.v1.patch
> 
> ::: devtools/client/aboutdebugging/modules/worker.js
> @@ +22,5 @@
> > + *        worker actor form to debug
> > + */
> > +exports.debugWorker = function(client, workerActor) {
> > +  if (!workerActor) {
> > +    return;
> 
> Nit: This shouldn't really happen, but maybe log an error here?

I removed the check from the method. There is no incentive for now to start validating this parameter and not the other one. The caller can be responsible for ensuring the valid arguments are provided to this method.

Comment 29

[Jan Keromnes [:janx]]

Comment on attachment 8738756 [details] [diff] [review]
bug1258902.part2.v2.patch

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

Looks good to me! Thanks for addressing all my comments.

(In reply to Julian Descottes [:jdescottes] from comment #28)
> > ::: devtools/client/aboutdebugging/modules/worker.js
> > > +exports.debugWorker = function(client, workerActor) {
> > > +  if (!workerActor) {
> > > +    return;
> > 
> > Nit: This shouldn't really happen, but maybe log an error here?
> 
> I removed the check from the method. There is no incentive for now to start
> validating this parameter and not the other one. The caller can be
> responsible for ensuring the valid arguments are provided to this method.

Ok, fair enough. Service Workers do check their `workerActor` before calling this, and we can be reasonably confident that Dedicated and Shared Workers always have a `workerActor`, since no issue indicating otherwise has surfaced yet. Additionally, the latter will be reinforced when we start validating prop types.

(In reply to Julian Descottes [:jdescottes] from comment #24)
> > Also please keep an eye on the Add-on icons, which seem a wee bit off now.
> 
> I have the exact same icon positioning between Nightly and my local build
> (both for addon & worker tabs). Can you confirm?

Add-on icons still look a bit off in my Xvfb-based display (could it be an Xvfb-related problem?). I'll upload a screenshot shortly.

Comment 30

[Jan Keromnes [:janx]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #25)
> Looking at the mockup, it currently seems redundant (I see
> https://serviceworke.rs/immediate-claim/service-worker.js twice.) I have no
> issues with the layout, other than that it seems like we're saying the same
> thing twice.
> 
> Are there instances where those two things would be different?

(In reply to Julian Descottes [:jdescottes] from comment #26)
> In the mockup, they are slightly different:
> - "https://serviceworke.rs/immediate-claim/service-worker.js": url of the JS
> file for the registered worker
> - "https://serviceworke.rs/immediate-claim/": scope of all pages affected by
> this worker
> 
> Both are set by the developer when registering the worker :
> > navigator.serviceWorker.register('/sw-test/sw.js', { scope: '/sw-test/' })
> 
> So far most service worker demos have the service worker JS at the root of
> the scope, but nothing prevents the developer from diverging from this. So I
> would keep displaying both here.

As Julian said, there can be rare cases where these two things are different, but otherwise Service Worker entries will generally look like they show redundant info, which does seem a little weird. I'm not sure if there is a good way around that.

Maybe as a follow-up improvement, we could show only the scope URI, i.e. "/immediate-claim/" (instead of the full URL "https://serviceworke.rs/immediate-claim/")? That would remove most of the redundant text (the base URL "https://serviceworke.rs", which I think will always be the same in both script URL and scope).

Comment 31

[Jan Keromnes [:janx]]

Attachment: icon-position.png

Julian, here is how the target icons look on my system with both of your patches applied. As you can see, the Extension icons look a bit too low.

Comment 32

[Jan Keromnes [:janx]]

(In reply to Jan Keromnes [:janx] from comment #31)
> Created attachment 8739010 [details]
> icon-position.png
> 
> Julian, here is how the target icons look on my system with both of your
> patches applied. As you can see, the Extension icons look a bit too low.

Actually nevermind, please forget about my remark. The icons do seem to be in the same position as before your patches. My eye was misled by the shape of the Add-on icon, but that problem is unrelated to this bug.

Comment 33

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Jan Keromnes [:janx] from comment #30)
> Maybe as a follow-up improvement, we could show only the scope URI, i.e.
> "/immediate-claim/" (instead of the full URL
> "https://serviceworke.rs/immediate-claim/")? That would remove most of the
> redundant text (the base URL "https://serviceworke.rs", which I think will
> always be the same in both script URL and scope).

Working on this as a follow-up seems good — no need to block on it, it isn't doing any harm.

I almost feel like for quick readability, you might be able to do:

my-cool-worker.js                                            debug
scope/scope/scope/my-cool-worker.js    unregister

... assuming the file name is what people are really quickly glancing for.

Comment 34

[Julian Descottes [:jdescottes]]

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

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5515579f5802
https://hg.mozilla.org/integration/fx-team/rev/0559e1ea4660

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5515579f5802
https://hg.mozilla.org/mozilla-central/rev/0559e1ea4660

Comment 37

[Ryan VanderMeulen [:RyanVM]]

This inadvertently re-enabled browser_service_workers_timeout.js. Keep in mind that skip-if conditions apply to the test above them in the manifest :). Hopefully the test you added here hasn't been permafailing all this time ;).
https://hg.mozilla.org/integration/fx-team/rev/dfd50ecc5291

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfd50ecc5291

Comment 39

[Julian Descottes [:jdescottes]]

Attachment: demo_unregister.gif

For reference, the new unregister link in the current UI

Comment 40

[Will Bamberg [:wbamberg]]

I've added a bit on this: https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Unregistering_workers.

Let me know if this covers it.

Comment 41

[Julian Descottes [:jdescottes]]

Thanks Will, looks good to me! 

This feature is only available for service workers, so maybe we should update the wording. 

Actually the 3 last sections : "unregistering workers", "debugging workers", "sending push events" are implicitly talking about service workers ("debugging workers" could apply to other workers but the bits about the "Start" / "Push" button are service worker specific).

I just updated the page to mention service worker where needed. Feel free to revert if you think it is too heavy/hard to read.

Maybe we could also regroup "unregistering workers" and "sending push events", since both are really service worker specific.

Comment 42

[Will Bamberg [:wbamberg]]

Thanks for the suggestions jdescottes. I've made the changes you suggested, and am marking this one dev-doc-complete.

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking in case this reopens.