Closed Bug 1403895 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

Attachments

(2 files)

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: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1403106
Blocks: 1383711
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)
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+
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
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)
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.