Closed
Bug 1403895
Opened 7 years ago
Closed 7 years ago
Split devtools/shared/client/main.js into multiple components
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
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 | ||
Updated•7 years ago
|
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 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)
Comment 3•7 years ago
|
||
> 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 4•7 years ago
|
||
mozreview-review |
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•7 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 8•7 years ago
|
||
mozreview-review |
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 9•7 years ago
|
||
mozreview-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•7 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•7 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•7 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.
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b59b0d7bf9f0 https://hg.mozilla.org/mozilla-central/rev/14841c4d8a97
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•