Closed
Bug 1397330
Opened 7 years ago
Closed 7 years ago
Lazy load protocol.js fronts
Categories
(DevTools :: Framework, enhancement, P3)
DevTools
Framework
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 3 open bugs)
Details
(Whiteboard: dt-fission)
Attachments
(2 files)
Loading of front is about 33ms/~5% of overall JS computation in the parent process: https://perfht.ml/2xavzjT We should try to lazy load them. You can see in this profile that inspector front loads: * highlighters * styles * layout * string And each front has to pull various specs as well. I imagine loading various front wouldn't be an issue if each front wasn't pulling various other modules.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
I flagged both of you on the review to have :jdescottes ramp up on protocol.js and :jryans validate this as *the* expert in this field ;)
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8905411 [details] Bug 1397330 - Lazy load inspector spec and front modules. https://reviewboard.mozilla.org/r/177220/#review182392 Seems resonable to me, I'll leave the r+/- choice to jryans. ::: devtools/shared/fronts/inspector.js:16 (Diff revision 1) > preEvent, > - types > + types, > + FrontModule > } = require("devtools/shared/protocol.js"); > + > +FrontModule("devtools/shared/fronts/styles", "pagestyle"); As discussed on IRC, it's very weird that the inspector front has to require those other fronts here. Reading through the code, I understand that the inspector spec contains types corresponding to those fronts, so they need to be loaded somewhere on the client. But since protocol.js is the one that needs to use the classes, it would make sense for protocol.js to keep a map from actor types to front module paths in order to be able to dynamically instanciate any front. Can be a follow-up as this is not a new issue. Note that there is another front requiring a front in a similar manner: styles.js with require("devtools/shared/fronts/stylesheets"); ::: devtools/shared/protocol.js:269 (Diff revision 1) > + // FrontModule, when registering a front lazily, > + // FrontClassWithSpec, when registering front synchronously, > + // generateActorSpec when defining specs, > + // specs modules, to register actor type early to use them in other types > + if (registeredTypes.has(name)) { > + return; Is it a code path we expect to hit frequently or should log a warning/error to try and fix such cases? ::: devtools/shared/protocol.js:287 (Diff revision 1) > + if (type.module) { > + // Loading the module will end up calling FrontClassWithSpec and set `frontClass` on `type` object. > + require(type.module); > + if (!type.frontClass) { > + throw new Error("Unable to find front class '" + name + "' in '" + type.module + "' module."); > + } > + } nit: could we have this lazyloading logic moved outside of this method? addActorType is already a bit messy ("if we are on the client" "if we are on the server") so it would be great if this complexity could feel less threatening. Either have a dedicated method that you call from here (if (type.frontModulePath) { loadFrontModule(frontModulePath)}), or maybe define a getter on type in your FrontModule() helper? ::: devtools/shared/protocol.js:287 (Diff revision 1) > // existing front on the connection, and create the front > // if it isn't found. > let actorID = typeof (v) === "string" ? v : v.actor; > let front = ctx.conn.getActor(actorID); > if (!front) { > + if (type.module) { Needs a comment explaining that this property means that the front module for this actor is lazy loaded and was not loaded yet. Also could be nice to remove the "module" property on type once we required it. I know in theory we won't go through this codepath again because the front should be retrieved via getActor next time, but when you're not familiar with this code it feels weird to not cleanup type.module. ::: devtools/shared/protocol.js:1548 (Diff revision 1) > > return cls; > }; > exports.FrontClassWithSpec = FrontClassWithSpec; > > +exports.FrontModule = function (module, typeName) { - I would swap typeName and module. - module is a confusing name for a path. 'frontModulePath' ? (both for the arg and the prop added to the actor "type") - the method name could mention lazyness, require etc... FrontModule is a bit generic for something that is very explicitly used for performance reasons ::: devtools/shared/protocol.js:1550 (Diff revision 1) > }; > exports.FrontClassWithSpec = FrontClassWithSpec; > > +exports.FrontModule = function (module, typeName) { > + if (!registeredTypes.has(typeName)) { > + types.addActorType(typeName, module); The module argument is not used by addActorType.
Attachment #8905411 -
Flags: review?(jdescottes)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #3) > Comment on attachment 8905411 [details] > Bug 1397330 - Lazy load inspector front classes. > > https://reviewboard.mozilla.org/r/177220/#review182392 > > Seems resonable to me, I'll leave the r+/- choice to jryans. > > ::: devtools/shared/fronts/inspector.js:16 > (Diff revision 1) > > preEvent, > > - types > > + types, > > + FrontModule > > } = require("devtools/shared/protocol.js"); > > + > > +FrontModule("devtools/shared/fronts/styles", "pagestyle"); > > As discussed on IRC, it's very weird that the inspector front has to require > those other fronts here. > > Reading through the code, I understand that the inspector spec contains > types corresponding to those fronts, so they need to be loaded somewhere on > the client. > > But since protocol.js is the one that needs to use the classes, it would > make sense for protocol.js to keep a map from actor types to front module > paths in order to be able to dynamically instanciate any front. I think that's a good idea as it matches what we do in the server for actors: http://searchfox.org/mozilla-central/source/devtools/server/main.js#423-588 But I wouldn't do it in protocol.js but somewhere else. I'm just starting to wonder if this change is going to help a lot bug 1222047! > > Can be a follow-up as this is not a new issue. I'm fine taking some time to think and come up with a meaningful and future prone patch. > > ::: devtools/shared/protocol.js:269 > (Diff revision 1) > > + // FrontModule, when registering a front lazily, > > + // FrontClassWithSpec, when registering front synchronously, > > + // generateActorSpec when defining specs, > > + // specs modules, to register actor type early to use them in other types > > + if (registeredTypes.has(name)) { > > + return; > > Is it a code path we expect to hit frequently or should log a warning/error > to try and fix such cases? This is frequent. So I explicitely avoided logging a warning here. The collision happens between a FrontModule call and a generateActorSpec called from a spec, itself loaded an actor, loaded by gcli in the parent process. > ::: devtools/shared/protocol.js:287 > (Diff revision 1) > > + if (type.module) { > > + // Loading the module will end up calling FrontClassWithSpec and set `frontClass` on `type` object. > > + require(type.module); > > + if (!type.frontClass) { > > + throw new Error("Unable to find front class '" + name + "' in '" + type.module + "' module."); > > + } > > + } > > nit: could we have this lazyloading logic moved outside of this method? > addActorType is already a bit messy ("if we are on the client" "if we are on > the server") so it would be great if this complexity could feel less > threatening. I had the exact same thought while looking at addActorType. I'm wondering if we should fork it and have addActorType and addFrontType?? It looks like the main thing in common is formType, which is simple enough to be forked. But we can also easily factorize it... > Either have a dedicated method that you call from here (if > (type.frontModulePath) { loadFrontModule(frontModulePath)}), or maybe define > a getter on type in your FrontModule() helper? Then, if we fork it, and if all fronts are lazy loaded, may be that's ok to keep it here? > ::: devtools/shared/protocol.js:1548 > (Diff revision 1) > > > > return cls; > > }; > > exports.FrontClassWithSpec = FrontClassWithSpec; > > > > +exports.FrontModule = function (module, typeName) { > > - the method name could mention lazyness, require etc... FrontModule is a > bit generic for something that is very explicitly used for performance > reasons Ideally, we would end up with all fronts being loaded like this, with FrontModule. Only then, I think the current name makes sense. We would not have lazy and non-lazy. Only lazy and that could just be a comment on this method. Speaking about the n-th kind of creating an actor or front, I referenced an helpful cleanup in bug 1397452 comment 1, where we could get rid of ActorClass and FrontClass. There function are only kept for old bootstrapped addons.
Assignee | ||
Comment 5•7 years ago
|
||
Oh, and thanks a ton for the review, I'm really happy to see a new pair of eyes on this ;)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8905411 [details] Bug 1397330 - Lazy load inspector spec and front modules. https://reviewboard.mozilla.org/r/177220/#review182856 Overall, protocol.js has some subtle / confusing things about it, like the registered actor types which are effectively global state, so we end up with these unusual / suspicious `require` calls that are only here because they need to register a type. The approach of this patch is pretty targetted to the specific case we are seeing here, and it still leaves the "strange" feeling calls (`FrontModule`) because of the global state issue. Is there a good way of helping protocol.js manage this more directly, sort of like how :jdescottes mentions? Could we annotate something in the spec files with the path? Maybe when the inspector spec uses `pagestyle`, it can annotate that with `{ path: "styles" }` since that where the fronts / specs / actors are found. This would mean it becomes a more convention-based system. Maybe it's too magical? I am still open to something like the current patch, it's a fairly small change, but I'd like to hear thoughts on other approaches too. Please at least address all of :jdescottes's notes if you want to proceed this way.
Attachment #8905411 -
Flags: review?(jryans)
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6) > Is there a good way of helping protocol.js manage this more directly, sort > of like how :jdescottes mentions? Could we annotate something in the spec > files with the path? Maybe when the inspector spec uses `pagestyle`, it can > annotate that with `{ path: "styles" }` since that where the fronts / specs > / actors are found. This would mean it becomes a more convention-based > system. Maybe it's too magical? I went for a "front" attribute within specs, but not from inspector specs, from each spec related to a given front. I have mixed feeling about having just: front: "styles", I tend to prefer using absolute require path where mean specifying a module. So even if this patch use a module name, I would actually prefer going for: front: "devtools/shared/fronts/styles", Then, I imagine we could potentially do the same thing but for actors: actor: "devtools/server/actors/styles", Also, at some point, it would be great to also be able to load the specs on demand. I think it is where a central place with all type names would be necessary. Something similar to what we do for actors here: http://searchfox.org/mozilla-central/source/devtools/server/main.js#423-588 but, for specs/typeNames. I believe we could do laziness at every level thanks to such registry and the "front"/"actor" attributes. This registry would be something like: const AllSpecs = { "devtools/shared/specs/styles": [ "pagestyle", "domstylerule", ] "devtools/shared/specs/highlighters": [ "highlighters", "customHighlighters", ], "devtools/shared/specs/layout": [ "grid", "layout", ], }; In addition to lazy load of specs, we should also be able to remove the various calls to require(devtools/shared/specs) from specs, which are here for the same reason as the require(devtools/shared/fronts) we are trying to remove. Thoughts?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8906969 [details] Bug 1397330 - Define all specs and fronts relying on implicit require. Here is comment 7 regarding lazy specs. Obviously "AllSpecs" registry should be somewhere else... This patch prevents loading `specs/{highlighters,styles,layout}` until it is really needed. And we can then require the various require(specs/...) from other specs. If we do down that road, I imagine we can: * remove all require(specs/...) everywhere in fronts/specs * revamp FrontClassWithSpec to pass the typename instead of spec object
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8905411 [details] Bug 1397330 - Lazy load inspector spec and front modules. https://reviewboard.mozilla.org/r/177220/#review184020 This new approach is confusing for me. I would prefer to use the exact same pattern as what you did in the next commit for specs.
Attachment #8905411 -
Flags: review?(jdescottes)
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8906969 [details] Bug 1397330 - Define all specs and fronts relying on implicit require. https://reviewboard.mozilla.org/r/178722/#review183994 As discussed on IRC I think we need to implement a generic solution where protocol.js is able to require all the necessary modules dynamically/lazily. But for now let's keep this simple and only special case the specs and fronts which can obviously be lazy loaded. So this patch is almost good to go IMO, it mostly needs a detailed comment to explain the role of the AllSpecs object. ::: devtools/shared/protocol.js:69 (Diff revision 1) > + // If that's the case, load the module first, which is going to register the > + // type into `registeredTypes` synchronously and be available in the next > + // block. > + let modulePath = lazyTypes.get(type); > + if (modulePath) { > + lazyTypes.delete(type); delete the entry only after a successful require? ::: devtools/shared/protocol.js:569 (Diff revision 1) > * @param type type > * The return value should be marshalled as this type. > */ > var RetVal = function (type) { > - this.type = types.getType(type); > + // Prevent force loading all RetVal types by accessing it only when needed > + loader.lazyGetter(this, "type", function () { No Arg() is currently relying on one of the dynamically loaded specs, but in the long term we should enforce lazyness there too. ::: devtools/shared/protocol.js:1627 (Diff revision 1) > } > > return ret; > }; > + > +const AllSpecs = { Add a comment to explain that all the specs listed here are never directly required, and might have to be dynamically loaded by protocol.js in case an Arg/RetVal corresponds to the listed actor types. Explain that this object maps modules paths to actor types. Also given the current situation, AllSpecs is probably not the right name. LazySpecs?
Attachment #8906969 -
Flags: review?(jdescottes) → review+
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8905411 [details] Bug 1397330 - Lazy load inspector spec and front modules. https://reviewboard.mozilla.org/r/177220/#review184028 Thanks for trying out this idea. Now I agree relative / magic paths are probably too much, it already looks suspicious when I read the spec files in the patch. :)
Attachment #8905411 -
Flags: review?(jryans)
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8906969 [details] Bug 1397330 - Define all specs and fronts relying on implicit require. https://reviewboard.mozilla.org/r/178722/#review184026 I think I like this design better so far. I'd like see it again with comments and final changes in place to double-check. Also, I am curious what the more generic solution you discussed on IRC was like... I didn't see any discussion in my IRC history. ::: devtools/shared/protocol.js:1627 (Diff revision 1) > } > > return ret; > }; > + > +const AllSpecs = { I don't think protocol.js is right place for the list of specs. I'd like keep a potentially large list like that separate from the code. Maybe `shared/specs/index.js` or similar? `types.js`?
Attachment #8906969 -
Flags: review?(jryans) → review-
Comment 15•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14) > Comment on attachment 8906969 [details] > Bug 1397330 - Lazy load inspector spec classes. > > https://reviewboard.mozilla.org/r/178722/#review184026 > > I think I like this design better so far. I'd like see it again with > comments and final changes in place to double-check. > > Also, I am curious what the more generic solution you discussed on IRC was > like... I didn't see any discussion in my IRC history. Clarified on IRC, but for the record: by generic solution, I simply meant applying the same loading strategy for all the specs and fronts. > > ::: devtools/shared/protocol.js:1627 > (Diff revision 1) > > } > > > > return ret; > > }; > > + > > +const AllSpecs = { > > I don't think protocol.js is right place for the list of specs. I'd like > keep a potentially large list like that separate from the code. Maybe > `shared/specs/index.js` or similar? `types.js`? I agree this will make it clearer and easier to maintain, particularly in the long run when we add other specs to it.
Assignee | ||
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=315527a6467268b06f1123ff7ccea2c13c09b89c
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8905411 [details] Bug 1397330 - Lazy load inspector spec and front modules. https://reviewboard.mozilla.org/r/177220/#review184270 Looks good! Just one additional comment for a potential follow-up. The lazySpecs/lazyFronts maps could be handled directly by the new index file. It could expose getFront(type), getSpecs(type) and would take care of the lazy loading, managing the lazy maps etc... protocol.js could be a bit a simpler after that. ::: devtools/shared/protocol.js:111 (Diff revision 3) > if (type === "longstring") { > require("devtools/shared/specs/string"); > return registeredTypes.get("longstring"); > } An old-school lazy loaded spec! I think it's worth moving to your new index file straight away. ::: devtools/shared/specs/index.js:1 (Diff revision 3) > /* This Source Code Form is subject to the terms of the Mozilla Public This change was registered as a copy of devtools/shared/specs/inspector.js Would be nice if we can avoid that, it doesn't really make sense. ::: devtools/shared/specs/index.js:7 (Diff revision 3) > -types.addDictType("searchresult", { > - list: "domnodelist", > - // Right now there is isn't anything required for metadata, > - // but it's json so it can be extended with extra data. > +// Registry indexing all specs and front modules > +// > +// All spec and front modules should be listed here > +// in order to be referenced by any other spec or front module. This comment should be consistent with the current implementation. Either rephrase to use "dynamically loaded specs and front modules", or add a visible "TODO": For now we only register dynamically loaded specs and fronts here. See Bug XXX for supporting all specs and front modules."
Attachment #8905411 -
Flags: review?(jdescottes) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8906969 [details] Bug 1397330 - Define all specs and fronts relying on implicit require. https://reviewboard.mozilla.org/r/178722/#review184290 mozreview forwarded the r+ but I'm fine with this. Note that we still have two magical requires: require("devtools/shared/specs/inspector"); in shared/specs/animation.js (needs "domnode" and "domwalker" both already registered) require("devtools/shared/specs/performance-recording"); in shared/specs/performance.js (needs ... "performance-recording") I doubt the animation inspector shows up on any profile since the inspector front/specs are most likely already loaded when we require the animationinspector modules. But that would still be nice to cleanup for consistency.
Comment 21•7 years ago
|
||
I just realized that the dynamic requires introduced here will not be webpack friendly. We will probably need modules using Launchpad to require all specs and front modules upfront (in their dedicated index.js for instance).
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8905411 [details] Bug 1397330 - Lazy load inspector spec and front modules. https://reviewboard.mozilla.org/r/177220/#review184454 Thanks, looks reasonable overall! :) I agree with :jdescottes comments, so I did not repeat them here. ::: devtools/shared/protocol.js:32 (Diff revision 3) > */ > > var types = Object.create(null); > exports.types = types; > > -var registeredTypes = types.registeredTypes = new Map(); > +var registeredTypes = new Map(); Valence uses the `registeredTypes` export: https://github.com/mozilla/valence/blob/49c8143270de74508904ba3843651f5b2efbbe17/lib/util/protocol-extra.js#L33 but I hear it's been decided to unship Valence. ::: devtools/shared/protocol.js:281 (Diff revision 3) > * @param string name > * The typestring to register. > */ > types.addActorType = function (name) { > + // We call addActorType from: > + // FrontClassWithSpec, when registering front synchronously, I think the comma after the name like "FrontClassWithSpec," is a bit strange to read... I'd say remove the commas. Or, if you like them, add one after generateActorSpec so it matches the others. ::: devtools/shared/protocol.js:303 (Diff revision 3) > // existing front on the connection, and create the front > // if it isn't found. > let actorID = typeof (v) === "string" ? v : v.actor; > let front = ctx.conn.getActor(actorID); > if (!front) { > + // If front isn't instanciate yet, create one. Nit: instanciate -> instanciated ::: devtools/shared/protocol.js:1632 (Diff revision 3) > } > > return ret; > }; > + > +require("devtools/shared/specs/index"); Add a comment about what this is and why it's here. Would it make more sense for this to happen at some other initialization stage? (I'm not sure myself...)
Attachment #8905411 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 23•7 years ago
|
||
DAMP reports no perf change: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=2742d08676f5d577c7f8fcf6d3cabc52f7b5714d&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800 But the profiler reports a win on devtools/shared/fronts/inspector.js load. From (without the patch): https://perfht.ml/2w3ifh7 7.4ms, 2.8% of overall js computation on parent process during toolbox box To (with the patch): https://perfht.ml/2w4zbUp 4.1ms, 1.2%
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #19) > Comment on attachment 8905411 [details] > Bug 1397330 - Lazy load inspector spec and front modules. > > https://reviewboard.mozilla.org/r/177220/#review184270 > > Looks good! Just one additional comment for a potential follow-up. The > lazySpecs/lazyFronts maps could be handled directly by the new index file. > It could expose getFront(type), getSpecs(type) and would take care of the > lazy loading, managing the lazy maps etc... protocol.js could be a bit a > simpler after that. So. I ended up doing something similar to that. I couldn't do getFront/getSpecs as the registeredTypes Map has to stay in protocol.js. Mostly because of primitive and all non-pure-actor types (like arrays, nullables, ...). It may be different for fronts, we may be able to keep a Map(type => frontClass) in index.js. But we would have to redirect FrontWithActorSpec to index.js But I prefered to keep similar integration between specs and fronts, and ended up implementing: - lazyLoadSpecs(type) - lazyLoadFront(type) It feels still imperfect. getFront and getSpecs would be better. But I think we reached a point where we should really followup on this. > > ::: devtools/shared/protocol.js:111 > (Diff revision 3) > > if (type === "longstring") { > > require("devtools/shared/specs/string"); > > return registeredTypes.get("longstring"); > > } > > An old-school lazy loaded spec! I think it's worth moving to your new index > file straight away. Oh, interesting to see it was doing exactly what I do :o Converted! > ::: devtools/shared/specs/index.js:1 > (Diff revision 3) > > /* This Source Code Form is subject to the terms of the Mozilla Public > > This change was registered as a copy of devtools/shared/specs/inspector.js > > Would be nice if we can avoid that, it doesn't really make sense. Should be fixed, yet another cinnabar magic trick... > ::: devtools/shared/specs/index.js:7 > (Diff revision 3) > > -types.addDictType("searchresult", { > > - list: "domnodelist", > > - // Right now there is isn't anything required for metadata, > > - // but it's json so it can be extended with extra data. > > +// Registry indexing all specs and front modules > > +// > > +// All spec and front modules should be listed here > > +// in order to be referenced by any other spec or front module. > > This comment should be consistent with the current implementation. Either > rephrase to use "dynamically loaded specs and front modules", or add a > visible "TODO": For now we only register dynamically loaded specs and fronts > here. See Bug XXX for supporting all specs and front modules." Went for the additional "TODO" note. Filled bug 1399589 for that.
Assignee | ||
Comment 27•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO until Sept. 19) from comment #22) > ::: devtools/shared/protocol.js:32 > (Diff revision 3) > > */ > > > > var types = Object.create(null); > > exports.types = types; > > > > -var registeredTypes = types.registeredTypes = new Map(); > > +var registeredTypes = new Map(); > > Valence uses the `registeredTypes` export: > > https://github.com/mozilla/valence/blob/ > 49c8143270de74508904ba3843651f5b2efbbe17/lib/util/protocol-extra.js#L33 > > but I hear it's been decided to unship Valence. I reverted that change, I thought it was really unused. I'll clean that up in a cleanup patch, later.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8905411 [details] Bug 1397330 - Lazy load inspector spec and front modules. https://reviewboard.mozilla.org/r/177220/#review184692 ::: devtools/shared/specs/index.js:31 (Diff revision 5) > + }, > + { > + types: ["grid", "layout"], > + spec: "devtools/shared/specs/layout", > + front: "devtools/shared/fronts/layout", > + }, Shouldn't we have the longstring here too?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #30) > Comment on attachment 8905411 [details] > Bug 1397330 - Lazy load inspector spec and front modules. > > https://reviewboard.mozilla.org/r/177220/#review184692 > > ::: devtools/shared/specs/index.js:31 > (Diff revision 5) > > + }, > > + { > > + types: ["grid", "layout"], > > + spec: "devtools/shared/specs/layout", > > + front: "devtools/shared/fronts/layout", > > + }, > > Shouldn't we have the longstring here too? Yes, I mess up a rebase...
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 36•7 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4621001d8ba5 Lazy load inspector spec and front modules. r=jdescottes,jryans https://hg.mozilla.org/integration/autoland/rev/a794ed07b821 Define all specs and fronts relying on implicit require. r=jdescottes
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4621001d8ba5 https://hg.mozilla.org/mozilla-central/rev/a794ed07b821
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
Assignee | ||
Updated•6 years ago
|
Whiteboard: dt-fission
You need to log in
before you can comment on or make changes to this bug.
Description
•