Bug 204151 - Automation: evaluateJavaScriptFunction should use Promises
Summary: Automation: evaluateJavaScriptFunction should use Promises
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 204880
Blocks: 184966
  Show dependency treegraph
 
Reported: 2019-11-13 04:06 PST by Carlos Garcia Campos
Modified: 2020-01-23 13:44 PST (History)
3 users (show)

See Also:


Attachments
Patch (18.39 KB, patch)
2019-12-05 03:18 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased partch (18.47 KB, patch)
2019-12-23 03:30 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (18.83 KB, patch)
2020-01-08 02:59 PST, Carlos Garcia Campos
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-11-13 04:06:28 PST
As documented in the spec. This is causing several test failures:

imported/w3c/webdriver/tests/execute_async_script/promise.py::test_promise_resolve
imported/w3c/webdriver/tests/execute_async_script/promise.py::test_promise_resolve_delayed
imported/w3c/webdriver/tests/execute_async_script/promise.py::test_promise_all_resolve
imported/w3c/webdriver/tests/execute_async_script/promise.py::test_await_promise_resolve
imported/w3c/webdriver/tests/execute_async_script/promise.py::test_promise_resolve_timeout
imported/w3c/webdriver/tests/execute_async_script/promise.py::test_promise_reject
imported/w3c/webdriver/tests/execute_async_script/promise.py::test_promise_reject_delayed
imported/w3c/webdriver/tests/execute_async_script/promise.py::test_promise_all_reject
imported/w3c/webdriver/tests/execute_async_script/promise.py::test_promise_reject_timeout
Comment 1 Carlos Garcia Campos 2019-12-05 03:18:49 PST
Created attachment 384893 [details]
Patch
Comment 2 Carlos Garcia Campos 2019-12-23 03:30:04 PST
Created attachment 386336 [details]
Rebased partch

It won't apply yet though, but it's rebased after r253883
Comment 3 Brian Burg 2020-01-07 16:06:51 PST
Does this apply now?
Comment 4 Carlos Garcia Campos 2020-01-08 00:23:19 PST
(In reply to Brian Burg from comment #3)
> Does this apply now?

I don't think so, it depends on bug #204880. Even if this applied, it would make imported/w3c/webdriver/tests/back/user_prompts.py and imported/w3c/webdriver/tests/forward/user_prompts.py regress without the patch for bug #204880, so better fix that one first.
Comment 5 Carlos Garcia Campos 2020-01-08 02:59:43 PST
Created attachment 387089 [details]
Rebased patch
Comment 6 Brian Burg 2020-01-09 12:05:52 PST
Comment on attachment 387089 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387089&action=review

r=me, nice work!

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:198
> +    } else if (JSValueIsObject(context, arguments[2])) {

I like this change.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:102
> +        // finishes resolved, so we need to start a new one here to wait for the second promise to be reolved or the timeout.

Nit: resolved

So in other words, `resultPromise` could resolve to a Promise that is not settled (because we passed the `resolve` for the inner racing promise), so it has to be waited. It's a little tragic to have to do this, but I guess I can't think of an alternative if we can't assume anything about the callback's argument type.
Comment 7 Carlos Garcia Campos 2020-01-10 00:44:44 PST
Committed r254329: <https://trac.webkit.org/changeset/254329>
Comment 8 Radar WebKit Bug Importer 2020-01-10 00:45:19 PST
<rdar://problem/58472883>