Closed Bug 1378850 Opened 2 years ago Closed 2 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
https://hg.mozilla.org/mozilla-central/rev/7289ba6c2ad1
Status: ASSIGNED → RESOLVED
Closed: 2 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.