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

feat: implement browsingContext.activate #1002

Merged
merged 2 commits into from
Jul 12, 2023
Merged

feat: implement browsingContext.activate #1002

merged 2 commits into from
Jul 12, 2023

Conversation

OrKoN
Copy link
Collaborator

@OrKoN OrKoN commented Jul 11, 2023

@thiagowfx
Copy link
Contributor

thiagowfx commented Jul 11, 2023

TODO for you (either in this PR or in the next one, but keep it in mind): Update

await this.#cdpTarget.cdpClient.sendCommand('Page.bringToFront');

...to use the bidi API instead of calling the CDP command directly.

We may end up removing this line altogether at some point, but for now it would be better to use the BiDi API, IMHO.

@thiagowfx
Copy link
Contributor

LGTM (adding one test would be nice).

@jrandolf-2 jrandolf-2 self-requested a review July 11, 2023 16:45
Copy link
Contributor

@jrandolf-2 jrandolf-2 left a comment

Choose a reason for hiding this comment

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

+1 to adding a test

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jul 12, 2023

Added a basic test (more extensive test coverage will be done by WPT)

@OrKoN OrKoN marked this pull request as ready for review July 12, 2023 07:08
@OrKoN OrKoN enabled auto-merge (squash) July 12, 2023 07:08
@OrKoN OrKoN merged commit 22e2417 into main Jul 12, 2023
13 checks passed
@OrKoN OrKoN deleted the orkon/activate branch July 12, 2023 07:48
Copy link
Contributor

@thiagowfx thiagowfx Jul 12, 2023

Choose a reason for hiding this comment

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

I would have suggested to add this to the browsing context test file.

The top-level directory for tests/* becomes increasingly messy if we put everything there.

Alternatively, we could mirror WPT and create a top-level browsing_context dir, and move this therein.

(Feel free to re-request review before submitting, I tend to review fast and would have suggested this in advance.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds like a separate issue. Putting everything into the single browsing context test file is messy too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's worth to create many subdirectories, like WPT? We could start to slowly migrate our codebase to a similar pattern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think subdirectories is the way to go where the directory name is the module name. I think we should not create a directory per command yet (a file per command/event should be okay)

Lightning00Blade pushed a commit that referenced this pull request Jul 21, 2023
🤖 I have created a release *beep* *boop*
---


##
[0.4.18](chromium-bidi-v0.4.17...chromium-bidi-v0.4.18)
(2023-07-21)


### Features

* implement browsingContext.activate
([#1002](#1002))
([22e2417](22e2417))
* implement drag n' drop
([#1006](#1006))
([6443045](6443045))
* **print:** throw unsupported operation when content area is empty
([#992](#992))
([71a8b5c](71a8b5c)),
closes
[#518](#518)
* refactor scripts and realms and fix generator serialization
([#1013](#1013))
([73ea6f0](73ea6f0)),
closes
[#562](#562)
* support iterator serialization
([#1042](#1042))
([9dff121](9dff121))


### Bug Fixes

* don't hold finished requests in memory
([#1058](#1058))
([f15163a](f15163a))
* NavigationStarted Event for sub-frames
([#1009](#1009))
([c4841f8](c4841f8))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
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.

3 participants