Split devtools/shared/client/main.js into multiple components

RESOLVED FIXED in Firefox 58

Status

P1
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Tracking

Trunk
Firefox 58
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
The file is more than 3000 lines long atm (http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/devtools/shared/client/main.js) and contains multiple "classes" that could live perfectly in their own file.
(Assignee)

Updated

2 years ago
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Updated

2 years ago
Blocks: 1403106
(Assignee)

Updated

2 years ago
Blocks: 1383711
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
and here is a try push with the patch applied https://treeherder.mozilla.org/#/jobs?repo=try&revision=62c1f7a094f2e80e6928662e9fda0ad83dd4039e.

Alex, this is a big review, but it's mostly moving things around.
Please feel free to tell me if I might do it another way (using hg copy for instance)
> I don't think there's a good way of keeping history on new files,
> unless we use hg copy from main.js. Maybe that would be better than
> nothing, but I'm not sure about this.

I've typically used 'hg copy' and then deleted the rest when splitting a file up to keep blame. In those cases it's been when splitting 1 file into just a few, but we may want to still do it even if splitting into a bunch.
Comment on attachment 8913203 [details]
Bug 1403895 - split main.js in mulitple files; .

https://reviewboard.mozilla.org/r/184602/#review189778

* Yes, it would be worth trying hg copy on two new files to see how it goes.
* I would keep Request() definition within debugger-client as it is private to DebbuggerClient.
* Use loader.lazyRequireGetter() to help save some module loading
* It may be worth following up to require the precise client module everywhere to save even more module loading. Each new module introduce a small overhead, so we should mitigate this by, on the other hand, load less JS bytes.

::: devtools/shared/client/object-client.js:9
(Diff revision 1)
> +
> +"use strict";
> +
> +const {arg, DebuggerClient} = require("./debugger-client");
> +const PropertyIteratorClient = require("./property-iterator-client");
> +const SymbolIteratorClient = require("./symbol-iterator-client");

Please use lazy loading for these two clients.

::: devtools/shared/client/source-client.js:8
(Diff revision 1)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {DebuggerClient} = require("./debugger-client");
> +const BreakpointClient = require("./breakpoint-client");

Please use lazy loading for this client and add an empty line between this and `noop`.

::: devtools/shared/client/tab-client.js:13
(Diff revision 1)
> +const promise = Cu.import("resource://devtools/shared/deprecated-sync-thenables.js", {}).Promise;
> +
> +const DevToolsUtils = require("devtools/shared/DevToolsUtils");
> +const eventSource = require("./event-source");
> +const {arg, DebuggerClient} = require("./debugger-client");
> +const ThreadClient = require("./thread-client");

Please lazy load this client.

::: devtools/shared/client/thread-client.js:19
(Diff revision 1)
> +
> +const ArrayBufferClient = require("./array-buffer-client");
> +const EnvironmentClient = require("./environment-client");
> +const LongStringClient = require("./long-string-client");
> +const ObjectClient = require("./object-client");
> +const SourceClient = require("./source-client");

Please lazy load these clients.

::: devtools/shared/client/worker-client.js:10
(Diff revision 1)
> +"use strict";
> +
> +const {DebuggerClient} = require("./debugger-client");
> +const DevToolsUtils = require("devtools/shared/DevToolsUtils");
> +const eventSource = require("./event-source");
> +const ThreadClient = require("./thread-client");

Please lazy load this client and add an empty space after.
Attachment #8913203 - Flags: review?(poirot.alex)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
I used hg copy to keep the history on files, as a consequence the review is huge and mozreview might crash your browser :)
I added a second patch to get rid of main.js and directly require the module where they are needed.
I guess it might also be a good idea to use lady loading where we can on the file I edited ? I'm willing to do this, just want to know if it's worth it.
Comment on attachment 8913203 [details]
Bug 1403895 - split main.js in mulitple files; .

https://reviewboard.mozilla.org/r/184602/#review190588

Thanks for the hg copy!
Attachment #8913203 - Flags: review?(poirot.alex) → review+
Comment on attachment 8913660 [details]
Bug 1403895 - Remove devtools/shared/client/main.js; .

https://reviewboard.mozilla.org/r/185048/#review190590

\o/ even more lazy loading!

> I guess it might also be a good idea to use lady loading where we can on the file I edited ? I'm willing to do this, just want to know if it's worth it.

I quickly scanned the few require calls and the vast majority in for debugger-client which is going to be loaded anyway. And a couple of object-client/environment-client which I imagine may be used quite early by the tools requiring them. So I would say it isn't as important.
Attachment #8913660 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8913660 [details]
Bug 1403895 - Remove devtools/shared/client/main.js; .

https://reviewboard.mozilla.org/r/185048/#review190590

Great ! Thnks for the review, i'm pushing this since try was all green

Comment 11

2 years ago
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 deb86bb51a37 -d 5c767e969819: rebasing 423617:deb86bb51a37 "Bug 1403895 - split main.js in mulitple files; r=ochameau."
rebasing 423618:fb8fdcf1ac42 "Bug 1403895 - Remove devtools/shared/client/main.js; r=ochameau." (tip)
other [source] changed devtools/client/responsivedesign/responsivedesign-old.js which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging devtools/client/framework/devtools-browser.js
merging devtools/client/responsive.html/manager.js
merging devtools/client/webconsole/webconsole.js
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b59b0d7bf9f0
split main.js in mulitple files; r=ochameau.
https://hg.mozilla.org/integration/autoland/rev/14841c4d8a97
Remove devtools/shared/client/main.js; r=ochameau.
https://hg.mozilla.org/mozilla-central/rev/b59b0d7bf9f0
https://hg.mozilla.org/mozilla-central/rev/14841c4d8a97
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

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