search

mozilla / positron

a experimental, Electron-compatible runtime on top of Gecko
Other
564 stars 64 forks source link

run renderer process init.js script to initialize environment (fixes #25) #26

Closed mykmelez closed 8 years ago

mykmelez commented 8 years ago

This branch evaluates the renderer process init.js script when a module loader is constructed for a new BrowserWindow. Running that script entrains a bunch of dependencies, including many Node core modules and some Node core native bindings. But relatively few of the Node APIs are actually exercised, from what I can tell. So it should be possible to get the script working without having to rewrite much Node.

So far this branch imports a bunch of Node modules verbatim and stubs a few native bindings.

marcoscaceres commented 8 years ago

Hmm... maybe ignore my comments as we are importing the Node modules verbatim.

mykmelez commented 8 years ago

Hmm... maybe ignore my comments as we are importing the Node modules verbatim.

Yes, we should avoid changing imported Node and Electron code to the extent possible. There'll be a couple cases where we do (I'm looking at you, web-view.js), but in general it's better to leave them be in order to make it easier to track upstream changes. (If you find a bug, however, then it makes sense to fix it in our repo and then submit a PR to fix it upstream as well.)

mykmelez commented 8 years ago

Ok, I've implemented/stubbed enough to get the renderer process's init.js script to successfully evaluate up to the point that code in web-view.js throws an exception, which means we can start working on . So this is now ready for review.

@marcoscaceres are you still interested/available to review this?

Note that the code in positron/node/ is imported from the Node project, and it's unchanged, so it isn't necessary to review that code.

marcoscaceres commented 8 years ago

Assigned to self for review. Happy to try to help.

marcoscaceres commented 8 years ago

OK, just a couple of little comments, but r+ from me otherwise.

mykmelez commented 8 years ago

@marcoscaceres Merge if you're satisfied with this, or let me know what else to do with it!

marcoscaceres commented 8 years ago

Sent one last comment. If you are confident we can proceed wrt IDWeakMap, then let's merge.

On 21 May 2016, at 2:48 AM, Myk Melez notifications@github.com wrote:

@marcoscaceres Merge if you're satisfied with this, or let me know what else to do with it!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub