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

Implement alert/confirm through Chrome's dialog manager #8341

Merged
merged 3 commits into from Jan 9, 2017

Conversation

Projects
None yet
3 participants
@kevinsawicki
Contributor

kevinsawicki commented Jan 5, 2017

This allows window.alert and window.confirm to work from <iframe> tags and sandboxed windows.

Refs #2644

@kevinsawicki kevinsawicki changed the title from Implement alert/confirm through dialog manager to Implement alert/confirm through Chrome's dialog manager Jan 5, 2017

@kevinsawicki kevinsawicki requested a review from deepak1556 Jan 5, 2017

@deepak1556

The overrides in lib/renderer/override,js and lib/renderer/inspector.js can be removed ?

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jan 6, 2017

Contributor

The overrides in lib/renderer/override,js can be removed ?

I didn't remove them because they both support a second title parameter that isn't part of the DOM API and so it isn't supported via the Chrome dialog manager. And so I didn't want to break any existing behavior where apps are passing a custom title.

We could remove them in 2.0 though.

Contributor

kevinsawicki commented Jan 6, 2017

The overrides in lib/renderer/override,js can be removed ?

I didn't remove them because they both support a second title parameter that isn't part of the DOM API and so it isn't supported via the Chrome dialog manager. And so I didn't want to break any existing behavior where apps are passing a custom title.

We could remove them in 2.0 though.

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Jan 6, 2017

Member

Makes sense, thanks for the explanation 👍

Member

deepak1556 commented Jan 6, 2017

Makes sense, thanks for the explanation 👍

@kevinsawicki kevinsawicki merged commit 871c3fc into master Jan 9, 2017

7 of 9 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #5129729 succeeded in 66s
Details
electron-linux-ia32 Build #5129730 succeeded in 171s
Details
electron-linux-x64 Build #5129731 succeeded in 237s
Details
electron-mas-x64 Build #3118 succeeded in 8 min 50 sec
Details
electron-osx-x64 Build #3131 succeeded in 8 min 58 sec
Details
electron-win-ia32 Build #2165 succeeded in 13 min
Details
electron-win-x64 Build #2153 succeeded in 13 min
Details

@kevinsawicki kevinsawicki deleted the dialog-manager branch Jan 9, 2017

@jackywang863

This comment has been minimized.

Show comment
Hide comment
@jackywang863

jackywang863 Jul 13, 2017

in newest master sources we did not found the above commits, so when commit the 3 file into master?

jackywang863 commented Jul 13, 2017

in newest master sources we did not found the above commits, so when commit the 3 file into master?

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