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

Add type field to "ScriptEvaluateResult" for "success" or "exception" #340

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Dec 1, 2022

Fixes #339.

This change needs an update of the wpt tests that I can work on if the change is fine.


Preview | Diff

index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@sadym-chromium sadym-chromium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with type: "success"

@whimboo whimboo force-pushed the script_result_type branch from fbd4ead to 827fa68 Compare December 1, 2022 15:57
@whimboo
Copy link
Contributor Author

whimboo commented Dec 1, 2022

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1803599 for updating the webdriver bidi client and tests.

@whimboo whimboo changed the title Add type field to "ScriptEvaluateResult" for "result" or "exception" Add type field to "ScriptEvaluateResult" for "success" or "exception" Dec 1, 2022
@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed https://github.com/w3c/webdriver-bidi/pull/109.

The full IRC log of that discussion <jgraham> topic: https://github.com//pull/109
<jgraham> github: https://github.com//pull/109
<jgraham> ack JimEvans
<AlexRudenko1> Jim Evans: added 109 to the list. Was there any movement on this PR?
<AlexRudenko1> jgraham (IRC): the last status was that this PR was fine approx. But I wanted to land HTML changes.
<AlexRudenko1> jgraham (IRC): and the last person to look at this was foolip (IRC) but he left
<foolip> I can react on chat, possibly
<sadym> q+
<AlexRudenko1> jgraham (IRC): have to rewrite the HTML part of this PR and land it
<jgraham> ack sadym
<AlexRudenko1> sadym (IRC): WPT tests are missing for the PR
<AlexRudenko1> jgraham (IRC): would also need to add tests
<AlexRudenko1> jgraham (IRC): is it something that is blocking?
<AlexRudenko1> Jim Evans: no just trying to clean up
<whimboo> https://github.com//pull/340
<AlexRudenko1> whimboo: filed a PR to add the type field
<jgraham> github: https://github.com//pull/340
<AlexRudenko1> whimboo: if it looks okay I can make the remaining changes

@jgraham jgraham merged commit 7dd07b6 into w3c:master Dec 5, 2022
@whimboo whimboo deleted the script_result_type branch December 5, 2022 10:21
@whimboo
Copy link
Contributor Author

whimboo commented Dec 5, 2022

@sadym-chromium given that changing the behavior of the webdriver bidi client for wdspec tests to correctly check for that type field would break all of the evaluation tests for Chrome do you think that you could add that field relatively soon, and we are going to land the test changes when that change hit the beta channel? Or what would be your suggestion?

@sadym-chromium
Copy link
Contributor

That's a good idea to land the implementation before the WPT. Let me do that.

@whimboo
Copy link
Contributor Author

whimboo commented Dec 7, 2022

@sadym-chromium I've landed the support for the type field in Firefox 109. Please let us know which Chrome release will have support for that as well. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add type field to "ScriptEvaluateResult" for "success" or "exception"
4 participants