jsproxy.cpp is 3000 lines long. It contains definitions of 2 proxy classes, and function implementations of friend API boilerplate, 4 different Proxy handler classes, as well as the engine's interface into the proxy world. Similarly, jswrapper.cpp contains definitions of a bunch of jsfriendapi helpers related to wrappers, as well as function definitions of some 3 or 4 proxy handler classes. It is now the case that we have reason to want to move one of these myriad definitions from one file to another. This is insanity. Jason has had the frankly staggering idea of organizing everything with its own little subfolder and giving each handler type its own header and implementation file. You know, me just might be onto something.

What do people think about this? Too much. Worth it? This is obviously a WIP. There's many more handlers to do, and all of jswrapper, though I will likely just break that out into *.cpp and leave the header (which kinda all goes together now) as one unit. The other alternative there is to have it contain the public facing stuff and include the other headers after splitting them out. Thoughts?

Comment on attachment 8482955 [details] [diff] [review] Just so we get a taste Review of attachment 8482955 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me.

Comment on attachment 8482955 [details] [diff] [review] Just so we get a taste Review of attachment 8482955 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/moz.build @@ +59,3 @@ > 'perf/jsperf.h', > + 'proxy/jsproxy.h', > + 'proxy/jswrapper.h', Drive-by comment: these should be something like proxy/Proxy.h and proxy/Wrapper.h. @@ +231,5 @@ > 'perf/jsperf.cpp', > 'prmjtime.cpp', > + 'proxy/DirectProxyHandler.cpp', > + 'proxy/jsproxy.cpp', > + 'proxy/jswrapper.cpp', Ditto.

Sorry for the noise. Seems like it should be easier to look over things one idea at a time.

Comment on attachment 8486206 [details] [diff] [review] Part 10: Factor out SecurityWrapper Review of attachment 8486206 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this!

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d095ec7083b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2096329d05c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0016652c3e8c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/72dad42e3912 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/abd86c308ffe remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae18d60bbd2a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe450659642 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/91a3c20e5e7f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d63a483978a5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/af39f6ba880b

https://hg.mozilla.org/mozilla-central/rev/4d095ec7083b https://hg.mozilla.org/mozilla-central/rev/b2096329d05c https://hg.mozilla.org/mozilla-central/rev/72dad42e3912 https://hg.mozilla.org/mozilla-central/rev/abd86c308ffe https://hg.mozilla.org/mozilla-central/rev/ae18d60bbd2a https://hg.mozilla.org/mozilla-central/rev/dbe450659642 https://hg.mozilla.org/mozilla-central/rev/91a3c20e5e7f https://hg.mozilla.org/mozilla-central/rev/d63a483978a5 https://hg.mozilla.org/mozilla-central/rev/af39f6ba880b https://hg.mozilla.org/mozilla-central/rev/e86946fb9889