Closed Bug 1361332 Opened 7 years ago Closed 7 years ago

Stop using sdk/util/object in DevTools

Categories

(DevTools :: General, enhancement, P1)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: pbro, Assigned: jdescottes)

References

Details

(Whiteboard: [nosdk])

Attachments

(1 file)

In bug 1350645 we're moving away from the Addon-SDK.

We use sdk/util/object in various places in DevTools:
http://searchfox.org/mozilla-central/search?q=sdk%2Futil%2Fobject&path=devtools

We should start using something else instead in these locations.
Luckily `object` module it should be easy to replace. Even if it's not exactly the same, `merge` should be able to be replaced by `Object.assign` (it is, indeed, what we're commonly use nowadays).

Object extend could be a little bit more complicate, depends by the usage, if we actually want a deep copy of the property's descriptor or not. If our usage doesn't require that, it would be easier. Otherwise, we should find a module in devtools where to add this utility function.
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Used in:

devtools/client/performance/modules/logic/waterfall-utils.js
devtools/server/actors/script.js
devtools/shared/fronts/inspector.js
devtools/shared/protocol.js
Used (lazy) also in:

devtools/server/actors/performance-recording.js
devtools/server/actors/performance.js
devtools/server/performance/recorder.js
devtools/shared/fronts/performance-recording.js
devtools/shared/fronts/profiler.js
devtools/shared/performance/recording-utils.js
Flags: qe-verify-
Whiteboard: [nosdk]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → Firefox 56
After reviewing the current usage of extend() in the DevTools codebase, I think it could have been replaced by merge() everywhere:
- devtools/server/performance/recorder.js : 
  - called on empty objects
  - called on options objects which are being augmented with additional properties. 
    Can't see why we would like the base option to be used as the prototype here
- devtools/shared/fronts/profiler.js: 
  - called on { stringify: true } to create an options object, doubt extend is useful here
- devtools/client/performance/modules/logic/waterfall-utils.js: called on simple objects with a single String property "name"

Finally:
- devtools/shared/performance/recording-utils.js: not actually used
- devtools/server/actors/performance.js : not actually used

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68ea77450326a77264447a1b1e94d2490912aeb6
Comment on attachment 8892124 [details]
Bug 1361332 - replace sdk/util/object by Object.assign in devtools;

https://reviewboard.mozilla.org/r/163112/#review168660

Thanks for working on this Julian! It looks good, r+ with the comments below addressed.

::: devtools/client/performance/modules/logic/waterfall-utils.js:21
(Diff revision 2)
>   * The marker is seeded with values from `marker`.
>   * @param object marker
>   * @return object
>   */
>  function createParentNode(marker) {
> -  return extend(marker, { submarkers: [] });
> +  return Object.assign(marker, { submarkers: [] });

One major difference between `extend` and `assign`/`merge` is that the latter actually modifying the `target` given (the first argument), where the first is using the first argument for create a new object.

So, a 1:1 replacement would be:
```js
return Object.assign(Object.create(marker),  { submarkers: [] });
```

Now, I read your comment: if you're absolutely sure that using the first argument as prototype in this specific case it doesn't make any difference at all, I would suggest at least to ensure that we're not modifying the argument given to avoid undesiderable side effects (now or in the future) that would be difficult to debug. So, in that specific case, I would suggest to modify this line into:

```js
return Object.assign({}, marker, { submarkers: [] });
```

Since the order is important, but if you're absolutely sure that `marker` will never have a `submarkers` property already, you can also have:

```js
return Object.assign({ submarkers: [] }, marker);
```

::: devtools/client/performance/modules/logic/waterfall-utils.js:58
(Diff revision 2)
>      // Extend the marker with extra properties needed in the marker tree
>      let extendedProps = { index: i };
>      if (collapsible) {
>        extendedProps.submarkers = [];
>      }
> -    curr = extend(curr, extendedProps);
> +    curr = Object.assign(curr, extendedProps);

You can remove the assignment here and have just:
```js
Object.assign(curr, extendedProps);
```

::: devtools/server/actors/script.js:2042
(Diff revision 2)
>      return { from: this.actorID,
>               actors: result };
>    }
>  });
>  
> -ThreadActor.prototype.requestTypes = object.merge(ThreadActor.prototype.requestTypes, {
> +ThreadActor.prototype.requestTypes = Object.assign(ThreadActor.prototype.requestTypes, {

Same as above, you can simplify removing the assignment.

::: devtools/shared/fronts/inspector.js:126
(Diff revision 2)
>        form.nodeValue = form.incompleteValue ? null : form.shortValue;
>      }
>  
>      // Shallow copy of the form.  We could just store a reference, but
>      // eventually we'll want to update some of the data.
> -    this._form = object.merge(form);
> +    this._form = Object.assign(form);

I believe that is a bug. You properly adapted the original code, but the original code doesn't actually perform any shallow copy, it just returns the same object, it's like having:
```js
this._form = this._form;
```

If we want to provide a shallow clone of the object, we should try with:

```js
this._form = Object.assign({}, this._form);
```
Attachment #8892124 - Flags: review?(zer0) → review+
Comment on attachment 8892124 [details]
Bug 1361332 - replace sdk/util/object by Object.assign in devtools;

https://reviewboard.mozilla.org/r/163112/#review168660

> One major difference between `extend` and `assign`/`merge` is that the latter actually modifying the `target` given (the first argument), where the first is using the first argument for create a new object.
> 
> So, a 1:1 replacement would be:
> ```js
> return Object.assign(Object.create(marker),  { submarkers: [] });
> ```
> 
> Now, I read your comment: if you're absolutely sure that using the first argument as prototype in this specific case it doesn't make any difference at all, I would suggest at least to ensure that we're not modifying the argument given to avoid undesiderable side effects (now or in the future) that would be difficult to debug. So, in that specific case, I would suggest to modify this line into:
> 
> ```js
> return Object.assign({}, marker, { submarkers: [] });
> ```
> 
> Since the order is important, but if you're absolutely sure that `marker` will never have a `submarkers` property already, you can also have:
> 
> ```js
> return Object.assign({ submarkers: [] }, marker);
> ```

Replaced by ({}, marker, {submarkers:[]}), I agree modifying the argument is unexpected (and doesn't match the jsdoc). For reference this was "fine" because this method is always called as follows:  `WaterfallUtils.createParentNode({ name: "(root)" })`

> I believe that is a bug. You properly adapted the original code, but the original code doesn't actually perform any shallow copy, it just returns the same object, it's like having:
> ```js
> this._form = this._form;
> ```
> 
> If we want to provide a shallow clone of the object, we should try with:
> 
> ```js
> this._form = Object.assign({}, this._form);
> ```

Good catch, I simply replaced all calls to object.merge with Object.assign. I hope nothing was relying on the fact that it was not a shallow copy. Will push to try again.
Fixed the last eslint violation, try looks green so far. Landing.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a02ac4abd5e
replace sdk/util/object by Object.assign in devtools;r=zer0
https://hg.mozilla.org/mozilla-central/rev/0a02ac4abd5e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.