Closed
Bug 1257873
Opened 8 years ago
Closed 8 years ago
Split Addons and Workers into separate Target classes in about:debugging
Categories
(DevTools :: about:debugging, defect, P2)
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: janx, Assigned: janx)
References
Details
Attachments
(1 file)
22.05 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 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•8 years ago
|
Assignee: nobody → janx
Status: NEW → ASSIGNED
Comment 3•8 years ago
|
||
I'll take a closer look next week, but that approach sounds totally sane!
Comment 4•8 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]: ----------------------------------------------------------------- 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•8 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•8 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•8 years ago
|
||
Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=816b3181b870
Keywords: checkin-needed
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/216d9ae88a31
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•