1832792 - Update RemoteValue deserialization methods to have `serializedValue` as first argument

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

All the deserialization helpers in RemoteValue.sys.mjs have the realm as first argument whereby the WebDriver BiDi spec and the serialization code has the value as first argument:

To deserialize local value given local protocol value, realm and session:

We should change the code to have serializedValue as first argument instead.

Comment 1

[Jing Zhu]

Hi, I would like to work on this ticket and I'll submit a patch soon. Thank you.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is great to hear! Let me know if you run into trouble. We are happy to help as well in the WebDriver channel on Matrix. Thanks!

Comment 3

[Jing Zhu]

Hi, should I also change the order of the arguments of the wrapper function in webdriver-bidi/modules/WindowGlobalBiDiModule.sys.mjs and all the places that import the module and call the deserialize function? Thanks

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes, we should have as best a unique ordering of arguments (to also match the specification) across all the places where it is used. So please change them all. Thanks.

Comment 5

[Jing Zhu]

Attachment: Bug 1832792 - [remote] Update RemoteValue deserialization methods to have `serializedValue` as first argument r?whimboo

Comment 6

[Jing Zhu]

Hi Henrik, I'm almost done with the patch. I modified and ran the tests in remote/webdriver-bidi/test. Are there any other tests that I need to run? Thanks.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jing Zhu from comment #6)

Hi Henrik, I'm almost done with the patch. I modified and ran the tests in remote/webdriver-bidi/test. Are there any other tests that I need to run? Thanks.

Yes, there are tests as well under testing/web-platform/tests/webdriver/tests/bidi that you would have to run. You can do that yourself or if the patch looks fine I can immediately create a try push to verify the changes. I'll have a look at your patch today. Thanks a lot for providing it!

Comment 8

[Jing Zhu]

Hi, I've ran the tests in testing/web-platform/tests/webdriver/tests/bidi and it passed. But I'm sorting some things out with my mercurial tree, can you hold off on the review for just a bit. Thank you

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sure. Right now the patch is still in WIP (work in progress) so I'll wait until you push with --no-wip so it gets marked with review requested.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/840505abaf56
[remote] Update RemoteValue deserialization methods to have `serializedValue` as first argument r=webdriver-reviewers,whimboo

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/840505abaf56

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Jing, thanks a lot for your contribution, and if you are interested to do more in our area just let us know what you are interested in. You can also find us in the WebDriver channel on Matrix.