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: promisify executeJavaScript #17312

Merged
merged 1 commit into from Mar 14, 2019

Conversation

@miniak
Copy link
Contributor

commented Mar 9, 2019

Description of Change

  • contents.executeJavaScript()
  • webFrame.executeJavaScript()
  • webFrame.executeJavaScriptInIsolatedWorld()
  • webviewTag.executeJavaScript()

Checklist

Release Notes

Notes: Converted contents.executeJavaScript(), webFrame.executeJavaScript(), webFrame.executeJavaScriptInIsolatedWorld() and webviewTag.executeJavaScript() to return Promises instead of taking callbacks.

@miniak miniak added the wip label Mar 9, 2019

@miniak miniak self-assigned this Mar 9, 2019

@miniak miniak force-pushed the miniak/promisify-execute-js branch 3 times, most recently from 5397848 to 17b9734 Mar 9, 2019

@miniak miniak changed the base branch from master to miniak/ipc-helpers Mar 9, 2019

@electron-cation electron-cation bot removed the new-pr 🌱 label Mar 10, 2019

@miniak miniak force-pushed the miniak/ipc-helpers branch from dd1afbd to 78fb556 Mar 12, 2019

@miniak miniak force-pushed the miniak/promisify-execute-js branch 2 times, most recently from e57ccd8 to 00c3c87 Mar 12, 2019

@miniak miniak force-pushed the miniak/promisify-execute-js branch from 00c3c87 to 8514d97 Mar 13, 2019

@miniak miniak requested a review from electron/wg-upgrades as a code owner Mar 13, 2019

@miniak miniak changed the base branch from miniak/ipc-helpers to master Mar 13, 2019

@miniak miniak changed the title [wip] feat: promisify executeJavaScript feat: promisify executeJavaScript Mar 13, 2019

@miniak miniak force-pushed the miniak/promisify-execute-js branch from 8514d97 to 5ee49a0 Mar 13, 2019

@miniak miniak removed the wip label Mar 13, 2019

@miniak miniak force-pushed the miniak/promisify-execute-js branch from 5ee49a0 to be95cef Mar 13, 2019

@deepak1556
Copy link
Member

left a comment

LGTM, with some style changes. Thanks!

Show resolved Hide resolved atom/renderer/api/atom_api_web_frame.cc Outdated
Show resolved Hide resolved atom/renderer/api/atom_api_web_frame.cc Outdated

@miniak miniak force-pushed the miniak/promisify-execute-js branch from be95cef to 93891e8 Mar 13, 2019

@alexeykuzmin
Copy link
Contributor

left a comment

👍

@codebytere

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

@miniak just resolve the markdown file conflict and we can get this across the finish line 🏁

@miniak miniak force-pushed the miniak/promisify-execute-js branch from 93891e8 to cb5be7e Mar 14, 2019

@miniak

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@codebytere resolved, can you please merge after the builds pass again?

@codebytere codebytere merged commit 2e89348 into master Mar 14, 2019

12 of 13 checks passed

Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190314.11 succeeded
Details
electron-arm64-testing Build #20190314.12 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Mar 14, 2019

Release Notes Persisted

Converted contents.executeJavaScript(), webFrame.executeJavaScript(), webFrame.executeJavaScriptInIsolatedWorld() and webviewTag.executeJavaScript() to return Promises instead of taking callbacks.

@codebytere codebytere deleted the miniak/promisify-execute-js branch Mar 14, 2019

Kiku-Reise added a commit to Kiku-Reise/electron that referenced this pull request May 16, 2019

@bughit

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Though I initially filed a bug (#12159) about the docs discrepancy for webFrame execute APIs, I very much agree with @MarshallOfSound's assessment that this was primarily a docs issue.

The webFrame execute APIs have always been synchronous (which makes perfect sense, this is in-process js evaluation), and under the hood they still are, despite now returning promises. The callback used to deliver the result synchronously before the method returned.

There is nothing gained and only loss of flexibility when you wrap an actually sync api in a promise. It's trivial for the user to go from sync to promise (Promise.(resolve|reject)) but going from promise to sync is impossible.

So this change (for webFrame) is:

  1. not an improvement
  2. a breaking change for 6.0 (I have code that depends on the synchronous nature of these, which now can't even be fixed)

Please revert it (for webFrame). An actual improvement (for webFrame) would be to replace the sync callback with a sync return (or add).

@bughit

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

If you believe that promise versions of the webFrame execute APIs should exist out the box (though it's not clear to me why they are needed), that can be done without breaking changes or at least loss of valuable sync functionality, by adding new async wrapper methods.

@bughit

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@bughit

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

@codebytere

Just to be clear, I am just trying to have a conversation about the impact of the webFrame.execute change, to reach a consensus on how to handle it better, without breaking code or at least losing current capabilities. 6.0 goes into quiet period on the 23rd.

To summarize, the webFrame.execute calls are synchronous. Wrapping them in promises is not an enhancement, it takes away the flexibility of using them synchronously, breaking existing code (example in #19161).

@codebytere

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

cc @miniak ☝️

@bughit

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

@miniak Your comment does not appear here, but I'll respond because I'd like to for this conversation to proceed. My suggestion from #19161:

... is to keep the callback in the webFrame.execute for compat, but deprecate it, which I believe is the standard procedure for removing functionality. Additionally return the eval result, which is a compatible change (before it was undefined)

However if it's simpler and quicker to drop the callback, that would be fine too.

@bughit

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

@codebytere @miniak

class SyncScriptExecutionCallback : public blink::WebScriptExecutionCallback {
 public:
  const char* error_message;
  v8::Local<v8::Value> result;

  explicit SyncScriptExecutionCallback() : error_message(nullptr) {}

  void Completed(
      const blink::WebVector<v8::Local<v8::Value>>& result) override {
    if (!result.empty()) {
      if (!result[0].IsEmpty()) {
        // Right now only single results per frame is supported.
        this->result = result[0];
      } else {
        this->error_message =
            "Script failed to execute, this normally means an error "
            "was thrown. Check the renderer console for the error.";
      }
    } else {
      this->error_message =
          "WebFrame was removed before script could run. This normally means "
          "the underlying frame was destroyed";
    }
  }
};

v8::Local<v8::Value> ExecuteJavaScript(mate::Arguments* args,
                                         v8::Local<v8::Value> window,
                                         const base::string16& code) {
  bool has_user_gesture = false;
  args->GetNext(&has_user_gesture);

  SyncScriptExecutionCallback sync_callback;

  GetRenderFrame(window)->GetWebFrame()->RequestExecuteScriptAndReturnValue(
      blink::WebScriptSource(blink::WebString::FromUTF16(code)),
      has_user_gesture, &sync_callback);

  if (sync_callback.error_message) {
    args->ThrowError(sync_callback.error_message);
    return v8::Null(args->isolate());
  }
  
  return sync_callback.result;
}

Is this acceptable?

@codebytere

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

@bughit while it may have sometimes worked synchronously, this has always been quite clearly documented as an async callback. A given function performing synchronously doesn’t mean the API contract is synchronous, and as such it is, in our opinion, not a breaking change in that manner. As such, we plan to keep this as it is.

@bughit

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

@codebytere

Not sometimes, in practice, which matters much more than incorrect documentation, it has always been synchronous, so it does break code, but more significantly, it can't be worked around.

A value can easily be turned to a promise but a fulfilled promise can not synchronously be turned into a value. So the cost of this change is very high, but the benefit is 0.

When I initially started using this api, I saw the documentation was incorrect and I reported it (#12159), so then as a fallback, experimentally and by examining the implementation I confirmed that the call is synchronous. But now you are saying that in the absence of valid docs, it was wrong of me to use the api as it was actually implemented (synchronously) and so I only have myself to blame that my code is suddenly broken in 6.0?

If you don't want to revert existing methods, can you at least accept sync varieties (webFrame.executeJavaScriptSync)? There is value in those and no harm.

@bughit

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

it may have sometimes worked synchronously

ExecuteJavaScript calls RequestExecuteScriptAndReturnValue

which creates a PausableScriptExecutor and calls run on it executor->Run();

executor->Run() is also what RequestExecuteScriptInIsolatedWorld does when passed kSynchronous. So executor->Run() == kSynchronous

  switch (option) {
    case kAsynchronousBlockingOnload:
      executor->RunAsync(PausableScriptExecutor::kOnloadBlocking);
      break;
    case kAsynchronous:
      executor->RunAsync(PausableScriptExecutor::kNonBlocking);
      break;
    case kSynchronous:
      executor->Run();
      break;
  }

kSynchronous is defined as "Execute script synchronously, unless the page is suspended."

  enum ScriptExecutionType {
    // Execute script synchronously, unless the page is suspended.
    kSynchronous,
    // Execute script asynchronously.
    kAsynchronous,
    // Execute script asynchronously, blocking the window.onload event.
    kAsynchronousBlockingOnload
  };

I am not a 100% sure but I think suspended means unloaded to save memory, which does not apply to electron, which means WebFrame.executeJavaScript is always synchronous which is borne out by my experience with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.