Bug 748490 allows us to define testing-only JS modules. I would like to expose httpd.js as a module so it can be imported with Cu.import(). The attached patch does just this. It defines EXPORTED_SYMBOLS in httpd.js, adds the necessary Makefile magic, and provides a simple test to ensure the module can be imported and that a server can be loaded. While I was in the Makefile, I also converted some tabs to spaces because my eyes started to hurt. A follow-up to this patch will likely be to remove the giant hack that is do_load_httpd_js() from xpcshell tests. But, that will be a long and painful process. I prefer not to think about it now.

Try at http://tbpl.mozilla.org/?tree=Try&rev=7e73c25d5c6f

Comment on attachment 623952 [details] [diff] [review] Expose httpd.js as testing module Try failed. Cancelling review.

Looks like the testing JS modules aren't being included in the archive being used to run the xpcshell tests. Time to file another bug...

Comment on attachment 623952 [details] [diff] [review] Expose httpd.js as testing module Review of attachment 623952 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/test/httpserver/httpd.js @@ +45,5 @@ > * component. See the accompanying README file for user documentation on > * httpd.js. > */ > > +const EXPORTED_SYMBOLS = [ Some of those symbols shouldn't actually be exported. "server" is purely for one-off manual testing, for example. The HTTP error codes shouldn't be exported, either, although perhaps something like that will need exporting at some point. I've also wanted to rename "nsHttpServer" to be non-prefixed at some point, so I think if we're going to export we should have |var HttpServer = nsHttpServer;| and export that. (A wider-scale rename can happen at a more leisurely pace, certainly.) Request and Response are also internal mechanism for implementing the interfaces exposed by server objects, so they shouldn't be exposed either. Really, only the server constructor itself should be exposed.

buildbot will still fail, as we'll need bug 755339 for the archiving foo to work. But, this works locally and the review can be conducted without waiting for these patches. We just can't check in until other things land. I removed some of the exported symbols per Waldo's feedback. I kept the HTTP_* constants and HttpError because some code (like services/sync/tests/unit/head_http_server.js) uses it. And, the goal should be to eventually replace load("httpd.js") in xpcshell tests, etc with Cu.import(). i.e. we'll need it eventually, so best to support it now. I also added an alias, HttpServer, that maps to nsHttpServer - also per feedback. Finally, when loaded as a module, gc() (from xpcshell) isn't available. I switched this to Components.utils.forceGC(), which seems to be equivalent.

This review is holding up some things we'd like to land on Fx15. Any chance I can get a review in the next few hours?

Comment on attachment 626485 [details] [diff] [review] Expose httpd.js as testing module Given time constraints I looked this over and think we should move forward.. jwalden might still want to read it as the author of this so leaving a f?

Comment on attachment 626485 [details] [diff] [review] Expose httpd.js as testing module Review of attachment 626485 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. I tend to go through my queue from oldest patch to newest, and I use the Request Queue page to do it. Thus I don't have a good way to see that a review's been outstanding for a longer period of time if the request is touched partway through, as appears to have happened here. ::: netwerk/test/httpserver/httpd.js @@ +789,5 @@ > this._notifyStopped(); > // Bug 508125: Add a GC here else we'll use gigabytes of memory running > // mochitests. We can't rely on xpcshell doing an automated GC, as that > // would interfere with testing GC stuff... > + Components.utils.gc(); I'm not 100% sure this is exactly the right incantation to do GC, but if this works, odds are it's the right one.

So this patch misses to export the SJS_TYPE constant which we probably should use in external frameworks too. Or is this limited to the httpd.js internals only and everyone whould define their own types?

(In reply to Henrik Skupin (:whimboo) from comment #11) > So this patch misses to export the SJS_TYPE constant which we probably > should use in external frameworks too. Or is this limited to the httpd.js > internals only and everyone whould define their own types? Yes, not all symbols are exported. We initially targeted the minimum required feature set. If you find something that you think should be exported, please file a new bug. Each symbol can be evaluated for export relevance in those bugs.