remove `Heritage` helper in view-helpers.js

RESOLVED FIXED in Firefox 59

Status

P3
enhancement
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: zer0, Assigned: vi.le)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

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
Blocks: 1402832
(Assignee)

Comment 3

a year ago
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
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 8

a year ago
mozreview-review
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 9

a year ago
mozreview-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 hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
mozreview-review-reply
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

a year ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12e30ac0c41c
Remove `Heritage` from devtools helper view-helpers.js; r=pbro

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12e30ac0c41c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.