1378850 - Stop using sdk/core/heritage in DevTools webconsole hudservice

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P1)

Description

[Soledad Penades [:sole] [:spenades]]

Used in: devtools/client/webconsole/hudservice.js

More details to follow as we triage.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1378850 - Stop using sdk/core/heritage in DevTools webconsole hudservice;

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

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The attached patch:
- introduces `extend` in devtoolsutils.js (or is there a better place?)
- uses `extend` in webconsole

Honza

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

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)

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Matteo: please see question in comment #4

Honza

Comment 6

[Soledad Penades [:sole] [:spenades]]

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!

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 8

[Soledad Penades [:sole] [:spenades]]

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?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8884266 [details]
Bug 1378850 - Stop using sdk/core/heritage in DevTools webconsole hudservice;

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

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8884266 [details]
Bug 1378850 - Stop using sdk/core/heritage in DevTools webconsole hudservice;

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

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8884266 [details]
Bug 1378850 - Stop using sdk/core/heritage in DevTools webconsole hudservice;

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

Comment 13

[Soledad Penades [:sole] [:spenades]]

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 :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68c5012ba344
Stop using sdk/core/heritage in DevTools webconsole hudservice; r=sole,zer0

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8884266 [details]
Bug 1378850 - Stop using sdk/core/heritage in DevTools webconsole hudservice;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155212/diff/4-5/

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44ca093166f6
Backed out changeset 68c5012ba344 for ESlint no-unused-vars failures on a CLOSED TREE.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7289ba6c2ad1
Stop using sdk/core/heritage in DevTools webconsole hudservice; r=sole,zer0

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7289ba6c2ad1