Closed Bug 1378850 Opened 7 years ago Closed 7 years ago

Stop using sdk/core/heritage in DevTools webconsole hudservice

Categories

(DevTools :: General, enhancement, P1)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: sole, Assigned: Honza)

References

Details

(Whiteboard: [nosdk])

Attachments

(1 file)

Used in: devtools/client/webconsole/hudservice.js More details to follow as we triage.
The attached patch: - introduces `extend` in devtoolsutils.js (or is there a better place?) - uses `extend` in webconsole Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P3
Comment on attachment 8884266 [details] Bug 1378850 - Stop using sdk/core/heritage in DevTools webconsole hudservice; https://reviewboard.mozilla.org/r/155212/#review160254 r+ with the `console-output` updated as well (here or in a follow-up bug, it could be here since we're already in webconsole). ::: devtools/client/webconsole/hudservice.js:8 (Diff revision 1) > "use strict"; > > -const {Cc, Ci, Cu} = require("chrome"); > - > var WebConsoleUtils = require("devtools/client/webconsole/utils").Utils; > -var { extend } = require("sdk/core/heritage"); > +const { extend } = require("devtools/shared/DevToolsUtils"); As team we should definitely have a better naming convention, so that we don't have module in kebab case mixed with camel case or lower case; sigh. Beside that, if we want to include `extend` in some shared module it's OK, but we have to update the other inline usage we have (I recall only this one: http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#33)
Attachment #8884266 - Flags: review?(zer0) → review+
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #3) > r+ with the `console-output` updated as well (here or in a follow-up bug, it > could be here since we're already in webconsole). What do you mean by this? The console-output.js file is already using `extends` from DevToolsUtils. > As team we should definitely have a better naming convention, so that we > don't have module in kebab case mixed with camel case or lower case; sigh. Totally agree. > Beside that, if we want to include `extend` in some shared module it's OK, > but we have to update the other inline usage we have (I recall only this > one: Removed as part of this patch. Thanks! Honza
Priority: P3 → P1
Whiteboard: [nosdk]
Target Milestone: --- → Firefox 56
@Matteo: please see question in comment #4 Honza
Flags: needinfo?(zer0)
Sorry for the drop-by comment, but I thought we were trying to use ES6's `extends` and classes instead of the SDK's utility methods (or a copy of it). Is there a reason why we'd use this DevToolsUtils extends instead? thanks!
Flags: needinfo?(odvarko)
(In reply to Soledad Penades [:sole] [:spenades] from comment #6) > Sorry for the drop-by comment, but I thought we were trying to use ES6's > `extends` and classes instead of the SDK's utility methods (or a copy of > it). Is there a reason why we'd use this DevToolsUtils extends instead? I used ES6 class in patch for bug 1378854 and it's definitely the right way to go. However, in case of the Console panel, it would represent a lot of changes. And most of the (old) web-console code will be removed as soon as the new Console front-end is ready and on by default anyway. Honza
Flags: needinfo?(odvarko)
Thanks for the explanation! :) Talking to Gabriel last Friday, he told me there's already an instance of this function in the Devtools code: http://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/utils.js#29 I'm concerned about using a utility for something that is built-in the language already, and also about having duplicated code. Although I understand it doesn't make sense to refactor to use `class` and `extends` if the code is going to be removed. Could you please file a bug to remove the extends when the old console front-end is shipped? And also could we possibly just use one `extends` function?
(In reply to Soledad Penades [:sole] [:spenades] from comment #8) > Talking to Gabriel last Friday, he told me there's already an instance of > this function in the Devtools code: > http://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/ > utils.js#29 Nice catch, I removed the one from the inspector dir and used the one from DevToolsUtils everywhere. Please review changes in the patch. > Could you please file a bug to remove the extends when the old console > front-end is shipped? I just filled such bug today and so made an additional comment in it. See: Bug 1381834 - Remove old Console front-end Thanks! Honza
Comment on attachment 8884266 [details] Bug 1378850 - Stop using sdk/core/heritage in DevTools webconsole hudservice; https://reviewboard.mozilla.org/r/155212/#review164050 Thank you for all the changes! This looks very clean now :)
Attachment #8884266 - Flags: review?(sole) → review+
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/68c5012ba344 Stop using sdk/core/heritage in DevTools webconsole hudservice; r=sole,zer0
Backout by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/44ca093166f6 Backed out changeset 68c5012ba344 for ESlint no-unused-vars failures on a CLOSED TREE.
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7289ba6c2ad1 Stop using sdk/core/heritage in DevTools webconsole hudservice; r=sole,zer0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: needinfo?(zer0)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: