From 8ce1930f0daf2a815bdea486880154437061f49e Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 26 Jan 2017 10:34:27 -0800 Subject: [PATCH 1/4] Add specs for missing startDrag options --- atom/browser/api/atom_api_web_contents.cc | 4 ++-- spec/api-web-contents-spec.js | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index afb6f4ce9ddf..0d52112b08c5 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -1327,7 +1327,7 @@ void WebContents::StartDrag(const mate::Dictionary& item, // Error checking. if (icon.IsEmpty()) { - args->ThrowError("icon must be set"); + args->ThrowError("Must specify non-empty 'icon' option"); return; } @@ -1337,7 +1337,7 @@ void WebContents::StartDrag(const mate::Dictionary& item, base::MessageLoop::current()); DragFileItems(files, icon->image(), web_contents()->GetNativeView()); } else { - args->ThrowError("There is nothing to drag"); + args->ThrowError("Must specify either 'file' or 'files' option"); } } diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index fb9c17125aa2..417b8d504607 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -285,4 +285,20 @@ describe('webContents module', function () { }) w.webContents.inspectElement(10, 10) }) + + describe('startDrag({file, icon})', () => { + it('throws errors for a missing file or a missing/empty icon', () => { + assert.throws(() => { + w.webContents.startDrag({icon: path.join(__dirname, 'fixtures', 'assets', 'logo.png')}) + }, /Must specify either 'file' or 'files' option/) + + assert.throws(() => { + w.webContents.startDrag({file: __filename, icon: __filename}) + }, /Must specify non-empty 'icon' option/) + + assert.throws(() => { + w.webContents.startDrag({file: __filename}) + }, /Must specify non-empty 'icon' option/) + }) + }) }) From e683f28e32b4869f2c3461ea8f2e0dcc66ffb1b5 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 26 Jan 2017 10:38:13 -0800 Subject: [PATCH 2/4] Require a non-empty drag image to prevent crash --- atom/browser/api/atom_api_web_contents.cc | 8 ++++++++ spec/api-web-contents-spec.js | 12 +++++++----- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 0d52112b08c5..a8f864c775bd 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -1327,9 +1327,17 @@ void WebContents::StartDrag(const mate::Dictionary& item, // Error checking. if (icon.IsEmpty()) { + args->ThrowError("Must specify 'icon' option"); + return; + } + +#if defined(OS_MACOSX) + // NSWindow.dragImage requires a non-empty NSImage + if (icon->image().IsEmpty()) { args->ThrowError("Must specify non-empty 'icon' option"); return; } +#endif // Start dragging. if (!files.empty()) { diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index 417b8d504607..98aa1169e6ea 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -292,13 +292,15 @@ describe('webContents module', function () { w.webContents.startDrag({icon: path.join(__dirname, 'fixtures', 'assets', 'logo.png')}) }, /Must specify either 'file' or 'files' option/) - assert.throws(() => { - w.webContents.startDrag({file: __filename, icon: __filename}) - }, /Must specify non-empty 'icon' option/) - assert.throws(() => { w.webContents.startDrag({file: __filename}) - }, /Must specify non-empty 'icon' option/) + }, /Must specify 'icon' option/) + + if (process.platform === 'darwin') { + assert.throws(() => { + w.webContents.startDrag({file: __filename, icon: __filename}) + }, /Must specify non-empty 'icon' option/) + } }) }) }) From eb6d92d4273f44c6216b4100382363860826846d Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 26 Jan 2017 11:04:59 -0800 Subject: [PATCH 3/4] Mention image cannot be empty on macOS --- docs/api/web-contents.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index eb64cc0208bc..3c66f1d2d60a 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -1132,8 +1132,9 @@ End subscribing for frame presentation events. #### `contents.startDrag(item)` * `item` Object - * `file` String - * `icon` [NativeImage](native-image.md) + * `file` String - The path to the file being dragged. + * `icon` [NativeImage](native-image.md) - The image must be non-empty on + macOS. Sets the `item` as dragging item for current drag-drop operation, `file` is the absolute path of the file to be dragged, and `icon` is the image showing under From 76e5589a313b62bc561146742256acf0838912c0 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 26 Jan 2017 16:15:10 -0800 Subject: [PATCH 4/4] Schedule function after executeJavaScript completes --- spec/api-browser-window-spec.js | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 9ca429136ed7..51aac5d52daa 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1663,35 +1663,33 @@ describe('BrowserWindow module', function () { }) describe('dev tool extensions', function () { - let showPanelIntervalId + let showPanelTimeoutId const showLastDevToolsPanel = () => { w.webContents.once('devtools-opened', function () { - clearInterval(showPanelIntervalId) - - showPanelIntervalId = setInterval(function () { + const show = function () { if (w == null || w.isDestroyed()) { - clearInterval(showLastDevToolsPanel) return } - const {devToolsWebContents} = w if (devToolsWebContents == null || devToolsWebContents.isDestroyed()) { - clearInterval(showLastDevToolsPanel) return } - var showLastPanel = function () { - var lastPanelId = WebInspector.inspectorView._tabbedPane._tabs.peekLast().id + const showLastPanel = function () { + const lastPanelId = WebInspector.inspectorView._tabbedPane._tabs.peekLast().id WebInspector.inspectorView.showPanel(lastPanelId) } - devToolsWebContents.executeJavaScript(`(${showLastPanel})()`) - }, 100) + devToolsWebContents.executeJavaScript(`(${showLastPanel})()`, false, () => { + showPanelTimeoutId = setTimeout(show, 100) + }) + } + showPanelTimeoutId = setTimeout(show, 100) }) } afterEach(function () { - clearInterval(showPanelIntervalId) + clearTimeout(showPanelTimeoutId) }) describe('BrowserWindow.addDevToolsExtension', function () {