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

feat: add contents.getOSProcessIdForFrame() #18769

Closed
wants to merge 2 commits into from
Closed

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Jun 13, 2019

Description of Change

Added webContents.getOSProcessIdForFrame(). This allows getting the pid of renderers hosting cross-origin frames.

Required for tests in #18650

Checklist

Release Notes

Notes: Added webContents.getOSProcessIdForFrame(). This allows getting the pid of renderers hosting cross-origin frames.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 13, 2019
@miniak miniak self-assigned this Jun 13, 2019
@miniak miniak added the wip ⚒ label Jun 13, 2019
@miniak miniak changed the title feat: add frameId argument to contents.getOSProcessId() feat: add contents.getOSProcessIdForFrame() Jun 13, 2019
@miniak miniak removed the wip ⚒ label Jun 13, 2019
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM, with additional spec changes. Thanks!

const [{ frameId: frameIdMain }, pidMain] = await promise1
const [{ frameId: frameIdFrame }, pidFrame] = await promise2

expect(w.webContents.getOSProcessIdForFrame(frameIdMain)).to.equal(pidMain)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a check for verifying the process ids are different and the frame ids are also different between the main frame and iframe.

The reason for frame id check is that its based on routing id, which maybe unique at the moment but if chromium makes them per process identifier then we should switch to something like GlobalFrameRoutingId. This spec would help recover from such changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deepak1556 I am actually thinking about changing the API lookup by GetFrameName instead

@miniak miniak added the wip ⚒ label Jun 13, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 14, 2019
@miniak miniak closed this Jun 15, 2019
@codebytere codebytere deleted the miniak/pid-for-frame branch June 16, 2019 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants