1325505 - Consider listing what container the worker is in on about:debugging page

Status: NEW

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

Description

[Jonathan Kingston [:jkt] he/him]

about:debugging#workers lists all workers even if they are in a container however it doesn't inform the user if they are in a container

However about:serviceworkers lists the 'userContextId' which is probably just as confusing too for users.
Origin: https://developer.chrome.com^userContextId=2

I suggest making the user aware of the container name and icon as custom containers can allow the user to set multiple containers with the same icon or same name.

This could be a source of confusion if the user is trying to debug the wrong container for their website.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1325505 - Adding in container name and icon into about:debugging

Review commit: https://reviewboard.mozilla.org/r/100720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100720/

Comment 2

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8821429 [details]
Bug 1325505 - Adding in container name and icon into about:debugging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100720/diff/1-2/

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8821429 [details]
Bug 1325505 - Adding in container name and icon into about:debugging

https://reviewboard.mozilla.org/r/100720/#review101384

Upload the missing file. The rest looks good. Ask a review to somebody working on devtools.

::: devtools/client/aboutdebugging/components/workers/panel.js:79
(Diff revision 2)
>          workers.service.push({
>            icon: WorkerIcon,
>            name: form.url,
>            url: form.url,
>            scope: form.scope,
> +          principal: form.principal,

you don't need the full principal but just originAttributes. Pass originAttributes instead.

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:198
(Diff revision 2)
>      }, Strings.GetStringFromName("unregister"));
>    },
>  
> +  renderContainer(target) {
> +    if (target.principal.originAttributes.userContextId) {
> +      let container = target.principal.originAttributes.userContextId;

call it userContextId.

::: devtools/server/actors/worker.js:415
(Diff revision 2)
>  
>      let isE10s = Services.appinfo.browserTabsRemoteAutostart;
>      return {
>        actor: this.actorID,
>        scope: registration.scope,
> +      principal: registration.principal,

same here, just originAttributes

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:10
(Diff revision 2)
>  this.EXPORTED_SYMBOLS = ["ContextualIdentityService"];
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +const DEBUG = false;
> +
> +if ("@mozilla.org/xre/app-info;1" in Cc) {

Cool, but move it into init().
plus, why this if()? Just do:

  let runtime = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
  if (runtime.processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
    // Refuse to run in child processes.
    throw new Error("You cannot use ContextualIdentites in child processes!");
  }

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:124
(Diff revision 2)
> +    for (let message of kMessages) {
> +      ppmm.addMessageListener(message, this);
> +    }
> +  },
> +
> +  unregisterListeners() {

where do you call unregisterListeners?
probably you want to register an observer to xpcom-shutdown.
Or you can try to use AsyncShutdown.

::: toolkit/content/aboutServiceWorkers.js:12
(Diff revision 2)
>  
> -Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import("resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +  "resource://gre/modules/Promise.jsm");

use the same indentation approach for these 2 lazy getters.

::: toolkit/content/aboutServiceWorkers.js:14
(Diff revision 2)
>  Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import("resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +  "resource://gre/modules/Promise.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ContextualIdentityServiceContentHandler",
> +                                  "resource://gre/modules/ContextualIdentityServiceContentHandler.jsm");

This file is not included in the patch :)

::: toolkit/content/aboutServiceWorkers.js:77
(Diff revision 2)
> +}
> +
> +function getContainerInfo(info) {
> +  return new Promise((resolve) => {
> +    if (info.principal.originAttributes.userContextId) {
> +      let container = info.principal.originAttributes.userContextId;

call it userContextId

::: toolkit/content/aboutServiceWorkers.js:83
(Diff revision 2)
> +      Promise.all([
> +        ContextualIdentityServiceContentHandler.getIdentityFromId(container),
> +        ContextualIdentityServiceContentHandler.getUserContextLabel(container)
> +      ]).then(([identity, label]) => {
> +        resolve({identity, label});
> +      }).catch(e => {

this catch() should not be needed.

Comment 4

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8821429 [details]
Bug 1325505 - Adding in container name and icon into about:debugging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100720/diff/2-3/

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8821429 [details]
Bug 1325505 - Adding in container name and icon into about:debugging

https://reviewboard.mozilla.org/r/100720/#review101812

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:39
(Diff revision 3)
>        scope: PropTypes.string.isRequired,
>        // registrationActor can be missing in e10s.
>        registrationActor: PropTypes.string,
> -      workerActor: PropTypes.string
> +      workerActor: PropTypes.string,
> +      originAttributes: PropTypes.shape({
> +        userContextId: PropTypes.string

PropTypes.number

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:122
(Diff revision 3)
> +    for (let message of kMessages) {
> +      ppmm.addMessageListener(message, this);
> +    }
> +  },
> +
> +  unregisterListeners() {

you don't call this method. Add a xpcom-shutdown observer.

::: toolkit/components/contextualidentity/ContextualIdentityServiceContentHandler.jsm:42
(Diff revision 3)
> +    }
> +  }
> +
> +  _getId() {
> +    return Cc["@mozilla.org/uuid-generator;1"]
> +             .getService(Ci.nsIUUIDGenerator).generateUUID().toString();

You can also just do:

_lastId: 0;
_getId() {
  return ++this._lastId;
}

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8821429 [details]
Bug 1325505 - Adding in container name and icon into about:debugging

https://reviewboard.mozilla.org/r/100720/#review101908

Thanks for the patch! Looks good for the most part. Have a look at my comments below.

I'll need to spend more time testing sw with containers. about:debugging seemed quite confused when registering the same worker in a regular tab and then in a container tab.

::: devtools/client/aboutdebugging/aboutdebugging.css:126
(Diff revision 3)
>  .target-detail > :not(:first-child) {
>    margin-left: 8px;
>  }
>  
> +.target-detail > .container-name {
> +  margin-left: 0;

The icon sits really close to the label right now, could we remove this rule? Or use something in-between 0 and 8px if you feel 8px is too much.

::: devtools/client/aboutdebugging/components/workers/panel.js:109
(Diff revision 3)
>                // e10s, and registrations are not forwarded to other processes until they
>                // reach the activated state. Augment the worker as a registration worker to
>                // display it in aboutdebugging.
>                worker.scope = form.scope;
>                worker.active = false;
>                workers.service.push(worker);

We have a fallback here that creates a service worker based on a worker. Might need to add an originAttributes object here.

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:199
(Diff revision 3)
>        onClick: this.unregister,
>        className: "unregister-link"
>      }, Strings.GetStringFromName("unregister"));
>    },
>  
> +  renderContainer(target) {

nit: we usually don't pass target but get it from this.props.

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:200
(Diff revision 3)
>        className: "unregister-link"
>      }, Strings.GetStringFromName("unregister"));
>    },
>  
> +  renderContainer(target) {
> +    if (target.originAttributes.userContextId) {

either protect this with if(target.originAttributes && target.originAttributes.userContextId) or set isRequired on originAttributes in propTypes

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:207
(Diff revision 3)
> +      let identity = ContextualIdentityService.getIdentityFromId(userContextId);
> +      if (identity) {
> +        let label = ContextualIdentityService.getUserContextLabel(userContextId);
> +
> +        return dom.li({
> +            className: "target-detail",

eslint: indentation is off

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:213
(Diff revision 3)
> +            "data-identity-icon": identity.icon,
> +            "data-identity-color": identity.color
> +          },
> +          dom.strong(null, Strings.GetStringFromName("container")),
> +          dom.span({
> +            className: "userContext-icon"

Since the classname doesn't follow the same conventions as the other classes used in aboutdebugging, could we have a comment here to explain that userContext-icon is an external classname used with usercontext.css in order to get the container icon?

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:222
(Diff revision 3)
> +            title: label
> +          }, label)
> +        );
> +      }
> +    }
> +    return '';

eslint: no single quotes

::: devtools/client/locales/en-US/aboutdebugging.properties:20
(Diff revision 3)
>  # LOCALIZATION NOTE (start):
>  # This string is displayed as a label of the button that starts a service worker.
>  start = Start
>  
>  scope = Scope
> +container = Container

Please add a localization note to make it clear that this refers to tab containers 

eg. This string is displayed when a service worker has been registered in a specific tab container.

Comment 8

[Julian Descottes [:jdescottes]]

Jonathan, do you want to follow up on your initial patch or is this up for grabs?

Comment 9

[Jonathan Kingston [:jkt] he/him]

Hey sorry Julian,

This is certainly very low on my priority list. The work was basically done however Bug 1328246 seems to be an issue and I wasn't able to figure out where the issue is. I would rather not it be showing the wrong container.

Thanks

Comment 10

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020