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 WebContents.opener and webContents.fromFrame() #35140

Merged
merged 5 commits into from Sep 26, 2022

Conversation

samuelmaddock
Copy link
Member

@samuelmaddock samuelmaddock commented Jul 29, 2022

Description of Change

closes #24807

Adds the ability to get the window opener of a WebContents.

// From WebContents instance
contents.opener;

// From WebFrameMain instance
webContents.fromFrame(frame)?.opener;

I'd like to also add WebFrameMain.opener, but there's no public API for this in render_frame_host.h

Checklist

Release Notes

Notes:

  • Added WebContents.opener to access window opener.
  • Added webContents.fromFrame(frame) to get the WebContents corresponding to a WebFrameMain instance.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 29, 2022
@samuelmaddock samuelmaddock added the semver/minor backwards-compatible functionality label Jul 29, 2022
@MarshallOfSound
Copy link
Member

So if an iframe opens a child window. childWindow.webContents.opener will point to windowOwningIframe not the iframe itself?

@samuelmaddock
Copy link
Member Author

So if an iframe opens a child window. childWindow.webContents.opener will point to windowOwningIframe not the iframe itself?

opener returns an instance of RenderFrameHost which would be an iframe in that case. I based the docs definition off of MDN's description. Realizing now that "window" is ambiguous though.

@samuelmaddock samuelmaddock added the target/21-x-y PR should also be added to the "21-x-y" branch. label Aug 3, 2022
@nornagon
Copy link
Member

I feel like there must be a way to validate that a RFH is controlled by a particular security origin! Perhaps SiteInstance is the way here? render_frame_host()->GetSiteInstance()->GetSecurityOrigin() maybe? Though SiteInstance isn't reliable in the face of non-site-per-process process models.

I feel like going through WebContents here is weird and wrong though. If Site A has a cross-origin iframe B in it, and B opens a child window C, would C.webContents.opener give A.webContents? That seems bad!

@nornagon
Copy link
Member

Is this no longer needed now that we have webFrameMain.origin?

@samuelmaddock
Copy link
Member Author

Is this no longer needed now that we have webFrameMain.origin?

Yeah, let's close unless we find another use case for it.

@samuelmaddock
Copy link
Member Author

Is this no longer needed now that we have webFrameMain.origin?

Yeah, let's close unless we find another use case for it.

@pushkin-
Copy link

Isn't my issue that would have been fixed by this a use case? The origin from what I can tell by that PR is just the URL (unless I misinterpreted something).

@samuelmaddock
Copy link
Member Author

Isn't my issue that would have been fixed by this a use case? The origin from what I can tell by that PR is just the URL (unless I misinterpreted something).

Ah yes, sorry about that. I believe the API surface is small enough that there's little risk in introducing it.

@nornagon lmk you're thoughts on still merging this to address the linked issue.

@samuelmaddock samuelmaddock reopened this Sep 13, 2022
* `frame` WebFrameMain

Returns `WebContents` | undefined - A WebContents instance with the given WebFrameMain, or
`undefined` if there is no WebContents associated with the given WebFrameMain.
Copy link
Member

Choose a reason for hiding this comment

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

hm... in what situation is there no WebContents for a WebFrameMain?

Copy link
Member Author

Choose a reason for hiding this comment

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

More accurately, this is for when the WebContents has been destroyed while the frame reference is held around. One of the tests in this PR checks this.

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.

Yeah this seems reasonable in that case. API LGTM

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.

API LGTM

@jkleinsc jkleinsc removed the new-pr 🌱 PR opened in the last 24 hours label Sep 26, 2022
@jkleinsc jkleinsc merged commit c09c94f into electron:main Sep 26, 2022
@release-clerk
Copy link

release-clerk bot commented Sep 26, 2022

Release Notes Persisted

  • Added WebContents.opener to access window opener.
  • Added webContents.fromFrame(frame) to get the WebContents corresponding to a WebFrameMain instance.

@trop
Copy link
Contributor

trop bot commented Sep 26, 2022

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

@trop trop bot added in-flight/21-x-y and removed target/21-x-y PR should also be added to the "21-x-y" branch. labels Sep 26, 2022
@samuelmaddock samuelmaddock deleted the feat/webcontents-opener branch September 26, 2022 16:40
@trop trop bot added merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/21-x-y labels Sep 28, 2022
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
)

* feat: add WebContents.opener

* feat: add webContents.fromFrame(frame)

* fix: unknown type name

* test: fix and add more fromFrame cases

* docs: clarified terminology
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/requested 🗳 merged/21-x-y PR was merged to the "21-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow us to discover what window opened the current window
5 participants