New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement partial chrome.* API for devtools extension #5711

Merged
merged 38 commits into from May 29, 2016

Conversation

Projects
None yet
6 participants
@zcbenz
Contributor

zcbenz commented May 26, 2016

The purpose of this PR is to partially implement chrome.* API to make the React devtools extension work.

Close #915.
Close #2598.

let manifestMap = {}
const getManifestFromPath = function (srcDirectory) {
let manifest = JSON.parse(fs.readFileSync(path.join(srcDirectory, 'manifest.json')))

This comment has been minimized.

@kevinsawicki

kevinsawicki May 26, 2016

Contributor

I think manifest could be const.

This comment has been minimized.

@zcbenz

zcbenz May 26, 2016

Contributor

Ah I used to think const means immutable.

if (err)
callback(-6) // FILE_NOT_FOUND
else
return callback({mimeType: 'text/html', data: content})

This comment has been minimized.

@kevinsawicki

kevinsawicki May 26, 2016

Contributor

I don't think you need the return here.

This comment has been minimized.

@paulcbetts

paulcbetts May 26, 2016

Contributor

It's super easy in a protocol handler to call callback > 1x, and weird stuff will happen, having the return is definitely a good idea

This comment has been minimized.

@kevinsawicki

kevinsawicki May 26, 2016

Contributor

@paulcbetts this is the last line in the method though and two lines above doesn't return

This comment has been minimized.

@paulcbetts

paulcbetts May 26, 2016

Contributor

Embrace your inner paranoia :) You're probably right, but in general, having an attitude of "Be super paranoid about callback in protocol handler" serves you well because it's a big pain to debug when it goes wrong

This comment has been minimized.

@kevinsawicki

kevinsawicki May 26, 2016

Contributor

I left this comment originally since it seemed like @zcbenz was removing automatically inserted returns from the CoffeeScript days, see lines 157 and 171 for other examples and it seemed like this was another case looking at line 105 in the old diff.

I'm totally into being defensive but consistency, consistency, consistency...

}
// Manage the background pages.
let backgroundPages = {}

This comment has been minimized.

@kevinsawicki

kevinsawicki May 26, 2016

Contributor

const ?

@paulcbetts

This comment has been minimized.

Contributor

paulcbetts commented May 26, 2016

Yass yass yass yass yass

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 29, 2016

@zcbenz zcbenz merged commit 9f0fc96 into master May 29, 2016

9 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3327121 succeeded in 43s
Details
electron-linux-ia32 Build #3327122 succeeded in 37s
Details
electron-linux-x64 Build #3327123 succeeded in 118s
Details
electron-mas-x64 Build #1342 succeeded in 5 min 40 sec
Details
electron-osx-x64 Build #1344 succeeded in 5 min 40 sec
Details
electron-win-ia32 Build #347 succeeded in 6 min 19 sec
Details
electron-win-x64 Build #341 succeeded in 6 min 17 sec
Details

@zcbenz zcbenz deleted the extension-code-cleanup branch May 29, 2016

@johnhaley81

This comment has been minimized.

Contributor

johnhaley81 commented May 30, 2016

@tomasro27

This comment has been minimized.

tomasro27 commented Oct 18, 2016

@zcbenz
I'm guessing these extensions don't work for previous versions of electron, even though the API call is there, right? (0.37.6 in particular). I particularly want to get "augury" extension to work with electron, but I have also tried other extensions, and they don't show up. No errors are displayed either. Using "BrowserWindow.addDevToolsExtension(PATH);"

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 18, 2016

0.37.6 in particular

These API's were only added in 1.2.x so anything before that won't work correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment