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.

Sign up for GitHub

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

Jump to bottom

Parent's visibility trumps inherited 'show' option #12228

Merged
merged 5 commits into from Mar 13, 2018
Merged

Parent's visibility trumps inherited 'show' option #12228

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
685948b
Parent's visibility trumps inherited 'show' option
ckerr Mar 12, 2018
e4d5ae5
Add tests
ckerr Mar 13, 2018
379bd79
Remove unnecessary work when merging options
ckerr Mar 13, 2018
2d90e76
Use idiomatic ES6 when merging options
ckerr Mar 13, 2018
e400585
Apply further ES6 bikeshedding
ckerr Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion lib/browser/guest-window-manager.js
View file
Open in desktop
Expand Up @@ -47,8 +47,19 @@ const mergeBrowserWindowOptions = function (embedder, options) {
options.webPreferences = {}
}
if (embedder.browserWindowOptions != null) {
let parentOptions = embedder.browserWindowOptions

// if parent's visibility is available, that overrides 'show' flag (#12125)
const win = BrowserWindow.fromWebContents(embedder.webContents)
if (win != null) {
const show = win.isVisible()
if (typeof show === 'boolean') {
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs say that isVisible() always returns a boolean
https://github.com/electron/electron/blob/master/docs/api/browser-window.md#winisvisible

Copy link
Member

Choose a reason for hiding this comment

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

Yeah didn't really query this, but looking at it, it can only ever be true/false right?

parentOptions = {...embedder.browserWindowOptions, show}
}
}

// Inherit the original options if it is a BrowserWindow.
mergeOptions(options, embedder.browserWindowOptions)
mergeOptions(options, parentOptions)
} else {
// Or only inherit webPreferences if it is a webview.
mergeOptions(options.webPreferences, embedder.getWebPreferences())
Expand Down
20 changes: 20 additions & 0 deletions spec/chromium-spec.js
View file
Open in desktop
Expand Up @@ -223,6 +223,26 @@ describe('chromium feature', () => {
b = window.open(`file://${fixtures}/pages/window-open-size.html`, '', 'show=no')
})

for (const show in [true, false]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for (const show of [true, false])

of iterates over array entries (kinda), in over an object properties.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in

Copy link
Member

Choose a reason for hiding this comment

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

Lol, I love how this works regardless though, because 0 is falsey and 1 is truthy 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

shia_lebeouf_magic.gif

it(`inherits parent visibility over parent {show=${show}} option`, (done) => {
let w = new BrowserWindow({show: show})
Copy link
Contributor

Choose a reason for hiding this comment

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

const w = new BrowserWindow({show})

Copy link
Member

Choose a reason for hiding this comment

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

technically the let shouldn't be here, but w is declared slightly higher as a let so we can't make it const here 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can. You just declare a new variable that simply shadows the one declared above it.


// toggle visibility
if (show) {
Copy link
Member

Choose a reason for hiding this comment

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

(nit) for something this short, maybe use a ternary?

Copy link
Member Author

@ckerr ckerr Mar 13, 2018 •
edited

Choose a reason for hiding this comment

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

Tried it that way and wound up in an ad-hoc bikeshed session with Sam and Alexey with more opinions than people. Let's leave it as-is. 😆

Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me 😁 already approved, ready when CI goes ✅!

w.hide()
} else {
w.show()
}

w.webContents.once('new-window', (e, url, frameName, disposition, options) => {
assert.equal(options.show, w.isVisible())
w.close()
done()
})
w.loadURL(`file://${fixtures}/pages/window-open.html`)
})
}

it('disables node integration when it is disabled on the parent window', (done) => {
let b
listener = (event) => {
Expand Down