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.

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

feat: split openExternal into sync and async #16176

Merged
merged 3 commits into from Jan 15, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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();

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jan 15, 2019

Member

Should this be done in the promise helper to reduce this boilerplate being duplicated for all promises?

This comment has been minimized.

Copy link
@zcbenz

zcbenz Jan 15, 2019

Member

I agree moving the code to promise helper. But I would prefer doing it in a separate PR since it is a refactoring that modifies multiple files.

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);
@@ -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')
}
}
]
@@ -21,7 +21,7 @@ function initialize () {

const openLinkExternally = (e) => {
e.preventDefault()
shell.openExternal(url)
shell.openExternalSync(url)
}

link.addEventListener('click', openLinkExternally)
@@ -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') }
}
]
}
@@ -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)
@@ -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)`

@@ -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)
}
})
```
@@ -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)
})
})
```
@@ -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()', () => {
@@ -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(() => {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.