search

mozilla / positron

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

First pass at DevTools for BrowserWindow #16

Closed jryans closed 8 years ago

jryans commented 8 years ago

I've added support to open the DevTools when calling BrowserWindow.webContents.openDevTools() as in the example app (so this fixes #3).

I added some additional abilities to the tools for targeting specific windows directly to help with this case. I've kept all the /devtools changes in separate commits from /positron ones, for hopefully easier merging if bits are landed in m-c later.

The tools seems to be up and running in general, but there's a few rough edges for now, like you get a giant error on quit if the tools are still open. I figured I would resolve these things separately. (Error on quit now resolved.)

@mykmelez, maybe you can review?

bgrins commented 8 years ago

Is there a corresponding bug to merge this work back into m-c?

bgrins commented 8 years ago

Is there a corresponding bug to merge this work back into m-c?

Or maybe it doesn't make sense to do that at this point. I was thinking we'd bump into conflicts trying to keep it in sync but these files don't seem too likely to drift.

jryans commented 8 years ago

No bug to merge to m-c yet. So far the devtools changes are pretty isloated, so I think it should be okay.

mykmelez commented 8 years ago

I can review, but it'd be helpful to have someone more familiar with the /devtools code look at that part of it. Perhaps @ochameau?

ochameau commented 8 years ago

I think we could reasonably land the devtools patch with a new test for getWindow in devtools/client/framework/test/browser_target_from_url.js Regarding unloading issues. I would really like to make it so that regular toolboxes used about: url. And remove the direct calls of Toolbox.xul from chrome, so that we kills some chrome interactions for the benefit of devtools.html. I imagine I would hit same issues than you and I'm easer to know more about unload issues.

jryans commented 8 years ago

Review comments addressed. r? @mykmelez, @ochameau.

mykmelez commented 8 years ago

Looks great! Like @marcoscaceres, I'm a fan of const, but I agree that it makes sense to use it similarly to existing usage in existing files in devtools/. It looks like you've addressed all the other issues, so I'll merge this, and we can address anything else that pops up in followups.

jryans commented 8 years ago

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1268134 to merge the DevTools changes here back to m-c, as they should be useful outside of Positron as well.