Closed Bug 1393464 Opened 3 years ago Closed 2 years ago

remove `Heritage` helper in view-helpers.js

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: zer0, Assigned: vi.le)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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.
Flags: needinfo?(zer0)
Priority: -- → P3
Severity: normal → enhancement
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
Flags: needinfo?(zer0)
Summary: remove/merge the duplicated extend helper in view-helpers.js → remove `Heritage` helper in view-helpers.js
Hello, can I work on this?
Flags: needinfo?(zer0)
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.
Assignee: nobody → vi.le
Status: NEW → ASSIGNED
Flags: needinfo?(zer0)
Thank you. Here is a patch :-)
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 on attachment 8939947 [details]
Bug 1393464 - Remove `Heritage` from devtools helper view-helpers.js;

https://reviewboard.mozilla.org/r/210230/#review216118
Attachment #8939947 - Flags: review?(pbrosset) → review+
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 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.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12e30ac0c41c
Remove `Heritage` from devtools helper view-helpers.js; r=pbro
https://hg.mozilla.org/mozilla-central/rev/12e30ac0c41c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.