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

Update setWindowOpenHandler "features" docs #30325

Closed
wants to merge 1 commit into from

Conversation

Prinzhorn
Copy link
Contributor

Description of Change

Since the introduction of setWindowOpenHandler features is no longer parsed but passed as a raw string (see #28518 (comment)). I've switched to JSON to pass data through window.open for my multi-window application. The docs should reflect that this can be an arbitrary string.

Checklist

  • PR description included and stakeholders cc'd
  • relevant documentation is changed or added

Release Notes

None

Since the introduction of `setWindowOpenHandler` `features` is no longer parsed but passed as a raw string (see electron#28518 (comment)). I've switched to JSON to pass data through `window.open` for my multi-window application. The docs should reflect that this can be an arbitrary string.
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 29, 2021
@pushkin-
Copy link

Huh, do you think it's safe to rely on passing invalid window features (like JSON) into window.open? I'm experimenting with a similar thing where I'm passing custom window features into window.open and intercepting them in the handler in Electron, but I worry that Chrome could make a change to start stripping out invalid features. Not sure if it's safe to rely on those always being filled out - unless the specs guarantee that invalid window features will be kept in the final string.

Though I guess now that I think about it, I'm guessing the features string is exactly what was supplied to window.open with no modifications, in which case this is a non-issue.

@Prinzhorn
Copy link
Contributor Author

@pushkin- I hope someone from the Electron team can clarify. But I think the point of setWindowOpenHandler is to give us control over window.open. I assume the features string is not even parsed at all, because Electron catches it before Chromium gets a chance to do its thing (that's the whole point of setWindowOpenHandler). So I'd be surprised if the string was manipulated in any way. If Chromium would break this then Electron will unbreak it for us with a patch.

@zcbenz zcbenz requested a review from nornagon August 2, 2021 01:17
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 5, 2021
@zcbenz
Copy link
Member

zcbenz commented Aug 10, 2021

/cc people who had worked on the setWindowOpenHandler API @loc @nornagon @VerteDinde

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.

We currently patch in the raw_features property:

+ // Extra fields added by Electron.
+ string raw_features;
+ network.mojom.URLRequestBody? body;

I would recommend against passing arbitrary JSON in the features string as the string is parsed by Chromium to determine other features: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/page/create_window.cc;l=69;drc=ce29454af324a1f7b72af4eeed3145c63f9405a9

an arbitrary JSON string may accidentally trigger some of these features unintentionally.

I don't think we should indicate that this can be an arbitrary string.

@Prinzhorn
Copy link
Contributor Author

Thanks for clarifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants