feat: promisify webContents.savePage() (#16742)

* feat: promisify webContents.savePage()

* no need to make compatible w callbacks, we're breaking it

* fix promise resolve type

* address feedback from review

* fix promise return text

* update smoke test

atom/browser/api/atom_api_web_contents.cc

@@ -1300,11 +1300,17 @@ std::string WebContents::GetUserAgent() {
   return web_contents()->GetUserAgentOverride();
 }
 
-bool WebContents::SavePage(const base::FilePath& full_file_path,
+v8::Local<v8::Promise> WebContents::SavePage(
-                           const content::SavePageType& save_type,
+    const base::FilePath& full_file_path,
-                           const SavePageHandler::SavePageCallback& callback) {
+    const content::SavePageType& save_type) {
-  auto* handler = new SavePageHandler(web_contents(), callback);
+  scoped_refptr<util::Promise> promise = new util::Promise(isolate());
-  return handler->Handle(full_file_path, save_type);
+  auto* handler = new SavePageHandler(web_contents(), promise);
+
+  const bool saveStarted = handler->Handle(full_file_path, save_type);
+  if (!saveStarted)
+    promise->RejectWithErrorMessage("Failed to save the page");
+
+  return promise->GetHandle();
 }
 
 void WebContents::OpenDevTools(mate::Arguments* args) {

atom/browser/api/atom_api_web_contents.h

@@ -149,9 +149,8 @@ class WebContents : public mate::TrackableObject<WebContents>,
   void SetUserAgent(const std::string& user_agent, mate::Arguments* args);
   std::string GetUserAgent();
   void InsertCSS(const std::string& css);
-  bool SavePage(const base::FilePath& full_file_path,
+  v8::Local<v8::Promise> SavePage(const base::FilePath& full_file_path,
-                const content::SavePageType& save_type,
+                                  const content::SavePageType& save_type);
-                const SavePageHandler::SavePageCallback& callback);
   void OpenDevTools(mate::Arguments* args);
   void CloseDevTools();
   bool IsDevToolsOpened();

atom/browser/api/save_page_handler.cc

@@ -16,8 +16,8 @@ namespace atom {
 namespace api {
 
 SavePageHandler::SavePageHandler(content::WebContents* web_contents,
-                                 const SavePageCallback& callback)
+                                 scoped_refptr<atom::util::Promise> promise)
-    : web_contents_(web_contents), callback_(callback) {}
+    : web_contents_(web_contents), promise_(promise) {}
 
 SavePageHandler::~SavePageHandler() {}
 
@@ -50,16 +50,10 @@ bool SavePageHandler::Handle(const base::FilePath& full_path,
 
 void SavePageHandler::OnDownloadUpdated(download::DownloadItem* item) {
   if (item->IsDone()) {
-    v8::Isolate* isolate = v8::Isolate::GetCurrent();
+    if (item->GetState() == download::DownloadItem::COMPLETE)
-    v8::Locker locker(isolate);
+      promise_->Resolve();
-    v8::HandleScope handle_scope(isolate);
+    else
-    if (item->GetState() == download::DownloadItem::COMPLETE) {
+      promise_->RejectWithErrorMessage("Failed to save the page.");
-      callback_.Run(v8::Null(isolate));
-    } else {
-      v8::Local<v8::String> error_message =
-          v8::String::NewFromUtf8(isolate, "Fail to save page");
-      callback_.Run(v8::Exception::Error(error_message));
-    }
     Destroy(item);
   }
 }

atom/browser/api/save_page_handler.h

@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "atom/common/promise_util.h"
 #include "components/download/public/common/download_item.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/save_page_type.h"
@@ -28,10 +29,8 @@ namespace api {
 class SavePageHandler : public content::DownloadManager::Observer,
                         public download::DownloadItem::Observer {
  public:
-  using SavePageCallback = base::Callback<void(v8::Local<v8::Value>)>;
-
   SavePageHandler(content::WebContents* web_contents,
-                  const SavePageCallback& callback);
+                  scoped_refptr<atom::util::Promise> promise);
   ~SavePageHandler() override;
 
   bool Handle(const base::FilePath& full_path,
@@ -48,7 +47,7 @@ class SavePageHandler : public content::DownloadManager::Observer,
   void OnDownloadUpdated(download::DownloadItem* item) override;
 
   content::WebContents* web_contents_;  // weak
-  SavePageCallback callback_;
+  scoped_refptr<atom::util::Promise> promise_;
 };
 
 }  // namespace api

docs/api/promisification.md

@@ -26,7 +26,6 @@ When a majority of affected functions are migrated, this flag will be enabled by
 - [ses.clearAuthCache(options[, callback])](https://github.com/electron/electron/blob/master/docs/api/session.md#clearAuthCache)
 - [contents.executeJavaScript(code[, userGesture, callback])](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#executeJavaScript)
 - [contents.print([options], [callback])](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#print)
-- [contents.savePage(fullPath, saveType, callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#savePage)
 - [webFrame.executeJavaScript(code[, userGesture, callback])](https://github.com/electron/electron/blob/master/docs/api/web-frame.md#executeJavaScript)
 - [webFrame.executeJavaScriptInIsolatedWorld(worldId, scripts[, userGesture, callback])](https://github.com/electron/electron/blob/master/docs/api/web-frame.md#executeJavaScriptInIsolatedWorld)
 - [webviewTag.executeJavaScript(code[, userGesture, callback])](https://github.com/electron/electron/blob/master/docs/api/webview-tag.md#executeJavaScript)
@@ -36,6 +35,7 @@ When a majority of affected functions are migrated, this flag will be enabled by
 - [app.getFileIcon(path[, options], callback)](https://github.com/electron/electron/blob/master/docs/api/app.md#getFileIcon)
 - [contents.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#capturePage)
 - [contents.printToPDF(options, callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#printToPDF)
+- [contents.savePage(fullPath, saveType, callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#savePage)
 - [contentTracing.getCategories(callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#getCategories)
 - [contentTracing.startRecording(options, callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#startRecording)
 - [contentTracing.stopRecording(resultFilePath, callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#stopRecording)

docs/api/web-contents.md

@@ -1555,17 +1555,15 @@ Sets the `item` as dragging item for current drag-drop operation, `file` is the
 absolute path of the file to be dragged, and `icon` is the image showing under
 the cursor when dragging.
 
-#### `contents.savePage(fullPath, saveType, callback)`
+#### `contents.savePage(fullPath, saveType)`
 
 * `fullPath` String - The full file path.
 * `saveType` String - Specify the save type.
   * `HTMLOnly` - Save only the HTML of the page.
   * `HTMLComplete` - Save complete-html page.
   * `MHTML` - Save complete-html page as MHTML.
-* `callback` Function - `(error) => {}`.
-  * `error` Error
 
-Returns `Boolean` - true if the process of saving page has been initiated successfully.
+Returns `Promise<void>` - resolves if the page is saved.
 
 ```javascript
 const { BrowserWindow } = require('electron')
@@ -1573,9 +1571,11 @@ let win = new BrowserWindow()
 
 win.loadURL('https://github.com')
 
-win.webContents.on('did-finish-load', () => {
+win.webContents.on('did-finish-load', async () => {
-  win.webContents.savePage('/tmp/test.html', 'HTMLComplete', (error) => {
+  win.webContents.savePage('/tmp/test.html', 'HTMLComplete').then(() => {
-    if (!error) console.log('Save page successfully')
+    console.log('Page was saved successfully.')
+  }).catch(err => {
+    console.log(err)
   })
 })
 ```

lib/browser/api/web-contents.js

@@ -344,6 +344,7 @@ WebContents.prototype.loadFile = function (filePath, options = {}) {
 
 WebContents.prototype.capturePage = deprecate.promisify(WebContents.prototype.capturePage)
 WebContents.prototype.printToPDF = deprecate.promisify(WebContents.prototype.printToPDF)
+WebContents.prototype.savePage = deprecate.promisify(WebContents.prototype.savePage)
 
 const addReplyToEvent = (event) => {
   event.reply = (...args) => {

spec/api-browser-window-spec.js

@@ -2575,12 +2575,8 @@ describe('BrowserWindow module', () => {
 
     it('should save page to disk', async () => {
       await w.loadFile(path.join(fixtures, 'pages', 'save_page', 'index.html'))
-      const error = await new Promise(resolve => {
+      await w.webContents.savePage(savePageHtmlPath, 'HTMLComplete')
-        w.webContents.savePage(savePageHtmlPath, 'HTMLComplete', function (error) {
+
-          resolve(error)
-        })
-      })
-      expect(error).to.be.null()
       assert(fs.existsSync(savePageHtmlPath))
       assert(fs.existsSync(savePageJsPath))
       assert(fs.existsSync(savePageCssPath))

spec/ts-smoke/electron/main.ts

@@ -113,8 +113,8 @@ app.on('ready', () => {
   mainWindow.loadURL('https://github.com')
 
   mainWindow.webContents.on('did-finish-load', function () {
-    mainWindow.webContents.savePage('/tmp/test.html', 'HTMLComplete', function (error) {
+    mainWindow.webContents.savePage('/tmp/test.html', 'HTMLComplete').then(() => {
-      if (!error) { console.log('Save page successfully') }
+      console.log('Page saved successfully')
     })
   })