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

fix(extensions): implement missing web_request hooks #22655

Merged
merged 56 commits into from Dec 18, 2020
Merged

fix(extensions): implement missing web_request hooks #22655

merged 56 commits into from Dec 18, 2020

Conversation

sentialx
Copy link
Contributor

@sentialx sentialx commented Mar 11, 2020 •
edited by nornagon

Description of Change

Currently the chrome.webRequest listeners don't do anything.

This PR adds missing web_request_api hooks required for intercepting requests using chrome.webRequest.

Ref #19447

Checklist

Release Notes

Notes: Fixed chrome.webRequest extensions API not intercepting any requests.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Mar 11, 2020
@sentialx sentialx changed the title fix(extensions): implement missing web_request hooks [wip] fix(extensions): implement missing web_request hooks Mar 12, 2020
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Mar 12, 2020
@zcbenz zcbenz added the wip ⚒ label Mar 20, 2020
@sentialx
Copy link
Contributor Author

sentialx commented Apr 6, 2020

@nornagon @zcbenz Could you give me some advice on how to fix the web socket unit test?

@sentialx
Copy link
Contributor Author

sentialx commented Apr 7, 2020

My PR is ready for review.

@sentialx sentialx changed the title [wip] fix(extensions): implement missing web_request hooks fix(extensions): implement missing web_request hooks Apr 7, 2020
@zcbenz zcbenz removed the wip ⚒ label Apr 7, 2020
@sentialx sentialx requested review from a team as code owners April 7, 2020 15:53
jkleinsc
jkleinsc requested changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

@sentialx can you rebase your branch on master? It looks like you pulled in a whole bunch of commits that are already in master.

@sentialx
Copy link
Contributor Author

sentialx commented Apr 7, 2020 •
edited

@jkleinsc I think I've reverted the rebase, I don't see those commits anymore

nornagon
nornagon requested changes Apr 7, 2020
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

How does this interact with Electron's webRequest API? If a request is intercepted by both an extension and Electron's webRequest API, does one take precedence? Does the request get passed first through one of those, then the other? Which comes first?

Also, I think this needs a test or two.

BTW, you can download built artifacts from CI, e.g. here's the one for mac from this PR. It's under the 'artifacts' tab on circleci under the 'osx-testing' job under the 'build-mac' check. https://circleci.com/gh/electron/electron/513409#artifacts/containers/0

shell/browser/electron_browser_client.cc Outdated Show resolved Hide resolved
nornagon
nornagon requested changes Apr 10, 2020
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

I'd like to see a test for this please! Ideally:

  1. Test the basic chrome.webRequest functionality to make sure it works at all.
  2. Test combining electron's webRequest with chrome.webRequest and assert that electron's webRequest isn't called.

spec-main/extensions-spec.ts is the place to look for existing tests.

shell/browser/electron_browser_client.cc Outdated Show resolved Hide resolved
sentialx added a commit to sentialx/electron that referenced this pull request Dec 21, 2020
@sentialx @nornagon @samuelmaddock
fix(extensions): implement missing web_request hooks (electron#22655)
9604031 
Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>
@sentialx sentialx mentioned this pull request Dec 21, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Dec 21, 2020

@sentialx has manually backported this PR to "11-x-y", please check out #27096

sentialx added a commit to sentialx/electron that referenced this pull request Dec 21, 2020
@sentialx @nornagon @samuelmaddock
fix(extensions): implement missing web_request hooks (electron#22655)
19a9f4c 
Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>
@sentialx sentialx mentioned this pull request Dec 21, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Dec 21, 2020

@sentialx has manually backported this PR to "10-x-y", please check out #27097

sentialx added a commit to sentialx/electron that referenced this pull request Dec 21, 2020
@sentialx @nornagon @samuelmaddock
fix(extensions): implement missing web_request hooks (electron#22655)
0b0e09f 
Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>
@sentialx sentialx mentioned this pull request Dec 21, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Dec 21, 2020

@sentialx has manually backported this PR to "12-x-y", please check out #27098

zcbenz pushed a commit that referenced this pull request Jan 5, 2021
@sentialx @nornagon @samuelmaddock
fix(extensions): implement missing web_request hooks (#22655) (#27096)
9a7bda5 
Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>

Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>
@trop trop bot removed the in-flight/12-x-y label Jan 5, 2021
zcbenz pushed a commit that referenced this pull request Jan 5, 2021
@sentialx @nornagon @samuelmaddock
fix(extensions): implement missing web_request hooks (#22655) (#27098)
975123b 
Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>

Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>
zcbenz pushed a commit that referenced this pull request Jan 5, 2021
@sentialx @nornagon @samuelmaddock
fix(extensions): implement missing web_request hooks (#27097)
700c410 
* fix(extensions): implement missing web_request hooks (#22655)

Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>

* fix: remove ukm_source_id

Co-authored-by: Jeremy Apthorp <nornagon@nornagon.net>
Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>
This was referenced Jan 30, 2021
Closed
Merged
@sentialx sentialx mentioned this pull request Feb 14, 2021
Open
@codebytere codebytere mentioned this pull request Jan 6, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@deepak1556 deepak1556 deepak1556 left review comments

@samuelmaddock samuelmaddock samuelmaddock left review comments

@nornagon nornagon nornagon approved these changes

@zcbenz zcbenz zcbenz approved these changes

Assignees
No one assigned
Labels
backport/approved ✅ semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sentialx @nornagon @jkleinsc @samuelmaddock @zcbenz @VerteDinde @deepak1556