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

Cdp serialization / deserialization #123

Merged
merged 16 commits into from
May 11, 2022
Merged

Cdp serialization / deserialization #123

merged 16 commits into from
May 11, 2022

Conversation

sadym-chromium
Copy link
Collaborator

@sadym-chromium sadym-chromium commented May 6, 2022

Use CDP generateWebDriverValue for serialization.

The PR happened to be quite huge, as it involved lots of refactoring.

  • Move serialization to CDP-side by using generateWebDriverValue in CallFunction, Evaluate and when serialization is needed.
  • Remove in-realm serialization / deserialization.
  • Reformatted some files.
  • Implemented CDP-serialization.
  • Serialization / deserialization tests.
  • Make e2e and WPT tests run on chrome-dev channel (as serialization is not in the stable Chrome yet).
  • Added e2e timeout 15 minutes to prevent hanging the step forever.

Maksim Sadym added 10 commits May 5, 2022 14:11
* Move serialization to CDP-side by using `generateWebDriverValue` in CallFunction, Evaluate and when serialization is needed.
* Remove in-realm serialization / deserialization.
* Reformatted some files.

Not done:
* Complete serialization tests.
* Deserialization tests.
.github/workflows/e2e.js.yml Outdated Show resolved Hide resolved
@sadym-chromium sadym-chromium force-pushed the cdp_ser branch 5 times, most recently from 1fe5532 to cec2924 Compare May 6, 2022 14:43
Force WPT using the dev chrome channel
@sadym-chromium sadym-chromium marked this pull request as ready for review May 6, 2022 15:09
#initializePageLifecycleEventListener() {
this.#cdpClient.Page.setLifecycleEventsEnabled({ enabled: true });

this.#cdpClient.Page.on('lifecycleEvent', async (params) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should there be .off calls for every .on call somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's keep it out of scope of this PR. Ideally, we should subscribe to events only when user subscribed to them, and unsubscribe when user unsubscribed. But for now Mapper (as well as Puppeteer, AFAIK) just listens to all event and sends the needed to user.

// Primitive Protocol Value
// https://w3c.github.io/webdriver-bidi/#data-types-protocolValue-primitiveProtocolValue
case 'undefined': {
return { unserializableValue: 'undefined' };
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does unserializableValue means? a value that cannot be deserialized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Runtime.CallArgument. unserializableValue is basically a string that is evaluated instead of being JSON-deserialized.

@OrKoN
Copy link
Collaborator

OrKoN commented May 10, 2022

LGTM in general but I am not very familiar with the bidi spec to see if serialisation is correct.

@sadym-chromium
Copy link
Collaborator Author

I am not very familiar with the bidi spec to see if serialisation is correct.

We delegate the serialization logic to CDP, but verify it in e2e tests. Deserialization is on the Mapper side, but it is tested by e2e as well.

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.

None yet

2 participants