refactor: make shell.OpenExternal async (#17135)

atom/browser/atom_browser_client.cc

@@ -794,14 +794,10 @@ void AtomBrowserClient::RenderProcessExited(
 }
 
 void OnOpenExternal(const GURL& escaped_url, bool allowed) {
-  if (allowed)
+  if (allowed) {
     platform_util::OpenExternal(
-#if defined(OS_WIN)
-        base::UTF8ToUTF16(escaped_url.spec()),
-#else
-        escaped_url,
-#endif
-        platform_util::OpenExternalOptions());
+        escaped_url, platform_util::OpenExternalOptions(), base::DoNothing());
+  }
 }
 
 void HandleExternalProtocolInUI(

atom/common/api/atom_api_shell.cc

@@ -52,33 +52,9 @@ void OnOpenExternalFinished(atom::util::Promise promise,
     promise.RejectWithErrorMessage(error.c_str());
 }
 
-bool OpenExternalSync(
-#if defined(OS_WIN)
-    const base::string16& url,
-#else
-    const GURL& url,
-#endif
-    mate::Arguments* args) {
-  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::Local<v8::Promise> OpenExternal(
-#if defined(OS_WIN)
-    const base::string16& url,
-#else
-    const GURL& url,
-#endif
-    mate::Arguments* args) {
+v8::Local<v8::Promise> OpenExternal(const GURL& url, mate::Arguments* args) {
   atom::util::Promise promise(args->isolate());
+  v8::Local<v8::Promise> handle = promise.GetHandle();
 
   platform_util::OpenExternalOptions options;
   if (args->Length() >= 2) {
@@ -89,7 +65,6 @@ v8::Local<v8::Promise> OpenExternal(
     }
   }
 
-  v8::Local<v8::Promise> handle = promise.GetHandle();
   platform_util::OpenExternal(
       url, options,
       base::BindOnce(&OnOpenExternalFinished, std::move(promise)));
@@ -158,7 +133,6 @@ 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);

atom/common/platform_util.h

@@ -36,23 +36,9 @@ struct OpenExternalOptions {
 
 // Open the given external protocol URL in the desktop's default manner.
 // (For example, mailto: URLs in the default mail user agent.)
-bool OpenExternal(
-#if defined(OS_WIN)
-    const base::string16& url,
-#else
-    const GURL& url,
-#endif
-    const OpenExternalOptions& options);
-
-// The asynchronous version of OpenExternal.
-void OpenExternal(
-#if defined(OS_WIN)
-    const base::string16& url,
-#else
-    const GURL& url,
-#endif
-    const OpenExternalOptions& options,
-    OpenExternalCallback callback);
+void OpenExternal(const GURL& url,
+                  const OpenExternalOptions& options,
+                  OpenExternalCallback callback);
 
 // Move a file to trash.
 bool MoveItemToTrash(const base::FilePath& full_path);

atom/common/platform_util_linux.cc

@@ -81,20 +81,16 @@ bool OpenItem(const base::FilePath& full_path) {
   return XDGOpen(full_path.value(), false);
 }
 
-bool OpenExternal(const GURL& url, const OpenExternalOptions& options) {
-  // Don't wait for exit, since we don't want to wait for the browser/email
-  // client window to close before returning
-  if (url.SchemeIs("mailto"))
-    return XDGEmail(url.spec(), false);
-  else
-    return XDGOpen(url.spec(), false);
-}
-
 void OpenExternal(const GURL& url,
                   const OpenExternalOptions& options,
                   OpenExternalCallback callback) {
-  // TODO(gabriel): Implement async open if callback is specified
-  std::move(callback).Run(OpenExternal(url, options) ? "" : "Failed to open");
+  // Don't wait for exit, since we don't want to wait for the browser/email
+  // client window to close before returning
+  if (url.SchemeIs("mailto"))
+    std::move(callback).Run(XDGEmail(url.spec(), false) ? ""
+                                                        : "Failed to open");
+  else
+    std::move(callback).Run(XDGOpen(url.spec(), false) ? "" : "Failed to open");
 }
 
 bool MoveItemToTrash(const base::FilePath& full_path) {

atom/common/platform_util_mac.mm

@@ -94,17 +94,10 @@ bool OpenItem(const base::FilePath& full_path) {
                                launchIdentifiers:NULL];
 }
 
-bool OpenExternal(const GURL& url, const OpenExternalOptions& options) {
-  DCHECK([NSThread isMainThread]);
-  NSURL* ns_url = net::NSURLWithGURL(url);
-  if (ns_url)
-    return OpenURL(ns_url, options.activate).empty();
-  return false;
-}
-
 void OpenExternal(const GURL& url,
                   const OpenExternalOptions& options,
                   OpenExternalCallback callback) {
+  DCHECK([NSThread isMainThread]);
   NSURL* ns_url = net::NSURLWithGURL(url);
   if (!ns_url) {
     std::move(callback).Run("Invalid URL");

atom/common/platform_util_win.cc

@@ -230,6 +230,29 @@ HRESULT DeleteFileProgressSink::ResumeTimer() {
   return S_OK;
 }
 
+std::string OpenExternalOnWorkerThread(
+    const GURL& url,
+    const platform_util::OpenExternalOptions& options) {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::MAY_BLOCK);
+  // Quote the input scheme to be sure that the command does not have
+  // parameters unexpected by the external program. This url should already
+  // have been escaped.
+  std::string escaped_url = url.spec();
+  escaped_url.insert(0, "\"");
+  escaped_url += "\"";
+
+  std::string working_dir = options.working_dir.AsUTF8Unsafe();
+
+  if (reinterpret_cast<ULONG_PTR>(
+          ShellExecuteA(nullptr, "open", escaped_url.c_str(), nullptr,
+                        working_dir.empty() ? nullptr : working_dir.c_str(),
+                        SW_SHOWNORMAL)) <= 32) {
+    return "Failed to open";
+  }
+  return "";
+}
+
 void ShowItemInFolderOnWorkerThread(const base::FilePath& full_path) {
   base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
                                                 base::BlockingType::MAY_BLOCK);
@@ -301,31 +324,15 @@ bool OpenItem(const base::FilePath& full_path) {
     return ui::win::OpenFileViaShell(full_path);
 }
 
-bool OpenExternal(const base::string16& url,
-                  const OpenExternalOptions& options) {
-  // Quote the input scheme to be sure that the command does not have
-  // parameters unexpected by the external program. This url should already
-  // have been escaped.
-  base::string16 escaped_url = L"\"" + url + L"\"";
-  auto working_dir = options.working_dir.value();
-
-  if (reinterpret_cast<ULONG_PTR>(
-          ShellExecuteW(nullptr, L"open", escaped_url.c_str(), nullptr,
-                        working_dir.empty() ? nullptr : working_dir.c_str(),
-                        SW_SHOWNORMAL)) <= 32) {
-    // We fail to execute the call. We could display a message to the user.
-    // TODO(nsylvain): we should also add a dialog to warn on errors. See
-    // bug 1136923.
-    return false;
-  }
-  return true;
-}
-
-void OpenExternal(const base::string16& url,
+void OpenExternal(const GURL& url,
                   const OpenExternalOptions& options,
                   OpenExternalCallback callback) {
-  // TODO(gabriel): Implement async open if callback is specified
-  std::move(callback).Run(OpenExternal(url, options) ? "" : "Failed to open");
+  base::PostTaskAndReplyWithResult(
+      base::CreateCOMSTATaskRunnerWithTraits(
+          {base::MayBlock(), base::TaskPriority::USER_BLOCKING})
+          .get(),
+      FROM_HERE, base::BindOnce(&OpenExternalOnWorkerThread, url, options),
+      std::move(callback));
 }
 
 bool MoveItemToTrash(const base::FilePath& path) {

docs/api/menu.md

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

docs/api/shell.md

@@ -32,18 +32,6 @@ Returns `Boolean` - Whether the item was successfully opened.
 
 Open the given file in the desktop's default manner.
 
-### `shell.openExternalSync(url[, options])`
-
-* `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_
-
-Returns `Boolean` - Whether an application was available to open the URL.
-
-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.

docs/api/webview-tag.md

@@ -774,10 +774,10 @@ The following example code opens the new url in system's default browser.
 const { shell } = require('electron')
 const webview = document.querySelector('webview')
 
-webview.addEventListener('new-window', (e) => {
+webview.addEventListener('new-window', async (e) => {
   const protocol = require('url').parse(e.url).protocol
   if (protocol === 'http:' || protocol === 'https:') {
-    shell.openExternalSync(e.url)
+    await shell.openExternal(e.url)
   }
 })
 ```

docs/tutorial/electron-timelines.md

@@ -47,4 +47,4 @@ Take a look at 5.0.0 Timeline [blog post](https://electronjs.org/blog/electron-5
 | Tue, 2019-Jul-30 | 6.0.0 | ✨ stable ✨ |
 
 ## 7.0.0 Release Schedule
-TBD
+TBD

docs/tutorial/security.md

@@ -681,12 +681,12 @@ windows, limiting it to only what you need.
 const { shell } = require('electron')
 
 app.on('web-contents-created', (event, contents) => {
-  contents.on('new-window', (event, navigationUrl) => {
+  contents.on('new-window', async (event, navigationUrl) => {
     // In this example, we'll ask the operating system
     // to open this event's url in the default browser.
     event.preventDefault()
 
-    shell.openExternalSync(navigationUrl)
+    await shell.openExternal(navigationUrl)
   })
 })
 ```

lib/browser/default-menu.ts

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

spec/api-remote-spec.js

@@ -198,7 +198,6 @@ 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.openExternalSync, 'function')
     })
 
     it('returns toString() of original function via toString()', () => {

spec/api-shell-spec.js

@@ -36,7 +36,7 @@ describe('shell module', () => {
       }
     })
 
-    it('opens an external link asynchronously', done => {
+    it('opens an external link', done => {
       const url = 'http://www.example.com'
       if (process.platform === 'linux') {
         process.env.BROWSER = '/bin/true'
@@ -46,18 +46,6 @@ describe('shell module', () => {
 
       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)', () => {

spec/ts-smoke/electron/main.ts

@@ -997,7 +997,7 @@ shell.moveItemToTrash('/home/user/Desktop/test.txt')
 
 shell.openExternal('https://github.com', {
   activate: false
-})
+}).then(() => {})
 
 shell.beep()
 

spec/ts-smoke/electron/renderer.ts

@@ -218,7 +218,7 @@ app.on('ready', () => {
 // shell
 // https://github.com/atom/electron/blob/master/docs/api/shell.md
 
-shell.openExternal('https://github.com')
+shell.openExternal('https://github.com').then(() => {})
 
 // <webview>
 // https://github.com/atom/electron/blob/master/docs/api/web-view-tag.md
@@ -238,8 +238,9 @@ webview.addEventListener('found-in-page', function (e) {
 
 const requestId = webview.findInPage('test')
 
-webview.addEventListener('new-window', function (e) {
-  require('electron').shell.openExternal(e.url)
+webview.addEventListener('new-window', async e => {
+  const { shell } = require('electron')
+  await shell.openExternal(e.url)
 })
 
 webview.addEventListener('close', function () {