Closed Bug 1449188 Opened 4 years ago Closed 4 years ago

Split devtools/server/actors/object.js in smaller files

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Summary: Split https://searchfox.org/mozilla-central/source/devtools/server/actors/object.js in smaller files → Split devtools/server/actors/object.js in smaller files
TRY seems okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e6b1c7473b36eee670c882638c702d8a830827&selectedJob=170761502

I split the files using `hg cp` so we don't lose history, but this makes the patch hard to read :/
Comment on attachment 8963062 [details]
Bug 1449188 - Split devtools/server/actors/object.js in smaller files; .

https://reviewboard.mozilla.org/r/231914/#review237480

Did you tried running DAMP?
I would like to confirm this is not regressing performance.

Otherwise, I think we should start bringing hierarchy into this folder. (Look 62 files in this folder!)
We started doing so with inspector and highlighters folder for example.
I would suggest moving all actors that are used by only one actor into a folder whose name is the actor name.
It means moving property-iterator.js and symbol-iterator.js into devtools/server/actors/object/.
Then, I also dislike seeing "utils" folders with random stuff in it.
The new `object` directory would be a better fit for the grip.js helper module, I believe.
And the two actors that it uses, ArrayBufferActor and SymbolActor (and only itself use them) could be moved into devtools/server/actors/object/grip/.

::: devtools/server/actors/utils/grip.js:1551
(Diff revision 1)
> -exports.SymbolActor = SymbolActor;
> -exports.createValueGrip = createValueGrip;
> -exports.stringIsLong = stringIsLong;
> -exports.longStringGrip = longStringGrip;
> -exports.arrayBufferGrip = arrayBufferGrip;
> +  enumArrayProperties,
> +  enumMapEntries,
> +  enumObjectProperties,
> +  enumSetEntries,
> +  enumWeakMapEntries,
> +  enumWeakSetEntries,

All these enum helpers should be inlined into property-iterator.js
This isn't used by any other module.

::: devtools/server/actors/utils/grip.js:1558
(Diff revision 1)
> +  getPromiseState,
> +  isArray,
> +  isTypedArray,
> +  longStringGrip,
> +  stringify,
> +  stringIsLong,

`stringify` would be better inlined into object.js
It is only used by object.js.
If you are concerned that it would hurt ObjectActor's comprehension, it would be better to move it into a distinct module. Otherwise grip.js which will tend to be full of random things.
Then, isArray, isTypeArray, getPromiseState may move along with `stringify` function. I'm not sure it is used outside of object.js and stringify usages.
Attachment #8963062 - Flags: review?(poirot.alex)
Comment on attachment 8963062 [details]
Bug 1449188 - Split devtools/server/actors/object.js in smaller files; .

https://reviewboard.mozilla.org/r/231914/#review237480

> All these enum helpers should be inlined into property-iterator.js
> This isn't used by any other module.

Some of them are used by DebuggerServer.ObjectActorPreviewers, so even if we move stringify back to object.js, we should either keep them in object.js or expose them in property-iterator

> `stringify` would be better inlined into object.js
> It is only used by object.js.
> If you are concerned that it would hurt ObjectActor's comprehension, it would be better to move it into a distinct module. Otherwise grip.js which will tend to be full of random things.
> Then, isArray, isTypeArray, getPromiseState may move along with `stringify` function. I'm not sure it is used outside of object.js and stringify usages.

yeah, I wasn't sure of moving things to grip.js, I did it in the end because it felt better to only have actor direct code in the file, but we're only moving the mess to another file :)
Comment on attachment 8963062 [details]
Bug 1449188 - Split devtools/server/actors/object.js in smaller files; .

https://reviewboard.mozilla.org/r/231914/#review237480

Thanks for the revierw Alex. I'll run DAMP and if it shows no regression will make the adjustments you suggest.
Comment on attachment 8963062 [details]
Bug 1449188 - Split devtools/server/actors/object.js in smaller files; .

https://reviewboard.mozilla.org/r/231914/#review238084

Thanks a lot, that shuffle of code looks really benefitial!
I find the association of actor + actorGrip really neat. This is surely going to help us refactor these actors to protocol.js.

Could you verify that DAMP is still OK with the latest version?

::: devtools/server/actors/long-string.js:18
(Diff revision 2)
>   * at the server's discretion.
>   *
>   * @param string String
>   *        The string.
>   */
>  function LongStringActor(string) {

Speaking about protocol.js refactoring...
I didn't realized that we were having two long string actor implementation. An old fashion one, here, and another, in devtools/server/actors/string.js using protocol.js! The two looks the same, except that one it will be harder to use the old fashion one from a procotol.js actor and vice versa.

So, I know that this actor isn't 100% specific to object, but may be it will reduce the confusion if we keep this file in devtools/server/actors/object/long-string.js.
Also, may be a comment saying that there is a newer, protocol.js version in string.js would help.

::: devtools/server/actors/long-string.js:44
(Diff revision 2)
>     */
>    grip: function() {
>      return {
>        "type": "longString",
>        "initial": this.string.substring(
>          0, DebuggerServer.LONG_STRING_INITIAL_LENGTH),

Otherwise, I was looking at long-string for another reason. We are using properties set on DebuggerServer for historical reasons. We weren't having modules in devtools for a long time and we ended up sharing attributes like that...

Now, may be it is better to wait for all long string usages to be refactored to protocol.js before moving all these LONG_STRING* attribute to string.js...

::: devtools/server/actors/object/utils.js:52
(Diff revision 2)
>   *         Whether the value is non-primitive.
>   */
>  function isObject(value) {
>    const type = typeof value;
>    return type == "object" ? value !== null : type == "function";
>  }

This `isObject` function is only used by stringifiers.js and can be moved there.
Attachment #8963062 - Flags: review?(poirot.alex) → review+
Comment on attachment 8963062 [details]
Bug 1449188 - Split devtools/server/actors/object.js in smaller files; .

https://reviewboard.mozilla.org/r/231914/#review238084

will do

> Speaking about protocol.js refactoring...
> I didn't realized that we were having two long string actor implementation. An old fashion one, here, and another, in devtools/server/actors/string.js using protocol.js! The two looks the same, except that one it will be harder to use the old fashion one from a procotol.js actor and vice versa.
> 
> So, I know that this actor isn't 100% specific to object, but may be it will reduce the confusion if we keep this file in devtools/server/actors/object/long-string.js.
> Also, may be a comment saying that there is a newer, protocol.js version in string.js would help.

you mean we could keep the old-style longString actor in actors/object.js right ?
Also, yes, I discovered we were having two longString actors. I'll file a bug to get rid of one of them (ideally, the old one which do not use protocol.js)

> Otherwise, I was looking at long-string for another reason. We are using properties set on DebuggerServer for historical reasons. We weren't having modules in devtools for a long time and we ended up sharing attributes like that...
> 
> Now, may be it is better to wait for all long string usages to be refactored to protocol.js before moving all these LONG_STRING* attribute to string.js...

yes, good idea since we already did that in the patch for previewers

> This `isObject` function is only used by stringifiers.js and can be moved there.

good catch, i'll move it
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> you mean we could keep the old-style longString actor in actors/object.js
> right ?

I thought about moving it in its own module still, in actors/object/long-string.js.
But keeping it in actors/object.js WFM.

> Also, yes, I discovered we were having two longString actors. I'll file a
> bug to get rid of one of them (ideally, the old one which do not use
> protocol.js)

Yes, we should remove the one you are modifying and keep actors/string.js.
But I don't know if we can use string.js's one easily from an old style actor?
(i.e. we may have to wait to refactor all actors using the old one to protocol.js first)
okay thanks for the info. 

> we may have to wait to refactor all actors using the old one to protocol.js first

yes, probably
Blocks: 1450977
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> Anything I'm missing Alex ?

No, it looks good!
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 72fd006ddd5221a43e8114f5248c8f5c252b5338 -d e4a3e7fb39f7: rebasing 455911:72fd006ddd52 "Bug 1449188 - Split devtools/server/actors/object.js in smaller files; r=ochameau." (tip)
merging devtools/server/actors/object.js and devtools/server/actors/array-buffer.js to devtools/server/actors/array-buffer.js
merging devtools/server/actors/object.js
merging devtools/server/actors/object.js and devtools/server/actors/object/long-string.js to devtools/server/actors/object/long-string.js
merging devtools/server/actors/object.js and devtools/server/actors/object/previewers.js to devtools/server/actors/object/previewers.js
merging devtools/server/actors/object.js and devtools/server/actors/object/property-iterator.js to devtools/server/actors/object/property-iterator.js
merging devtools/server/actors/object.js and devtools/server/actors/object/stringifiers.js to devtools/server/actors/object/stringifiers.js
merging devtools/server/actors/object.js and devtools/server/actors/object/symbol-iterator.js to devtools/server/actors/object/symbol-iterator.js
merging devtools/server/actors/object.js and devtools/server/actors/object/symbol.js to devtools/server/actors/object/symbol.js
merging devtools/server/actors/object.js and devtools/server/actors/object/utils.js to devtools/server/actors/object/utils.js
warning: conflicts while merging devtools/server/actors/array-buffer.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/actors/object.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/actors/object/long-string.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/actors/object/previewers.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/actors/object/property-iterator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/actors/object/stringifiers.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/actors/object/symbol.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/actors/object/utils.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e930b5aad58
Split devtools/server/actors/object.js in smaller files; r=ochameau.
https://hg.mozilla.org/mozilla-central/rev/1e930b5aad58
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.