feat: split openExternal into sync and async (#16176)

* feat: split openExternal into sync and async

* v8::Locker => mate::Locker

* fix: enter js env when resolving promise

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(
+void OnOpenExternalFinished(const v8::Global<v8::Context>& context,
-    v8::Isolate* isolate,
+                            scoped_refptr<atom::util::Promise> promise,
-    const base::Callback<void(v8::Local<v8::Value>)>& callback,
+                            const std::string& error) {
-    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) {
+  return platform_util::OpenExternal(url, options);
-    base::Callback<void(v8::Local<v8::Value>)> callback;
+}
-    if (args->GetNext(&callback)) {
+
-      platform_util::OpenExternal(
+v8::Local<v8::Promise> OpenExternal(
-          url, options,
+#if defined(OS_WIN)
-          base::Bind(&OnOpenExternalFinished, args->isolate(), callback));
+    const base::string16& url,
-      return true;
+#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,13 +13,13 @@ 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`
           )
         }
@@ -27,13 +27,13 @@ const setDefaultApplicationMenu = () => {
       {
         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)
@@ -62,3 +60,5 @@ 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)
+- [ ] [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
+Open the given external protocol URL in the desktop's default manner. (For example, mailto: URLs in the user's default mail agent).
-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.
+  describe('shell.openExternal()', () => {
-  beforeEach(function () {
+    let envVars = {}
-    if (process.platform !== 'win32') {
+
-      this.skip()
+    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(() => {

spec/package-lock.json

@@ -72,7 +72,7 @@
     },
     "bl": {
       "version": "1.2.2",
-      "resolved": "http://registry.npmjs.org/bl/-/bl-1.2.2.tgz",
+      "resolved": "https://registry.npmjs.org/bl/-/bl-1.2.2.tgz",
       "integrity": "sha512-e8tQYnZodmebYDWGH7KMRvtzKXaJHx3BbilrgZCfvyLUYdKpK1t5PSPmpkny/SgiTSCnjfLW7v5rlONXVFkQEA==",
       "optional": true,
       "requires": {
@@ -228,7 +228,7 @@
     },
     "commander": {
       "version": "2.15.1",
-      "resolved": "http://registry.npmjs.org/commander/-/commander-2.15.1.tgz",
+      "resolved": "https://registry.npmjs.org/commander/-/commander-2.15.1.tgz",
       "integrity": "sha512-VlfT9F3V0v+jr4yxPc5gg9s62/fIVWsd2Bk2iD435um1NlGMYdVCq+MjcXnhYq2icNOizHr1kK+5TI6H0Hy0ag==",
       "dev": true
     },
@@ -267,7 +267,7 @@
     },
     "dbus-native": {
       "version": "0.2.5",
-      "resolved": "http://registry.npmjs.org/dbus-native/-/dbus-native-0.2.5.tgz",
+      "resolved": "https://registry.npmjs.org/dbus-native/-/dbus-native-0.2.5.tgz",
       "integrity": "sha512-ocxMKCV7QdiNhzhFSeEMhj258OGtvpANSb3oWGiotmI5h1ZIse0TMPcSLiXSpqvbYvQz2Y5RsYPMNYLWhg9eBw==",
       "dev": true,
       "requires": {
@@ -358,7 +358,7 @@
     },
     "duplexer": {
       "version": "0.1.1",
-      "resolved": "http://registry.npmjs.org/duplexer/-/duplexer-0.1.1.tgz",
+      "resolved": "https://registry.npmjs.org/duplexer/-/duplexer-0.1.1.tgz",
       "integrity": "sha1-rOb/gIwc5mtX0ev5eXessCM0z8E=",
       "dev": true
     },
@@ -512,7 +512,7 @@
     },
     "get-stream": {
       "version": "3.0.0",
-      "resolved": "http://registry.npmjs.org/get-stream/-/get-stream-3.0.0.tgz",
+      "resolved": "https://registry.npmjs.org/get-stream/-/get-stream-3.0.0.tgz",
       "integrity": "sha1-jpQ9E1jcN1VQVOy+LtsFqhdO3hQ=",
       "dev": true
     },
@@ -753,7 +753,7 @@
     },
     "mkdirp": {
       "version": "0.5.1",
-      "resolved": "http://registry.npmjs.org/mkdirp/-/mkdirp-0.5.1.tgz",
+      "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.1.tgz",
       "integrity": "sha1-MAV0OOrGz3+MR2fzhkjWaX11yQM=",
       "requires": {
         "minimist": "0.0.8"
@@ -761,7 +761,7 @@
       "dependencies": {
         "minimist": {
           "version": "0.0.8",
-          "resolved": "http://registry.npmjs.org/minimist/-/minimist-0.0.8.tgz",
+          "resolved": "https://registry.npmjs.org/minimist/-/minimist-0.0.8.tgz",
           "integrity": "sha1-hX/Kv8M5fSYluCKCYuhqp6ARsF0="
         }
       }
@@ -941,7 +941,7 @@
     },
     "os-homedir": {
       "version": "1.0.2",
-      "resolved": "http://registry.npmjs.org/os-homedir/-/os-homedir-1.0.2.tgz",
+      "resolved": "https://registry.npmjs.org/os-homedir/-/os-homedir-1.0.2.tgz",
       "integrity": "sha1-/7xJiDNuDoM94MFox+8VISGqf7M=",
       "optional": true
     },
@@ -958,7 +958,7 @@
     },
     "os-tmpdir": {
       "version": "1.0.2",
-      "resolved": "http://registry.npmjs.org/os-tmpdir/-/os-tmpdir-1.0.2.tgz",
+      "resolved": "https://registry.npmjs.org/os-tmpdir/-/os-tmpdir-1.0.2.tgz",
       "integrity": "sha1-u+Z0BseaqFxc/sdm/lc0VV36EnQ=",
       "dev": true
     },
@@ -1000,7 +1000,7 @@
     },
     "path-is-absolute": {
       "version": "1.0.1",
-      "resolved": "http://registry.npmjs.org/path-is-absolute/-/path-is-absolute-1.0.1.tgz",
+      "resolved": "https://registry.npmjs.org/path-is-absolute/-/path-is-absolute-1.0.1.tgz",
       "integrity": "sha1-F0uSaHNVNP+8es5r9TpanhtcX18=",
       "dev": true
     },
@@ -1018,7 +1018,7 @@
     },
     "pause-stream": {
       "version": "0.0.11",
-      "resolved": "http://registry.npmjs.org/pause-stream/-/pause-stream-0.0.11.tgz",
+      "resolved": "https://registry.npmjs.org/pause-stream/-/pause-stream-0.0.11.tgz",
       "integrity": "sha1-/lo0sMvOErWqaitAPuLnO2AvFEU=",
       "dev": true,
       "requires": {
@@ -1139,7 +1139,7 @@
     },
     "rimraf": {
       "version": "2.2.8",
-      "resolved": "http://registry.npmjs.org/rimraf/-/rimraf-2.2.8.tgz",
+      "resolved": "https://registry.npmjs.org/rimraf/-/rimraf-2.2.8.tgz",
       "integrity": "sha1-5Dm+Kq7jJzIZUnMPmaiSnk/FBYI=",
       "dev": true
     },
@@ -1305,7 +1305,7 @@
     },
     "string_decoder": {
       "version": "1.1.1",
-      "resolved": "http://registry.npmjs.org/string_decoder/-/string_decoder-1.1.1.tgz",
+      "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.1.1.tgz",
       "integrity": "sha512-n/ShnvDi6FHbbVfviro+WojiFzv+s8MPMHBczVePfUpDJLwoLT0ht1l4YwBCbi8pJAveEEdnkHyPyTP/mzRfwg==",
       "requires": {
         "safe-buffer": "~5.1.0"
@@ -1321,7 +1321,7 @@
     },
     "strip-eof": {
       "version": "1.0.0",
-      "resolved": "http://registry.npmjs.org/strip-eof/-/strip-eof-1.0.0.tgz",
+      "resolved": "https://registry.npmjs.org/strip-eof/-/strip-eof-1.0.0.tgz",
       "integrity": "sha1-u0P/VZim6wXYm1n80SnJgzE2Br8=",
       "dev": true
     },
@@ -1391,7 +1391,7 @@
     },
     "through": {
       "version": "2.3.8",
-      "resolved": "http://registry.npmjs.org/through/-/through-2.3.8.tgz",
+      "resolved": "https://registry.npmjs.org/through/-/through-2.3.8.tgz",
       "integrity": "sha1-DdTJ/6q8NXlgsbckEV1+Doai4fU=",
       "dev": true
     },
@@ -1486,7 +1486,7 @@
     },
     "wrap-ansi": {
       "version": "2.1.0",
-      "resolved": "http://registry.npmjs.org/wrap-ansi/-/wrap-ansi-2.1.0.tgz",
+      "resolved": "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-2.1.0.tgz",
       "integrity": "sha1-2Pw9KE3QV5T+hJc8rs3Rz4JP3YU=",
       "dev": true,
       "requires": {