Split Addons and Workers into separate Target classes in about:debugging

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: about:debugging
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: janx, Assigned: janx)

Tracking

(Blocks: 2 bugs)

47 Branch
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
The component in aboutdebugging/components/target.js is getting bigger, and it's amassing unrelated target-type specific code. To prevent incidental complexity, we should split the Target component into target-type specific sub-components (current candidates are AddonTarget and WorkerTarget).

Partly to blame is the central `target` object model, and more specifically its `target.type` string property that causes a lot of type-specific switches to appear. We should remove this property.
(Assignee)

Comment 1

2 years ago
Created attachment 8732224 [details] [diff] [review]
Split Addons and Workers into separate Target classes in about:debugging.

This patch splits Target into AddonTarget and WorkerTarget, and removed the `target.type` string property.

Also a few ESLint drive-by fixes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=816b3181b870
Attachment #8732224 - Flags: review?(poirot.alex)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8732224 [details] [diff] [review]
Split Addons and Workers into separate Target classes in about:debugging.

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

James, we talked on IRC about inheritance versus composition, and I said I'd flag you for feedback.

However, it turns out that there isn't much in common after all, except a bit of DOM rendering, so I ended up re-implementing two separate components (no inheritance or composition here).

I split components/target.js into components/addon-target.js and components/worker-target.js. The only common part is:

  render() {
    dom.div({ className: "target" },
      dom.img({
        className: "target-icon",
        role: "presentation",
        src: target.icon
      }),
      dom.div({ className: "target-details" },
        dom.div({ className: "target-name" }, target.name)
      ),
      dom.button({
        className: "debug-button",
        onClick: this.debug,
        disabled: debugDisabled,
      }, Strings.GetStringFromName("debug"))
    );
  }

Too small to be worth sharing across components, right?

So basically, I'm just flagging you for general React-style feedback.
Attachment #8732224 - Flags: feedback?(jlong)
(Assignee)

Updated

2 years ago
Assignee: nobody → janx
Status: NEW → ASSIGNED
I'll take a closer look next week, but that approach sounds totally sane!
Comment on attachment 8732224 [details] [diff] [review]
Split Addons and Workers into separate Target classes in about:debugging.

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

Works great. Thanks for coming up with this patch so promptly!

It feel great to see about:debugging not take the caveats of webide codebase.

I see that you are fixing various eslint issues, but there is still many:
/mnt/desktop/gecko/devtools/client/aboutdebugging/components/worker-target.js
   27:3   warning  render must be placed after isServiceWorker                react/sort-comp
   28:11  warning  'target' is missing in props validation                    react/prop-types
   28:19  warning  'debugDisabled' is missing in props validation             react/prop-types
   63:11  warning  'client' is missing in props validation                    react/prop-types
   63:19  warning  'target' is missing in props validation                    react/prop-types
   78:11  warning  'client' is missing in props validation                    react/prop-types
   78:19  warning  'target' is missing in props validation                    react/prop-types
   90:11  warning  'client' is missing in props validation                    react/prop-types
   90:19  warning  'target' is missing in props validation                    react/prop-types
  104:25  warning  'target' is missing in props validation                    react/prop-types
  104:32  warning  'target.workerActor' is missing in props validation        react/prop-types
  109:25  warning  'target' is missing in props validation                    react/prop-types
  109:32  warning  'target.registrationActor' is missing in props validation  react/prop-types

I don't know what is prop-types rule, but feel free to fix that in followup.

::: devtools/client/aboutdebugging/components/worker-target.js
@@ +105,5 @@
> +  },
> +
> +  isServiceWorker() {
> +    // We know the target is a service worker if it has a registration actor.
> +    return !!this.props.target.registrationActor;

This is weird to me. I don't fully get why you are trying to get rid of type attribute.

::: devtools/client/aboutdebugging/components/workers-tab.js
@@ -80,5 @@
>  
>      this.getWorkerForms().then(forms => {
>        forms.registrations.forEach(form => {
>          workers.service.push({
> -          type: "serviceworker",

It would more explicit to keep type attribute to distinguish workers.
Doing some duck typing around registrationActor being available or not seems unsafe. If the server unexpectedly can't provide form.actor it will be considered as a random worker. Ok, it will break things anyway, but it would be harder to figure out what is going on.
Attachment #8732224 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 5

2 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Works great.

\o/

> Thanks for coming up with this patch so promptly!

You're welcome.

> It feel great to see about:debugging not take the caveats of webide codebase.

Big +1, thanks for noticing this before it was too late!

> I see that you are fixing various eslint issues, but there is still many:

Yes, I don't think we enforce eslint on try yet, because bug 1245496 fixed everything, but a few new problems were able to sneak in since that.

> /mnt/desktop/gecko/devtools/client/aboutdebugging/components/worker-target.js
>    27:3   warning  render must be placed after isServiceWorker              
> react/sort-comp

I don't really like this rule, and filed bug 1258353 to question it.

>    28:11  warning  'target' is missing in props validation                  
> react/prop-types
>    28:19  warning  'debugDisabled' is missing in props validation           
> react/prop-types
>    63:11  warning  'client' is missing in props validation                  
> react/prop-types
>    63:19  warning  'target' is missing in props validation                  
> react/prop-types
>    78:11  warning  'client' is missing in props validation                  
> react/prop-types
>    78:19  warning  'target' is missing in props validation                  
> react/prop-types
>    90:11  warning  'client' is missing in props validation                  
> react/prop-types
>    90:19  warning  'target' is missing in props validation                  
> react/prop-types
>   104:25  warning  'target' is missing in props validation                  
> react/prop-types
>   104:32  warning  'target.workerActor' is missing in props validation      
> react/prop-types
>   109:25  warning  'target' is missing in props validation                  
> react/prop-types
>   109:32  warning  'target.registrationActor' is missing in props validation
> react/prop-types
> 
> I don't know what is prop-types rule, but feel free to fix that in followup.

It seems useful to start validating our props. I'll file a follow-up bug for this.

> > +  isServiceWorker() {
> > +    // We know the target is a service worker if it has a registration actor.
> > +    return !!this.props.target.registrationActor;
> 
> This is weird to me. I don't fully get why you are trying to get rid of type
> attribute.

I'm getting rid of "type" for a few reasons:
- We don't really need it
- It encourages type-specific code in common places instead of dedicated modules (remember bug 1023081?)
- I think it's better to check for what you actually need (e.g. a specific actor to talk to) instead of matching a vague "type" string

> >      this.getWorkerForms().then(forms => {
> >        forms.registrations.forEach(form => {
> >          workers.service.push({
> > -          type: "serviceworker",
> 
> It would more explicit to keep type attribute to distinguish workers.
> Doing some duck typing around registrationActor being available or not seems
> unsafe. If the server unexpectedly can't provide form.actor it will be
> considered as a random worker. Ok, it will break things anyway, but it would
> be harder to figure out what is going on.

Actually, if the server doesn't provide `form.actor` for the registration, it will still correctly show up under "Service Workers" in about:debugging, because registrations are already separated by getWorkerForms().

You would still be able to debug the Service Worker if one is running, but you would't have SW-specific features like `Push` or `Start`, which makes sense because they can't work without a `registrationActor` to talk to (actually, the `Start` button would still show, but wouldn't do anything, because it's visibility is not subject to `isServiceWorker()`. I think it should not be visible if `!target.registrationActor` a.k.a `!this.isServiceWorker()`).

If in the future we do need to further differentiate between worker types, then I think we should further split up worker-target.js (e.g. into service-worker-target.js or shared-worker-target.js, and maybe share the `openWorkerToolbox()` implementation with a mixin).

I really think that maintaining a custom "type" string-enum is not the right solution.
(Assignee)

Comment 6

2 years ago
Comment on attachment 8732224 [details] [diff] [review]
Split Addons and Workers into separate Target classes in about:debugging.

Feedback is not actually required, because the original question about inheritance is no longer relevant to this bug.
Attachment #8732224 - Flags: feedback?(jlong)
(Assignee)

Comment 7

2 years ago
Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=816b3181b870
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/216d9ae88a31
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.