feat: split openExternal into sync and async

atom/common/api/atom_api_shell.cc

@@ -10,6 +10,7 @@
 #include "atom/common/native_mate_converters/string16_converter.h"
 #include "atom/common/node_includes.h"
 #include "atom/common/platform_util.h"
+#include "atom/common/promise_util.h"
 #include "native_mate/dictionary.h"
 
 #if defined(OS_WIN)
@@ -43,17 +44,24 @@ struct Converter<base::win::ShortcutOperation> {
 
 namespace {
 
-void OnOpenExternalFinished(
-    v8::Isolate* isolate,
-    const base::Callback<void(v8::Local<v8::Value>)>& callback,
-    const std::string& error) {
+void OnOpenExternalFinished(const v8::Global<v8::Context>& context,
+                            scoped_refptr<atom::util::Promise> promise,
+                            const std::string& error) {
+  v8::Isolate* isolate = promise->isolate();
+  mate::Locker locker(isolate);
+  v8::HandleScope handle_scope(isolate);
+  v8::MicrotasksScope script_scope(isolate,
+                                   v8::MicrotasksScope::kRunMicrotasks);
+  v8::Context::Scope context_scope(
+      v8::Local<v8::Context>::New(isolate, context));
+
   if (error.empty())
-    callback.Run(v8::Null(isolate));
+    promise->Resolve();
   else
-    callback.Run(v8::String::NewFromUtf8(isolate, error.c_str()));
+    promise->RejectWithErrorMessage(error.c_str());
 }
 
-bool OpenExternal(
+bool OpenExternalSync(
 #if defined(OS_WIN)
     const base::string16& url,
 #else
@@ -69,17 +77,35 @@ bool OpenExternal(
     }
   }
 
-  if (args->Length() >= 3) {
-    base::Callback<void(v8::Local<v8::Value>)> callback;
-    if (args->GetNext(&callback)) {
-      platform_util::OpenExternal(
-          url, options,
-          base::Bind(&OnOpenExternalFinished, args->isolate(), callback));
-      return true;
+  return platform_util::OpenExternal(url, options);
+}
+
+v8::Local<v8::Promise> OpenExternal(
+#if defined(OS_WIN)
+    const base::string16& url,
+#else
+    const GURL& url,
+#endif
+    mate::Arguments* args) {
+  scoped_refptr<atom::util::Promise> promise =
+      new atom::util::Promise(args->isolate());
+
+  platform_util::OpenExternalOptions options;
+  if (args->Length() >= 2) {
+    mate::Dictionary obj;
+    if (args->GetNext(&obj)) {
+      obj.Get("activate", &options.activate);
+      obj.Get("workingDirectory", &options.working_dir);
     }
   }
 
-  return platform_util::OpenExternal(url, options);
+  v8::Global<v8::Context> context(args->isolate(),
+                                  args->isolate()->GetCurrentContext());
+  platform_util::OpenExternal(
+      url, options,
+      base::Bind(&OnOpenExternalFinished, std::move(context), promise));
+
+  return promise->GetHandle();
 }
 
 #if defined(OS_WIN)
@@ -144,6 +170,7 @@ void Initialize(v8::Local<v8::Object> exports,
   mate::Dictionary dict(context->GetIsolate(), exports);
   dict.SetMethod("showItemInFolder", &platform_util::ShowItemInFolder);
   dict.SetMethod("openItem", &platform_util::OpenItem);
+  dict.SetMethod("openExternalSync", &OpenExternalSync);
   dict.SetMethod("openExternal", &OpenExternal);
   dict.SetMethod("moveItemToTrash", &platform_util::MoveItemToTrash);
   dict.SetMethod("beep", &platform_util::Beep);

default_app/menu.js

@@ -13,27 +13,27 @@ const setDefaultApplicationMenu = () => {
       {
         label: 'Learn More',
         click () {
-          shell.openExternal('https://electronjs.org')
+          shell.openExternalSync('https://electronjs.org')
         }
       },
       {
         label: 'Documentation',
         click () {
-          shell.openExternal(
+          shell.openExternalSync(
             `https://github.com/electron/electron/tree/v${process.versions.electron}/docs#readme`
           )
         }
       },
       {
         label: 'Community Discussions',
         click () {
-          shell.openExternal('https://discuss.atom.io/c/electron')
+          shell.openExternalSync('https://discuss.atom.io/c/electron')
         }
       },
       {
         label: 'Search Issues',
         click () {
-          shell.openExternal('https://github.com/electron/electron/issues')
+          shell.openExternalSync('https://github.com/electron/electron/issues')
         }
       }
     ]

default_app/renderer.js

@@ -21,7 +21,7 @@ function initialize () {
 
     const openLinkExternally = (e) => {
       e.preventDefault()
-      shell.openExternal(url)
+      shell.openExternalSync(url)
     }
 
     link.addEventListener('click', openLinkExternally)

docs/api/menu.md

@@ -245,7 +245,7 @@ const template = [
     submenu: [
       {
         label: 'Learn More',
-        click () { require('electron').shell.openExternal('https://electronjs.org') }
+        click () { require('electron').shell.openExternalSync('https://electronjs.org') }
       }
     ]
   }

docs/api/promisification.md

@@ -8,7 +8,6 @@ When a majority of affected functions are migrated, this flag will be enabled by
 
 ### Candidate Functions
 
-- [ ] [app.getFileIcon(path[, options], callback)](https://github.com/electron/electron/blob/master/docs/api/app.md#getFileIcon)
 - [ ] [app.importCertificate(options, callback)](https://github.com/electron/electron/blob/master/docs/api/app.md#importCertificate)
 - [ ] [request.write(chunk[, encoding][, callback])](https://github.com/electron/electron/blob/master/docs/api/client-request.md#write)
 - [ ] [request.end([chunk][, encoding][, callback])](https://github.com/electron/electron/blob/master/docs/api/client-request.md#end)
@@ -41,7 +40,6 @@ When a majority of affected functions are migrated, this flag will be enabled by
 - [ ] [ses.clearHostResolverCache([callback])](https://github.com/electron/electron/blob/master/docs/api/session.md#clearHostResolverCache)
 - [ ] [ses.getBlobData(identifier, callback)](https://github.com/electron/electron/blob/master/docs/api/session.md#getBlobData)
 - [ ] [ses.clearAuthCache(options[, callback])](https://github.com/electron/electron/blob/master/docs/api/session.md#clearAuthCache)
-- [ ] [shell.openExternal(url[, options, callback])](https://github.com/electron/electron/blob/master/docs/api/shell.md#openExternal)
 - [ ] [contents.executeJavaScript(code[, userGesture, callback])](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#executeJavaScript)
 - [ ] [contents.getZoomFactor(callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#getZoomFactor)
 - [ ] [contents.getZoomLevel(callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#getZoomLevel)
@@ -61,4 +59,6 @@ When a majority of affected functions are migrated, this flag will be enabled by
 
 - [ ] [win.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/browser-window.md#capturePage)
 - [ ] [webviewTag.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/webview-tag.md#capturePage)
-- [ ] [contents.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#capturePage)
+- [ ] [contents.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#capturePage)
+- [ ] [app.getFileIcon(path[, options], callback)](https://github.com/electron/electron/blob/master/docs/api/app.md#getFileIcon)
+- [ ] [shell.openExternal(url[, options, callback])](https://github.com/electron/electron/blob/master/docs/api/shell.md#openExternal)

docs/api/shell.md

@@ -34,21 +34,29 @@ Returns `Boolean` - Whether the item was successfully opened.
 
 Open the given file in the desktop's default manner.
 
-### `shell.openExternal(url[, options, callback])`
+### `shell.openExternalSync(url[, options])`
 
-* `url` String - Max 2081 characters on windows, or the function returns false.
+* `url` String - Max 2081 characters on Windows, or the function returns false.
 * `options` Object (optional)
   * `activate` Boolean (optional) - `true` to bring the opened application to the
     foreground. The default is `true`. _macOS_
   * `workingDirectory` String (optional) - The working directory. _Windows_
-* `callback` Function (optional) _macOS_ - If specified will perform the open asynchronously.
-  * `error` Error
 
 Returns `Boolean` - Whether an application was available to open the URL.
-If callback is specified, always returns true.
 
-Open the given external protocol URL in the desktop's default manner. (For
-example, mailto: URLs in the user's default mail agent).
+Open the given external protocol URL in the desktop's default manner. (For example, mailto: URLs in the user's default mail agent).
+
+### `shell.openExternal(url[, options])`
+
+* `url` String - Max 2081 characters on windows.
+* `options` Object (optional)
+  * `activate` Boolean (optional) - `true` to bring the opened application to the
+    foreground. The default is `true`. _macOS_
+  * `workingDirectory` String (optional) - The working directory. _Windows_
+
+Returns `Promise<void>`
+
+Open the given external protocol URL in the desktop's default manner. (For example, mailto: URLs in the user's default mail agent).
 
 ### `shell.moveItemToTrash(fullPath)`
 

docs/api/webview-tag.md

@@ -782,7 +782,7 @@ const webview = document.querySelector('webview')
 webview.addEventListener('new-window', (e) => {
   const protocol = require('url').parse(e.url).protocol
   if (protocol === 'http:' || protocol === 'https:') {
-    shell.openExternal(e.url)
+    shell.openExternalSync(e.url)
   }
 })
 ```

docs/tutorial/security.md

@@ -639,7 +639,7 @@ app.on('web-contents-created', (event, contents) => {
     // to open this event's url in the default browser.
     event.preventDefault()
 
-    shell.openExternal(navigationUrl)
+    shell.openExternalSync(navigationUrl)
   })
 })
 ```

spec/api-remote-spec.js

@@ -198,7 +198,7 @@ describe('remote module', () => {
       assert.strictEqual(typeof remote.app.getPath, 'function')
       assert.strictEqual(typeof remote.webContents.getFocusedWebContents, 'function')
       assert.strictEqual(typeof remote.clipboard.readText, 'function')
-      assert.strictEqual(typeof remote.shell.openExternal, 'function')
+      assert.strictEqual(typeof remote.shell.openExternalSync, 'function')
     })
 
     it('returns toString() of original function via toString()', () => {

spec/api-shell-spec.js

@@ -16,14 +16,55 @@ describe('shell module', () => {
     iconIndex: 1
   }
 
-  // (alexeykuzmin): `.skip()` in `before` doesn't work for nested `describe`s.
-  beforeEach(function () {
-    if (process.platform !== 'win32') {
-      this.skip()
-    }
+  describe('shell.openExternal()', () => {
+    let envVars = {}
+
+    beforeEach(function () {
+      envVars = {
+        display: process.env.DISPLAY,
+        de: process.env.DE,
+        browser: process.env.BROWSER
+      }
+    })
+
+    afterEach(() => {
+      // reset env vars to prevent side effects
+      if (process.platform === 'linux') {
+        process.env.DE = envVars.de
+        process.env.BROWSER = envVars.browser
+        process.env.DISPLAY = envVars.display
+      }
+    })
+
+    it('opens an external link asynchronously', done => {
+      const url = 'http://www.example.com'
+      if (process.platform === 'linux') {
+        process.env.BROWSER = '/bin/true'
+        process.env.DE = 'generic'
+        process.env.DISPLAY = ''
+      }
+
+      shell.openExternal(url).then(() => done())
+    })
+
+    it('opens an external link synchronously', () => {
+      const url = 'http://www.example.com'
+      if (process.platform === 'linux') {
+        process.env.DE = 'generic'
+        process.env.DE = '/bin/true'
+        process.env.DISPLAY = ''
+      }
+
+      const success = shell.openExternalSync(url)
+      assert.strictEqual(true, success)
+    })
   })
 
   describe('shell.readShortcutLink(shortcutPath)', () => {
+    beforeEach(function () {
+      if (process.platform !== 'win32') this.skip()
+    })
+
     it('throws when failed', () => {
       assert.throws(() => {
         shell.readShortcutLink('not-exist')
@@ -37,6 +78,10 @@ describe('shell module', () => {
   })
 
   describe('shell.writeShortcutLink(shortcutPath[, operation], options)', () => {
+    beforeEach(function () {
+      if (process.platform !== 'win32') this.skip()
+    })
+
     const tmpShortcut = path.join(os.tmpdir(), `${Date.now()}.lnk`)
 
     afterEach(() => {