`create` and `navigate.browsingContext` #55

sadym-chromium · 2021-10-05T15:27:11Z

Implement browsingContext.navigate command.
Add browsingContext.create -> params.type.
Add unit test for different event orders in browsingContext.create.
Made EVALUATOR_SCRIPT a property to allow unit testing.
BrowsingContextProcessor is responsible for handling events for testability.

foolip

I've reviewed about half way through, sending the comments I have before I head into a meeting.

src/bidiMapper/bidiProtocolTypes.ts

foolip · 2021-10-14T09:13:10Z

src/bidiMapper/domains/context/browsingContextProcessor.spec.ts

+      EVALUATOR_SCRIPT
+    );
+
+    // Actual `Context.create` logic involves several CDP calls, so mock it to avoid all the simulations.


This is a helpful comment, thanks!

src/bidiMapper/domains/context/browsingContextProcessor.ts

Co-authored-by: Philip Jägenstedt <philip@foolip.org>

bwalderman · 2021-10-18T19:06:42Z

src/bidiMapper/domains/context/browsingContextProcessor.spec.ts

+    browsingContextProcessor = new BrowsingContextProcessor(
+      cdpConnection,
+      'SELF_TARGET_ID',
+      sinon.fake as unknown as (t: Context) => Promise<void>,


Can you wrap the sinon.fake as unknown as (t: Context) => Promise<void> code into a typedFake function similar to typedSpy?

Also, shouldn't this call fake (e.g. fake())?

src/bidiMapper/domains/context/browsingContextProcessor.ts

bwalderman · 2021-10-18T19:23:40Z

src/bidiMapper/domains/context/browsingContextProcessor.ts

+
+      const existingContext = await this._tryGetContext(targetId);
+      if (existingContext) {
+        resolve(existingContext.toBidi());


I think awaiting here might introduce a potential race condition. Once Target.createTarget returns, we need to register the onAttachToTarget handler synchronously, or there's a chance the event will come and we'll miss it.

Agree. Added synchronous check, if the context is already known, in which case attachedToTarget is not needed.

I'm not sure I understand how the latest change will solve the race condition. I see that line 172 is checking for a known contextId. But, we're creating a new blank CDP target on line 165, so won't we always have a new unknown contextId?

If the event came at the moment of line 172, it means it was handled by _handleAttachedToTargetEvent in line 54. If not, the code will go synchronously till line 195, where event handler is attached. Having the second handler in line 195 is needed just to resolve the command promise.

It looks confusing. Another option is to keep a single event handler in this class BrowsingContextProcessor in line 54. But in this case, we have to notify the process_PROTO_browsingContext_create command when the context is actually attached and initialized.

The first (current) approach allows to use built-in events queue like browserCdpClient.Target.on. The second approach requires introducing our internal event queue and subscription mechanism.

Thank you, that makes more sense. I think this is safe then. Even though await this._tryGetContext(targetId) will yield to the runtime, any attachedToTarget event should be handled by _handleAttachedToTargetEvent and the context will be put in the map before _tryGetContext actually executes.

bwalderman · 2021-10-18T19:25:20Z

src/bidiMapper/domains/context/context.ts

@@ -80,6 +87,23 @@ export class Context {
    };
  }

+  public async navigate(
+    url: string,
+    wait: BrowsingContext.ReadinessState = 'none'


A default parameter makes sense at the API level, but for clarity, I'd prefer to avoid them in internal/implementation classes.

* Avoid race condition in waiting for a new target created; * Minor changes.

bwalderman · 2021-10-27T07:31:30Z

src/bidiMapper/domains/context/browsingContextProcessor.ts

+
+      const existingContext = await this._tryGetContext(targetId);
+      if (existingContext) {
+        resolve(existingContext.toBidi());


I'm not sure I understand how the latest change will solve the race condition. I see that line 172 is checking for a known contextId. But, we're creating a new blank CDP target on line 165, so won't we always have a new unknown contextId?

sadym-chromium · 2021-10-29T08:22:59Z

I replied @bwalderman in the thread #55 (comment)

bwalderman · 2021-11-01T23:39:25Z

src/bidiMapper/domains/context/browsingContextProcessor.ts

+
+      const existingContext = await this._tryGetContext(targetId);
+      if (existingContext) {
+        resolve(existingContext.toBidi());


Thank you, that makes more sense. I think this is safe then. Even though await this._tryGetContext(targetId) will yield to the runtime, any attachedToTarget event should be handled by _handleAttachedToTargetEvent and the context will be put in the map before _tryGetContext actually executes.

sadym-chromium requested review from foolip and bwalderman October 5, 2021 15:27

sadym-chromium changed the title ~~Navigate~~ browsingContext.navigate Oct 5, 2021

sadym-chromium marked this pull request as draft October 6, 2021 20:12

sadym-chromium force-pushed the navigate branch 3 times, most recently from f7700df to 89609d2 Compare October 7, 2021 14:40

sadym-chromium changed the base branch from main to typification October 7, 2021 14:40

sadym-chromium changed the title ~~browsingContext.navigate~~ browsingContext creation and navigation Oct 7, 2021

sadym-chromium marked this pull request as ready for review October 7, 2021 21:39

sadym-chromium changed the title ~~browsingContext creation and navigation~~ create and navigate.browsingContext Oct 8, 2021

sadym-chromium force-pushed the navigate branch 2 times, most recently from ba79ff1 to 4c0cf3d Compare October 8, 2021 09:56

sadym-chromium force-pushed the typification branch 3 times, most recently from 896600f to 1c08cbe Compare October 11, 2021 10:54

sadym-chromium force-pushed the navigate branch 2 times, most recently from 1be66dd to c1c3e1c Compare October 11, 2021 11:05

sadym-chromium force-pushed the typification branch from 1c08cbe to 536afd4 Compare October 13, 2021 11:07

Base automatically changed from typification to main October 13, 2021 11:09

sadym-chromium added 5 commits October 13, 2021 11:11

browsingContext.navigate

b32c8f4

Pass EVALUATOR_SCRIPT as a parameter for testibility.

0d040a0

Unit tests + avoid race condition while waiting Context creation.

b8adffc

minor change

5a92cd2

browsingContext.navigate -> PROTO.browsingContext.navigate

74244e3

sadym-chromium force-pushed the navigate branch from c1c3e1c to 74244e3 Compare October 13, 2021 11:12

foolip reviewed Oct 14, 2021

View reviewed changes

sadym-chromium and others added 3 commits October 14, 2021 11:37

Update src/bidiMapper/domains/context/browsingContextProcessor.ts

953a9ab

Co-authored-by: Philip Jägenstedt <philip@foolip.org>

PROTO.browsingContext.navigate -> browsingContext.navigate

8f83b58

browsingContext.create -> PROTO.browsingContext.create

3fd095b

bwalderman suggested changes Oct 18, 2021

View reviewed changes

PR fixes:

e515527

* Avoid race condition in waiting for a new target created; * Minor changes.

sadym-chromium requested a review from bwalderman October 19, 2021 11:49

bwalderman suggested changes Oct 27, 2021

View reviewed changes

bwalderman approved these changes Nov 1, 2021

View reviewed changes

sadym-chromium merged commit 4b2e246 into main Nov 2, 2021

sadym-chromium deleted the navigate branch November 2, 2021 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`create` and `navigate.browsingContext` #55

`create` and `navigate.browsingContext` #55

sadym-chromium commented Oct 5, 2021 •

edited

Loading

foolip left a comment

foolip Oct 14, 2021

bwalderman Oct 18, 2021

bwalderman Oct 18, 2021

sadym-chromium Oct 19, 2021

bwalderman Oct 18, 2021

sadym-chromium Oct 19, 2021

bwalderman Oct 27, 2021

sadym-chromium Oct 29, 2021

bwalderman Nov 1, 2021

bwalderman Oct 18, 2021

sadym-chromium Oct 19, 2021

bwalderman Oct 27, 2021

sadym-chromium commented Oct 29, 2021

bwalderman Nov 1, 2021

create and navigate.browsingContext #55

create and navigate.browsingContext #55

Conversation

sadym-chromium commented Oct 5, 2021 • edited Loading

foolip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadym-chromium commented Oct 29, 2021

Choose a reason for hiding this comment

`create` and `navigate.browsingContext` #55

`create` and `navigate.browsingContext` #55

sadym-chromium commented Oct 5, 2021 •

edited

Loading