From 43e6d7fe88367af71e73cd1996f41fdd665fa3e6 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 19 Aug 2019 09:10:18 -0700 Subject: [PATCH] chore: add error throwing utility (#19803) * chore: add error throwing utility * feedback from review * DRY out repeated isolate calls --- filenames.gni | 2 + native_mate/native_mate/function_template.h | 11 +++++ .../browser/api/atom_api_system_preferences.h | 4 +- .../api/atom_api_system_preferences_mac.mm | 6 +-- shell/common/error_util.cc | 45 ++++++++++++++++++ shell/common/error_util.h | 47 +++++++++++++++++++ spec-main/api-system-preferences-spec.ts | 7 +++ 7 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 shell/common/error_util.cc create mode 100644 shell/common/error_util.h diff --git a/filenames.gni b/filenames.gni index 1a3578d851ad..20ce1b16667f 100644 --- a/filenames.gni +++ b/filenames.gni @@ -490,6 +490,8 @@ filenames = { "shell/common/keyboard_util.h", "shell/common/deprecate_util.cc", "shell/common/deprecate_util.h", + "shell/common/error_util.cc", + "shell/common/error_util.h", "shell/common/mouse_util.cc", "shell/common/mouse_util.h", "shell/common/mac/main_application_bundle.h", diff --git a/native_mate/native_mate/function_template.h b/native_mate/native_mate/function_template.h index f557ee55ab0b..5000bd100513 100644 --- a/native_mate/native_mate/function_template.h +++ b/native_mate/native_mate/function_template.h @@ -5,6 +5,7 @@ #ifndef NATIVE_MATE_NATIVE_MATE_FUNCTION_TEMPLATE_H_ #define NATIVE_MATE_NATIVE_MATE_FUNCTION_TEMPLATE_H_ +#include "../shell/common/error_util.h" #include "base/callback.h" #include "base/logging.h" #include "native_mate/arguments.h" @@ -128,6 +129,16 @@ inline bool GetNextArgument(Arguments* args, return true; } +// Allow clients to pass a util::Error to throw errors if they +// don't need the full mate::Arguments +inline bool GetNextArgument(Arguments* args, + int create_flags, + bool is_first, + electron::util::ErrorThrower* result) { + *result = electron::util::ErrorThrower(args->isolate()); + return true; +} + // Classes for generating and storing an argument pack of integer indices // (based on well-known "indices trick", see: http://goo.gl/bKKojn): template diff --git a/shell/browser/api/atom_api_system_preferences.h b/shell/browser/api/atom_api_system_preferences.h index 6ceb3d88a8a8..9682f8afb10a 100644 --- a/shell/browser/api/atom_api_system_preferences.h +++ b/shell/browser/api/atom_api_system_preferences.h @@ -12,6 +12,7 @@ #include "base/values.h" #include "native_mate/handle.h" #include "shell/browser/api/event_emitter.h" +#include "shell/common/error_util.h" #include "shell/common/promise_util.h" #if defined(OS_WIN) @@ -95,7 +96,8 @@ class SystemPreferences : public mate::EventEmitter void RemoveUserDefault(const std::string& name); bool IsSwipeTrackingFromScrollEventsEnabled(); - std::string GetSystemColor(const std::string& color, mate::Arguments* args); + std::string GetSystemColor(util::ErrorThrower thrower, + const std::string& color); bool CanPromptTouchID(); v8::Local PromptTouchID(v8::Isolate* isolate, diff --git a/shell/browser/api/atom_api_system_preferences_mac.mm b/shell/browser/api/atom_api_system_preferences_mac.mm index 02d5082e8abb..4d5ac423a128 100644 --- a/shell/browser/api/atom_api_system_preferences_mac.mm +++ b/shell/browser/api/atom_api_system_preferences_mac.mm @@ -405,8 +405,8 @@ std::string SystemPreferences::GetAccentColor() { return base::SysNSStringToUTF8([sysColor RGBAValue]); } -std::string SystemPreferences::GetSystemColor(const std::string& color, - mate::Arguments* args) { +std::string SystemPreferences::GetSystemColor(util::ErrorThrower thrower, + const std::string& color) { NSColor* sysColor = nil; if (color == "blue") { sysColor = [NSColor systemBlueColor]; @@ -427,7 +427,7 @@ std::string SystemPreferences::GetSystemColor(const std::string& color, } else if (color == "yellow") { sysColor = [NSColor systemYellowColor]; } else { - args->ThrowError("Unknown system color: " + color); + thrower.ThrowError("Unknown system color: " + color); return ""; } diff --git a/shell/common/error_util.cc b/shell/common/error_util.cc new file mode 100644 index 000000000000..46327bfc2463 --- /dev/null +++ b/shell/common/error_util.cc @@ -0,0 +1,45 @@ +// Copyright (c) 2019 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#include + +#include "native_mate/converter.h" +#include "shell/common/error_util.h" + +namespace electron { + +namespace util { + +ErrorThrower::ErrorThrower(v8::Isolate* isolate) : isolate_(isolate) {} + +// This constructor should be rarely if ever used, since +// v8::Isolate::GetCurrent() uses atomic loads and is thus a bit +// costly to invoke +ErrorThrower::ErrorThrower() : isolate_(v8::Isolate::GetCurrent()) {} + +ErrorThrower::~ErrorThrower() = default; + +void ErrorThrower::ThrowError(const std::string& err_msg) { + Throw(v8::Exception::Error, err_msg); +} + +void ErrorThrower::ThrowTypeError(const std::string& err_msg) { + Throw(v8::Exception::TypeError, err_msg); +} + +void ErrorThrower::ThrowRangeError(const std::string& err_msg) { + Throw(v8::Exception::RangeError, err_msg); +} + +void ErrorThrower::ThrowReferenceError(const std::string& err_msg) { + Throw(v8::Exception::ReferenceError, err_msg); +} + +void ErrorThrower::ThrowSyntaxError(const std::string& err_msg) { + Throw(v8::Exception::SyntaxError, err_msg); +} + +} // namespace util + +} // namespace electron diff --git a/shell/common/error_util.h b/shell/common/error_util.h new file mode 100644 index 000000000000..eaf6cacdb0dd --- /dev/null +++ b/shell/common/error_util.h @@ -0,0 +1,47 @@ +// Copyright (c) 2019 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#ifndef SHELL_COMMON_ERROR_UTIL_H_ +#define SHELL_COMMON_ERROR_UTIL_H_ + +#include + +#include "native_mate/converter.h" + +namespace electron { + +namespace util { + +class ErrorThrower { + public: + explicit ErrorThrower(v8::Isolate* isolate); + ErrorThrower(); + + ~ErrorThrower(); + + void ThrowError(const std::string& err_msg); + void ThrowTypeError(const std::string& err_msg); + void ThrowRangeError(const std::string& err_msg); + void ThrowReferenceError(const std::string& err_msg); + void ThrowSyntaxError(const std::string& err_msg); + + private: + v8::Isolate* isolate() const { return isolate_; } + + using ErrorGenerator = + v8::Local (*)(v8::Local err_msg); + void Throw(ErrorGenerator gen, const std::string& err_msg) { + v8::Local exception = gen(mate::StringToV8(isolate_, err_msg)); + if (!isolate_->IsExecutionTerminating()) + isolate_->ThrowException(exception); + } + + v8::Isolate* isolate_; +}; + +} // namespace util + +} // namespace electron + +#endif // SHELL_COMMON_ERROR_UTIL_H_ diff --git a/spec-main/api-system-preferences-spec.ts b/spec-main/api-system-preferences-spec.ts index 97200a9b0aa7..8c636b55c90f 100644 --- a/spec-main/api-system-preferences-spec.ts +++ b/spec-main/api-system-preferences-spec.ts @@ -117,6 +117,13 @@ describe('systemPreferences module', () => { }) ifdescribe(process.platform === 'darwin')('systemPreferences.getSystemColor(color)', () => { + it('throws on invalid system colors', () => { + const color = 'bad-color' + expect(() => { + systemPreferences.getSystemColor(color as any) + }).to.throw(`Unknown system color: ${color}`) + }) + it('returns a valid system color', () => { const colors = ['blue', 'brown', 'gray', 'green', 'orange', 'pink', 'purple', 'red', 'yellow']