Skip to content
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

docs: Fix types and missing property in WebRequest APIs #17046

Merged
merged 3 commits into from Feb 28, 2019

Conversation

3 participants
@rhysd
Copy link
Contributor

commented Feb 19, 2019

Description of Change

I found that some types in electron.d.ts are wrong related to WebRequest API. I believe that it is generated from docs so I fixed the docs.

  • Hook methods of WebRequest should accept null as listener parameter for removing the listener. This is documented in doc
  • referrer prop is missing in OnBoeforeRequestDetails object

Checklist

Release Notes

Notes: no-notes

@rhysd rhysd requested a review from as a code owner Feb 19, 2019

@rhysd rhysd force-pushed the rhysd:fix-doc-types branch 2 times, most recently from c85f0b9 to f8a2b5f Feb 19, 2019

@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

I'm not sure but the CI failure seems not related to change of this PR.

An error occurred inside the spec runner: Error: Electron tests failed with code 1.
    at runElectronTests (C:\projects\src\electron\script\spec-runner.js:53:11)
    at main (C:\projects\src\electron\script\spec-runner.js:27:9)
    at process._tickCallback (internal/process/next_tick.js:68:7)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! electron@6.0.0-nightly.20190213 test: `node ./script/spec-runner.js electron/spec "--ci" "--enable-logging"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the electron@6.0.0-nightly.20190213 test script.

https://windows-ci.electronjs.org/project/AppVeyor/electron-9vmit/build/1.0.4316

@rhysd rhysd force-pushed the rhysd:fix-doc-types branch from f8a2b5f to 5be3904 Feb 23, 2019

@rhysd rhysd force-pushed the rhysd:fix-doc-types branch from 5be3904 to 48b206c Feb 23, 2019

@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

CI passed. Could you review this?

@codebytere

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@rhysd we had a minisummit last week so we didn't have very much time to review things but we're all getting back now so this should be addressed soon!

@codebytere
Copy link
Member

left a comment

looks good to me but cc @deepak1556 for a second confirmation on the referrer param?

@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@codebytere @deepak1556 Thank you for the review. I just confirmed the referrer param based on Electron 4.0.4 only in my local and did not look any implementation. So additional review is helpful 👍. Please let me know if my modification is not correct. I'll update it.

@jkleinsc jkleinsc requested a review from deepak1556 Feb 27, 2019

@deepak1556
Copy link
Member

left a comment

referrer param will be provided for all webRequest api responses, can you also add it to them. Thanks! lgtm otherwise.

@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

referrer param will be provided for all webRequest api responses, can you also add it to them. Thanks! lgtm otherwise.

Sure! I'll update this branch soon.

@rhysd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

@deepak1556 @codebytere I added referrer property to each responses of webRequest APIs at 203c383.

@codebytere
Copy link
Member

left a comment

approving again with latest changes!

@codebytere codebytere merged commit 6d55498 into electron:master Feb 28, 2019

9 of 10 checks passed

Artifact Comparison Changes Detected
Details
Backportable? - 5-0-x Clean Backport
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Feb 28, 2019

Release Notes Persisted

no-notes

@trop

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

I have automatically backported this PR to "5-0-x", please check out #17167

@sofianguy sofianguy added this to Fixed in 5.0.0-beta.5 in 5.0.x Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.