refactor: add promise helper and change whenReady to be native impl (#13115)

* Add promise helper and change whenReady to be native impl

* remove commented code

* add GetInner helper to dedupe promise code

* add Promise.reject helper to be consistent with JS

* fix linting

* update promise impl per feedback

* remove param name from unused isolate

* Use non-depreceated resolvers for promises

* Add thread dchecks for promise helper, intiialize promise pointer to nullptr

atom/browser/api/atom_api_app.cc

@@ -1219,6 +1219,7 @@ void App::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("getName", base::Bind(&Browser::GetName, browser))
       .SetMethod("setName", base::Bind(&Browser::SetName, browser))
       .SetMethod("isReady", base::Bind(&Browser::is_ready, browser))
+      .SetMethod("whenReady", base::Bind(&Browser::WhenReady, browser))
       .SetMethod("addRecentDocument",
                  base::Bind(&Browser::AddRecentDocument, browser))
       .SetMethod("clearRecentDocuments",

atom/browser/api/atom_api_app.h

@@ -15,6 +15,7 @@
 #include "atom/browser/browser.h"
 #include "atom/browser/browser_observer.h"
 #include "atom/common/native_mate_converters/callback.h"
+#include "atom/common/promise_util.h"
 #include "base/process/process_iterator.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "chrome/browser/icon_manager.h"

atom/browser/browser.cc

@@ -154,10 +154,23 @@ void Browser::DidFinishLaunching(const base::DictionaryValue& launch_info) {
     base::CreateDirectoryAndGetError(user_data, nullptr);
 
   is_ready_ = true;
+  if (ready_promise_) {
+    ready_promise_->Resolve();
+  }
   for (BrowserObserver& observer : observers_)
     observer.OnFinishLaunching(launch_info);
 }
 
+util::Promise* Browser::WhenReady(v8::Isolate* isolate) {
+  if (!ready_promise_) {
+    ready_promise_ = new util::Promise(isolate);
+    if (is_ready()) {
+      ready_promise_->Resolve();
+    }
+  }
+  return ready_promise_;
+}
+
 void Browser::OnAccessibilitySupportChanged() {
   for (BrowserObserver& observer : observers_)
     observer.OnAccessibilitySupportChanged();

atom/browser/browser.h

@@ -10,6 +10,7 @@
 
 #include "atom/browser/browser_observer.h"
 #include "atom/browser/window_list_observer.h"
+#include "atom/common/promise_util.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
@@ -245,6 +246,7 @@ class Browser : public WindowListObserver {
   bool is_shutting_down() const { return is_shutdown_; }
   bool is_quiting() const { return is_quiting_; }
   bool is_ready() const { return is_ready_; }
+  util::Promise* WhenReady(v8::Isolate* isolate);
 
  protected:
   // Returns the version of application bundle or executable file.
@@ -280,6 +282,8 @@ class Browser : public WindowListObserver {
 
   int badge_count_ = 0;
 
+  util::Promise* ready_promise_ = nullptr;
+
 #if defined(OS_MACOSX)
   base::DictionaryValue about_panel_options_;
 #endif

atom/common/promise_util.cc

@@ -0,0 +1,37 @@
+// Copyright (c) 2018 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "atom/common/promise_util.h"
+
+#include <string>
+
+namespace atom {
+
+namespace util {
+
+v8::Maybe<bool> Promise::RejectWithErrorMessage(const std::string& string) {
+  v8::Local<v8::String> error_message =
+      v8::String::NewFromUtf8(isolate(), string.c_str());
+  v8::Local<v8::Value> error = v8::Exception::Error(error_message);
+  return GetInner()->Reject(isolate()->GetCurrentContext(),
+                            mate::ConvertToV8(isolate(), error));
+}
+
+v8::Local<v8::Object> Promise::GetHandle() const {
+  return GetInner()->GetPromise();
+}
+
+}  // namespace util
+
+}  // namespace atom
+
+namespace mate {
+
+v8::Local<v8::Value> mate::Converter<atom::util::Promise*>::ToV8(
+    v8::Isolate*,
+    atom::util::Promise* val) {
+  return val->GetHandle();
+}
+
+}  // namespace mate

atom/common/promise_util.h

@@ -0,0 +1,87 @@
+// Copyright (c) 2018 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_COMMON_PROMISE_UTIL_H_
+#define ATOM_COMMON_PROMISE_UTIL_H_
+
+#include <string>
+
+#include "content/public/browser/browser_thread.h"
+#include "native_mate/converter.h"
+
+namespace atom {
+
+namespace util {
+
+class Promise {
+ public:
+  explicit Promise(v8::Isolate* isolate) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    isolate_ = isolate;
+    resolver_.Reset(isolate, v8::Promise::Resolver::New(isolate));
+  }
+  ~Promise() {}
+
+  v8::Isolate* isolate() const { return isolate_; }
+
+  virtual v8::Local<v8::Object> GetHandle() const;
+
+  v8::Maybe<bool> Resolve() {
+    return GetInner()->Resolve(isolate()->GetCurrentContext(),
+                               v8::Undefined(isolate()));
+  }
+
+  v8::Maybe<bool> Reject() {
+    return GetInner()->Reject(isolate()->GetCurrentContext(),
+                              v8::Undefined(isolate()));
+  }
+
+  template <typename T>
+  v8::Maybe<bool> Resolve(T* value) {
+    return GetInner()->Resolve(isolate()->GetCurrentContext(),
+                               mate::ConvertToV8(isolate(), value));
+  }
+
+  template <typename T>
+  v8::Maybe<bool> Reject(T* value) {
+    return GetInner()->Reject(isolate()->GetCurrentContext(),
+                              mate::ConvertToV8(isolate(), value));
+  }
+
+  v8::Maybe<bool> RejectWithErrorMessage(const std::string& error);
+
+ protected:
+  v8::Isolate* isolate_;
+
+ private:
+  v8::Local<v8::Promise::Resolver> GetInner() const {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    return resolver_.Get(isolate());
+  }
+
+  v8::Global<v8::Promise::Resolver> resolver_;
+
+  DISALLOW_COPY_AND_ASSIGN(Promise);
+};
+
+}  // namespace util
+
+}  // namespace atom
+
+namespace mate {
+
+template <>
+struct Converter<atom::util::Promise*> {
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
+                                   atom::util::Promise* val);
+  // TODO(MarshallOfSound): Implement FromV8 to allow promise chaining
+  //                        in native land
+  // static bool FromV8(v8::Isolate* isolate,
+  //                    v8::Local<v8::Value> val,
+  //                    Promise* out);
+};
+
+}  // namespace mate
+
+#endif  // ATOM_COMMON_PROMISE_UTIL_H_

filenames.gypi

@@ -522,6 +522,8 @@
       'atom/common/platform_util_linux.cc',
       'atom/common/platform_util_mac.mm',
       'atom/common/platform_util_win.cc',
+      'atom/common/promise_util.h',
+      'atom/common/promise_util.cc',
       'atom/renderer/api/atom_api_renderer_ipc.h',
       'atom/renderer/api/atom_api_renderer_ipc.cc',
       'atom/renderer/api/atom_api_spell_check_client.cc',

lib/browser/api/app.js

@@ -12,30 +12,12 @@ const {deprecate, Menu} = electron
 const {EventEmitter} = require('events')
 
 let dockMenu = null
-let readyPromise = null
 
 // App is an EventEmitter.
 Object.setPrototypeOf(App.prototype, EventEmitter.prototype)
 EventEmitter.call(app)
 
 Object.assign(app, {
-  whenReady () {
-    if (readyPromise !== null) {
-      return readyPromise
-    }
-
-    if (app.isReady()) {
-      readyPromise = Promise.resolve()
-    } else {
-      readyPromise = new Promise(resolve => {
-        // XXX(alexeykuzmin): Explicitly ignore any data
-        // passed to the event handler to avoid memory leaks.
-        app.once('ready', () => resolve())
-      })
-    }
-
-    return readyPromise
-  },
   setApplicationMenu (menu) {
     return Menu.setApplicationMenu(menu)
   },