1393464 - remove `Heritage` helper in view-helpers.js

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

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

In bug 1378847 we're using the DevTools' extend function, that is duplicated in this file. We should remove or merge it.

http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/devtools/client/shared/widgets/view-helpers.js#20-37

Comment 1

[Patrick Brosset <:pbro>]

This looks like a technical refactoring that should be fairly easy for someone having a bit of JS experience.
Matteo, do you mind making this bug a bit more actionable please? Like adding links to the files that need to be changed, and detailing exactly what is needed.

Comment 2

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

Heritage's helper object was made to provide a couple of functionality inherited from SDK; but they're not needed anymore: `Heritage.extend` can be replaced by "devtools/shared/extend" [1], and `Heritage.getOwnPropertyDescriptors` is now implemented natively as `Object.getOwnPropertyDescriptors` [2].

Therefore `Heritage` should be removed, and its usage replaced by the functions above.

Here the occurrence we have in our codebase: http://searchfox.org/mozilla-central/search?q=Heritage&case=true&path=devtools


[1] http://searchfox.org/mozilla-central/source/devtools/shared/extend.js
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyDescriptors

Comment 3

[Vincent Lequertier]

Hello, can I work on this?

Comment 4

[Patrick Brosset <:pbro>]

Thank you Vincent for your interest and sorry about the delay. I'll assign the bug to you now. Feel free to submit patches and ask me for reviews.
Is comment 2 enough for you to start? If not, feel free to ask for more information here on this bug.

Comment 5

[Vincent Lequertier]

Attachment: Bug 1393464 - Remove `Heritage` from devtools helper view-helpers.js;

Calling "devtools/shared/extend" instead.

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

Comment 6

[Vincent Lequertier]

Thank you. Here is a patch :-)

Comment 7

[Patrick Brosset <:pbro>]

Thanks Vincent for this! A lot of changes.
I've pushed your commit to TRY to make sure it doesn't break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdd6740f838ff01331a7e97668643a8c675522c8
I've also test locally the main panels you changed, they still seem to work fine.
So I think we should be good here. Let's just wait for TRY to end, and then we can land this.
I'll R+ the change in the meantime.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8939947 [details]
Bug 1393464 - Remove `Heritage` from devtools helper view-helpers.js;

https://reviewboard.mozilla.org/r/210230/#review216118

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8939947 [details]
Bug 1393464 - Remove `Heritage` from devtools helper view-helpers.js;

https://reviewboard.mozilla.org/r/210230/#review216124

::: devtools/client/performance/performance-controller.js:20
(Diff revision 1)
>  });
>  var { Task } = require("devtools/shared/task");
> -/* exported Heritage, ViewHelpers, WidgetMethods, setNamedTimeout, clearNamedTimeout */
> -var { Heritage, ViewHelpers, WidgetMethods, setNamedTimeout, clearNamedTimeout } = require("devtools/client/shared/widgets/view-helpers");
> +/* exported ViewHelpers, WidgetMethods, setNamedTimeout, clearNamedTimeout */
> +var { ViewHelpers, WidgetMethods, setNamedTimeout, clearNamedTimeout } = require("devtools/client/shared/widgets/view-helpers");
>  var { PrefObserver } = require("devtools/client/shared/prefs");
> -
> +const { extend } = require("devtools/shared/extend");

Ah, there is an ESLint job on TRY and it just failed on this line, saying that `extend` is being defined but is never used.
In reality it is, but just not in this file.
So I think you'll need to add a comment line just above this line like this:

```
/* exported extend */
const { extend } = require("devtools/shared/extend");
```

Comment 10

[Vincent Lequertier]

Comment on attachment 8939947 [details]
Bug 1393464 - Remove `Heritage` from devtools helper view-helpers.js;

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

Comment 11

[Vincent Lequertier]

Comment on attachment 8939947 [details]
Bug 1393464 - Remove `Heritage` from devtools helper view-helpers.js;

https://reviewboard.mozilla.org/r/210230/#review216124

> Ah, there is an ESLint job on TRY and it just failed on this line, saying that `extend` is being defined but is never used.
> In reality it is, but just not in this file.
> So I think you'll need to add a comment line just above this line like this:
> 
> ```
> /* exported extend */
> const { extend } = require("devtools/shared/extend");
> ```

Should be good now, thank you. On my computer I saw a lot of eslint warnings so I thought all warnings would be ignored somehow.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12e30ac0c41c
Remove `Heritage` from devtools helper view-helpers.js; r=pbro

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/12e30ac0c41c