Fix for missing additional features fields when creating new popup windows #7291

Merged
merged 4 commits into from Oct 4, 2016

Projects

None yet

4 participants

@MichaelVasseur
Contributor
MichaelVasseur commented Sep 21, 2016 edited

The non-standard features (like this-is-not-a-standard-feature) given to window.open() are lost in the popup window creation workflow. This leads to the 'new-window' event handler not receiving these 'additional' features. These features may be used by applications to give some specific data to the popup.

NOTE : This pull request depends on pull request #234 in libchromiumcontent repo.

@zeke
Member
zeke commented Sep 22, 2016

@MichaelVasseur can you add a note to https://github.com/electron/electron/blob/master/docs/api/window-open.md explaining the change you've made here?

@MichaelVasseur
Contributor

@zeke I pushed the changes to the documentation. Thanks for your feedback !

atom/browser/api/atom_api_app.cc
@@ -525,6 +525,7 @@ void App::OnLogin(LoginHandler* login_handler,
void App::OnCreateWindow(const GURL& target_url,
const std::string& frame_name,
WindowOpenDisposition disposition,
+ const std::vector<base::string16>& features,
@MarshallOfSound
MarshallOfSound Sep 23, 2016 Member

This indenting seems off by quite a bit, it should be aligned with the line above

atom/browser/api/atom_api_app.h
@@ -50,6 +50,7 @@ class App : public AtomBrowserClient::Delegate,
void OnCreateWindow(const GURL& target_url,
const std::string& frame_name,
WindowOpenDisposition disposition,
+ const std::vector<base::string16>& features,
@MarshallOfSound
MarshallOfSound Sep 23, 2016 Member

Indenting again, maybe your editor is configured from tabs / spaces incorrectly?

@MichaelVasseur
Contributor

@MarshallOfSound Indentation issues fixed.

@MichaelVasseur
Contributor
MichaelVasseur commented Sep 23, 2016 edited

Remaining errors are due to C++ interfaces changes required (introduced in #234)

@zcbenz zcbenz self-assigned this Oct 4, 2016
@zcbenz
Contributor
zcbenz commented Oct 4, 2016

👍

@zcbenz zcbenz merged commit bf21892 into electron:master Oct 4, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@MichaelVasseur
Contributor

I added a new PR #7494 to unit test this change.

@aluxian-huginn aluxian-huginn referenced this pull request in Aluxian/Messenger-for-Desktop Oct 6, 2016
Closed

Update dependency: electron v1.4.3 #723

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