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.

Sign up for GitHub

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

Jump to bottom

feat: allow headers to be sent with webContents.downloadURL() #39455

Merged
merged 1 commit into from Aug 17, 2023
Merged

feat: allow headers to be sent with webContents.downloadURL() #39455

merged 1 commit into from Aug 17, 2023

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Aug 10, 2023 •
edited

Description of Change

Refs #38785.

There's no reason that the API signatures for these methods shouldn't be identical, and it's arguably more confusing for users that they're not, given they both trigger will-download on session.

NOTE: The bulk of this change is test changes and additions - the code change is minimal and identical to what was changed in #38785.

Caught when cleaning up some tests.

Checklist

Release Notes

Notes: Added the ability to send HTTP headers with webContents.downloadURL().

@codebytere codebytere added the semver/minor backwards-compatible functionality label Aug 10, 2023
@electron-cation electron-cation bot added api-review/requested 🗳 new-pr 🌱 PR opened in the last 24 hours labels Aug 10, 2023
@codebytere codebytere changed the title feat: allow headers to be sent with webContents.downloadURL() feat: allow headers to be sent with webContents.downloadURL() Aug 10, 2023
itsananderson
itsananderson approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@itsananderson itsananderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

ckerr
ckerr reviewed Aug 14, 2023
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM.

One small doc comment, we've now got three identical options structs in the docs. Candidate for a new addition to docs/api/structures/ ?

zcbenz
zcbenz approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

@codebytere codebytere added target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Aug 15, 2023
@github-actions github-actions bot added the target/27-x-y PR should also be added to the "27-x-y" branch. label Aug 16, 2023
@codebytere codebytere merged commit 00746e6 into main Aug 17, 2023
19 checks passed
@codebytere codebytere deleted the allow-headers-wc-downloadurl branch August 17, 2023 12:17
@release-clerk
Copy link

release-clerk bot commented Aug 17, 2023

Release Notes Persisted

Added the ability to send HTTP headers with webContents.downloadURL().

@trop trop bot mentioned this pull request Aug 17, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Aug 17, 2023

I have automatically backported this PR to "27-x-y", please check out #39560

@trop trop bot mentioned this pull request Aug 17, 2023
Merged
@trop trop bot removed the target/27-x-y PR should also be added to the "27-x-y" branch. label Aug 17, 2023
@trop trop bot mentioned this pull request Aug 17, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Aug 17, 2023

I have automatically backported this PR to "25-x-y", please check out #39561

@trop
Copy link
Contributor

trop bot commented Aug 17, 2023

I have automatically backported this PR to "26-x-y", please check out #39562

@trop trop bot added in-flight/26-x-y and removed target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Aug 17, 2023
@trop trop bot added merged/27-x-y PR was merged to the "27-x-y" branch. merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. and removed in-flight/27-x-y in-flight/25-x-y in-flight/26-x-y labels Aug 24, 2023
win32ss pushed a commit to win32ss/supermium-electron that referenced this pull request Sep 24, 2023
@codebytere
feat: allow headers to be sent with webContents.downloadURL() (elec…
725366b 
…tron#39455)

feat: allow headers to be sent with webContents.downloadURL()
@commit-lint commit-lint bot mentioned this pull request Nov 6, 2023
Open
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@codebytere
feat: allow headers to be sent with webContents.downloadURL() (elec…
84b082b 
…tron#39455)

feat: allow headers to be sent with webContents.downloadURL()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr left review comments

@itsananderson itsananderson itsananderson approved these changes

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 Awaiting requested review from deepak1556

@miniak miniak Awaiting requested review from miniak

Assignees
No one assigned
Labels
api-review/approved ✅ merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@codebytere @ckerr @itsananderson @zcbenz