-
Notifications
You must be signed in to change notification settings - Fork 24
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
Feature: Complete migration to CDP #539
Conversation
…l into feat/use-cdp-completely
…l into feat/use-cdp-completely
…l into feat/use-cdp-completely
…l into feat/use-cdp-completely
Add cookies by parsing network urls. Just like application tab does.
CDP
to parse cookies.…l into feat/use-cdp-completely
|
||
if ( | ||
preSetSettings?.allowedNumberOfTabs && | ||
Object.keys(preSetSettings).includes('isUsingCDP') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional Chaining can help
Object.keys(preSetSettings).includes('isUsingCDP') | |
preSetSettings?.isUsingCDP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant do here because it is a boolean variable so we might not know whether or not the variable exists or it is false.
packages/extension/src/serviceWorker/chromeListeners/runtimeOnMessageListener.ts
Outdated
Show resolved
Hide resolved
synchnorousCookieStore.tabMode = sessionStorage.allowedNumberOfTabs; | ||
} | ||
|
||
if (Object.keys(sessionStorage).includes('isUsingCDP')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional Chaining
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant do here because it is a boolean variable so we might not know whether or not the variable exists or it is false.
packages/extension/src/serviceWorker/chromeListeners/runtimeStartUpListener.ts
Outdated
Show resolved
Hide resolved
synchnorousCookieStore.tabMode = storage.allowedNumberOfTabs; | ||
} | ||
|
||
if (Object.keys(storage).includes('isUsingCDP')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional Chaining
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant do here because it is a boolean variable so we might not know whether or not the variable exists or it is false.
Correct JSDoc. Remove tabFramesIdMap.
…l into feat/use-cdp-completely
targets.map(async ({ id, url }) => { | ||
if (url.startsWith('http')) { | ||
await attachCDP({ targetId: id }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we attaching CDP to all targets on all ALLOWED_EVENTS
every time? Can it have any side effects when the same target is attached CDP more than once, like it happens with event listeners in JavaScript?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Great work, thanks.
Description
This PR aims to use CDP completely rather than using the
chrome.webRequest
API along withCDP
.Relevant Technical Choices
Frame Tree Creation and Tab Identification:
requestWillBeSent
andresponseReceived
are sometimes not available in thechrome.debugger.getTargets()
.source.targetId
is defined which we use to look into the frameTree of the tab if that target exists then we get the TabId of the tab and use it to update.Cookie analysis:
requestWillBeSent
andresponseReceived
events provideframeId
andurls
.responseReceivedExtraInfo
andrequestWillBeSentExtraInfo
provide information about cookies.extraInfo
is not processed until their respectiverequestWillBeSent
orresponseReceived
has not arrived.extraInfo
arrives before their counterpart, they are stored in a map. Once the counterpart arrives they are processed.extraInfo
arrives after the counterpart, they are processed immediately.Network.getCookies
every 5 seconds to avoid any mismatch of the data.Testing Instructions
Enable CDP
then click on Reload.Additional Information:
Screenshot/Screencast
Checklist