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

Prototype channel mechanism (script.MessageEvent) #319

Merged
merged 2 commits into from
Mar 23, 2023
Merged

Conversation

thiagowfx
Copy link
Contributor

@thiagowfx thiagowfx commented Nov 21, 2022

Provide a way to communicate from scripts evaluated on the page side to
the BiDi client.

This is a safe (non-observable by the page script) analog of the Runtime.addBinding CDP command.

Prototype the mechanism using long-poll. Implementation details: http://go/webdriver-binding-implementation.

Reference: https://chromedevtools.github.io/devtools-protocol/tot/Runtime/#method-callFunctionOn

Bug: #294

Aligning with spec:

  • BindingValue -> Channel
  • ScriptBindingCalledParams -> MessageParams

Status

@sadym-chromium
Copy link
Collaborator

This PR is the beginning of the #294

@thiagowfx thiagowfx changed the title Add specification for binding mechanism Add bindings data types from specification Feb 8, 2023
@thiagowfx
Copy link
Contributor Author

Planning to resume this PR this week.

@thiagowfx
Copy link
Contributor Author

Q: What is the difference between protocol-parser.ts and protocol.ts?

Q: Some definitions have both schema + event type with zod.infer. Others don't have schema. What's the difference?

@sadym-chromium
Copy link
Collaborator

sadym-chromium commented Feb 23, 2023

Q: What is the difference between protocol-parser.ts and protocol.ts?

As Puppeteer doesn't need zod, but needs BiDi types, we had to duplicate them. Details: #350

Q: Some definitions have both schema + event type with zod.infer. Others don't have schema. What's the difference?

Can you give a specific example?

@thiagowfx thiagowfx force-pushed the thiagowfx/binding branch 2 times, most recently from e5f18d4 to 8e924fa Compare February 24, 2023 00:35
@thiagowfx thiagowfx changed the title Add bindings data types from specification Add specification for binding mechanism (script.bindingCalled) Feb 24, 2023
@thiagowfx thiagowfx force-pushed the thiagowfx/binding branch 3 times, most recently from be60d88 to ac8c148 Compare February 24, 2023 00:58
@thiagowfx
Copy link
Contributor Author

Side note: discussed off-line.

@thiagowfx thiagowfx force-pushed the thiagowfx/binding branch 2 times, most recently from 371e49d to cfae72a Compare February 27, 2023 16:23
@thiagowfx
Copy link
Contributor Author

@thiagowfx
Copy link
Contributor Author

@sadym-chromium I continued the design doc but now I am stuck. More specifically:

  1. What should getMessage do?
  2. What should sendMessage do?
  3. What should the deserialization (the case) do?
  4. Do we have a definition of "Binding" anywhere? https://w3c.github.io/webdriver-bidi/ contains nothing. I'd like to document it in the newly added class.
  5. When/where should I emit the script.bindingCalled event?
  6. What should the E2E test look like? Should it be a call to script.evaluate? If yes, that should it contain?

@sadym-chromium
Copy link
Collaborator

@sadym-chromium I continued the design doc but now I am stuck. More specifically:

  1. What should sendMessage do?

sendMessage should add message to the Binding's message queue.

  1. What should getMessage do?

getMessage is used for long-poll messages to Mapper from the page. It returns either a value from the message queue, if it's non-empty, or a promise, which will be resolved after a message appears in the queue.

  1. What should the deserialization (the case) do?

During deserialization, you should create a binding object on the page side, store it's objectId (will be required to poll messages), and provide its sendMessage to the user script as an argument.

  1. Do we have a definition of "Binding" anywhere? https://w3c.github.io/webdriver-bidi/ contains nothing. I'd like to document it in the newly added class.

It's a CDP term: https://chromedevtools.github.io/devtools-protocol/tot/Runtime/#method-addBinding. In BiDi it will probably called in another way:

  1. When/where should I emit the script.bindingCalled event?

Script Evaluator should start long-polling getMessage(), and send script.bindingCalled event for each message.

  1. What should the E2E test look like? Should it be a call to script.evaluate? If yes, that should it contain?

There is no way to pass argument to script.evaluate. The simplest e2e test should call script.callFunction with binding argument and with some script invoking that binding. Then you should verify the events are received by the Mapper. Additionally I can think of events order verification, and serialization.

Copy link
Collaborator

@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.

comments after 1:1

src/bidiMapper/domains/script/scriptEvaluator.ts Outdated Show resolved Hide resolved
src/bidiMapper/domains/script/scriptEvaluator.ts Outdated Show resolved Hide resolved
src/bidiMapper/domains/script/scriptEvaluator.ts Outdated Show resolved Hide resolved
src/bidiMapper/domains/script/scriptEvaluator.ts Outdated Show resolved Hide resolved
@thiagowfx thiagowfx force-pushed the thiagowfx/binding branch 2 times, most recently from 2bb7736 to 5911dcc Compare February 28, 2023 22:49
@thiagowfx
Copy link
Contributor Author

@sadym-chromium PTAL. I wrote 3 TODO(BLOCKER) comments.

thiagowfx pushed a commit that referenced this pull request Mar 22, 2023
This is a no-op in terms of new features, it just reorganizes some of
the script evaluator methods.

Bug: #294
@thiagowfx thiagowfx force-pushed the thiagowfx/binding branch 4 times, most recently from bea31a4 to 00e4cc6 Compare March 22, 2023 16:01
@thiagowfx thiagowfx enabled auto-merge (squash) March 22, 2023 22:14
@thiagowfx thiagowfx disabled auto-merge March 22, 2023 22:14
@thiagowfx thiagowfx enabled auto-merge (squash) March 22, 2023 22:14
@thiagowfx thiagowfx force-pushed the thiagowfx/binding branch 3 times, most recently from 69cb06c to 83c413a Compare March 23, 2023 10:01
Provide a way to communicate from scripts evaluated on the page side to
the BiDi client.

This is a safe (non-observable by the page script) analog of the
`Runtime.addBinding` CDP command.

Bug: #294

Co-authored-by: [email protected]
@thiagowfx thiagowfx merged commit e640735 into main Mar 23, 2023
@thiagowfx thiagowfx deleted the thiagowfx/binding branch March 23, 2023 10:46
thiagowfx pushed a commit that referenced this pull request Mar 23, 2023
thiagowfx pushed a commit that referenced this pull request Mar 23, 2023
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.

2 participants