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

Add WebRequest API #7

Merged
merged 29 commits into from
Nov 28, 2017
Merged

Add WebRequest API #7

merged 29 commits into from
Nov 28, 2017

Conversation

hugomano
Copy link
Contributor

@hugomano hugomano commented Nov 23, 2017

Todo

  • Add removeListener
  • Add hasListener
  • Abstract RPC
  • Clean code
  • Implement calls in dummy extension
  • Bump version

@hugomano hugomano requested a review from alexstrat November 23, 2017 16:05
@hugomano hugomano changed the title Add WebRequest API [WIP] Add WebRequest API Nov 23, 2017
@hugomano
Copy link
Contributor Author

hugomano commented Nov 23, 2017

@alexstrat, right now the electrons api works well, tested with custom agent header. The cancel callback work also in the dummy extension callback({cancel: false, requestHeaders: details.requestHeaders}). The cycle is complete between main and renderer processes but the electron don't handle rules.

Lulumi implementation:
Injector
WebRequestEvent
Browser side

Chromium: API

@hugomano
Copy link
Contributor Author

hugomano commented Nov 23, 2017

capture d ecran 2017-11-23 a 20 04 30

@alexstrat WTF?

Tab console on the left and background console on the right...
Log same lines

@hugomano
Copy link
Contributor Author

@alexstrat ready for a review. I've tried to abstract the RPC but it's not really necessary I think. We will add a lot of lines for the same behaviour.

@hugomano hugomano changed the title [WIP] Add WebRequest API Add WebRequest API Nov 27, 2017
@@ -36,6 +36,8 @@ constants.TABS_SEND_MESSAGE_RESULT_ = 'CHROME_TABS_SEND_MESSAGE_RESULT_';
constants.TABS_EXECUTESCRIPT = 'CHROME_TABS_EXECUTESCRIPT';
constants.TABS_EXECUTESCRIPT_RESULT_ = 'CHROME_TABS_EXECUTESCRIPT_RESULT_';

constants.TABS_EXECUTESCRIPT_RESULT_ = 'CHROME_TABS_EXECUTESCRIPT_RESULT_';
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

"permissions": [
"webRequest",
"webRequestBlocking",
"*://*/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

One day we'll need to handle this correctly :) one day

example/main.js Outdated
urls: ['<all_urls>']
}

session.defaultSession.webRequest.onResponseStarted(filter, (details) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

@hugomano
Copy link
Contributor Author

@alexstrat done

const fromChromeObjectToElectronObject = (details) => {
if (details.responseHeaders) {
const responseHeaders = [];
Object.keys(details.responseHeaders).forEach((k) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Array#map ;)

if (cbReturn && cbReturn.responseHeaders) {
const responseHeaders = {};
cbReturn.responseHeaders.forEach((header) => {
return responseHeaders[header.name] = [header.value]
Copy link
Contributor

Choose a reason for hiding this comment

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

return is confusing

}

removeListener (callback) {
const digest = callbackToDigest(callback);
Copy link
Contributor

@alexstrat alexstrat Nov 28, 2017

Choose a reason for hiding this comment

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

Still not sure why you need a callbackToDigest

This could not work here👇: const index = this.listeners.findIndex(cb => cb === callback); ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Edited

this.rpcManager = new RpcIpcManager(lib, this.rpcScopeRenderer);
}

handlerBehaviorChanged () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's now ddd a console.warning when dummy-mock methods

'onHeadersReceived', 'onAuthRequired', 'onResponseStarted', 'onBeforeRedirect', 'onCompleted',
'onErrorOccurred'];

const fromElectronObjectToChromeObject = (cbReturn) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: to be clear, I guess you can reduce the scope of this utility to only headers and rename it more precisely

@alexstrat
Copy link
Contributor

@hugomano regarding abstraction, I suggested something in #8. It's only pseudo-code. Let me know if it helps

}
}

module.exports = ChromeWebRequestAPIHandler;
Copy link
Contributor

@alexstrat alexstrat Nov 28, 2017

Choose a reason for hiding this comment

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

Minor: end of line

this.controlerRPCScope = `${eventId}-controller`;
this.eventRPCScope = `${eventId}-event`;

this.rcpManager = new RpcIpcManager({
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: type rpc

try {
return listener.call(this, args);
} catch (e) {
return e
Copy link
Contributor

@alexstrat alexstrat Nov 28, 2017

Choose a reason for hiding this comment

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

Rather console.error and return nothing, otherwise it'll try to pass it into callback on main's side

Copy link
Contributor

Choose a reason for hiding this comment

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

unless electron-simple-rpc is smart enough to recognize it's an error.
However we don't care

@alexstrat
Copy link
Contributor

One last non minor change and we're good to go ;)

@alexstrat alexstrat merged commit 6f48209 into master Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants