-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
api(chromium): remove Target from public API #1163
Conversation
"New pages can be reported by firing 'pageevent' on the BrowserContext" This makes so many things easier. Thank you! 🙏 |
- <[PageEvent]> | ||
|
||
Emitted when a new Page is created in the BrowserContext. The event will also fire for popup | ||
pages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This event will fire for popup pages, shift-clicks, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this event fire if you open a new tab with the browser ui? Tested this locally and it triggers for newPage and pages opened in the UI for chromium (mac-745253) 👍.
docs/api.md
Outdated
@@ -1661,6 +1668,13 @@ This method returns all of the dedicated [WebWorkers](https://developer.mozilla. | |||
|
|||
> **NOTE** This does not contain ServiceWorkers | |||
|
|||
### class: PageEvent | |||
|
|||
Event object passed to the listeners of 'pageevent' on 'BrowserContext'. Provides access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ```
and ['popup'](link) event of Page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
> **NOTE** Only includes targets from this browser context. | ||
> **NOTE** Only works with persistent context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you could provide more context on why this only works in persistent context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the background pages are always created in the default context which doesn't work well with browser context level isolation that we want to have. If this becomes a problem we can extend the API in the future.
docs/api.md
Outdated
@@ -264,6 +264,7 @@ await context.close(); | |||
|
|||
<!-- GEN:toc --> | |||
- [event: 'close'](#event-close) | |||
- [event: 'pageevent'](#event-pageevent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the name pageevent
. This looks to me like something happened inside the page. What about newpage
or pagecreated
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to 'page'
@@ -1661,6 +1668,13 @@ This method returns all of the dedicated [WebWorkers](https://developer.mozilla. | |||
|
|||
> **NOTE** This does not contain ServiceWorkers | |||
|
|||
### class: PageEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. What about NewPageEvent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep this one as is as it's shorter. Happy to rename if others prefer NewPageEvent or PageCreatedEvent.
docs/api.md
Outdated
#### event: 'targetdestroyed' | ||
- <[ChromiumTarget]> | ||
#### event: 'serviceworker' | ||
- <[Page]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service worker is a page?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Updated.
53efc49
to
6ea5259
Compare
Events.CRBrowserContext.Target{Created,Destroyed,Changed}
in favor of"pageevent"
onBrowserContext
BrowserContext
ChromiumBrowserContext.backgroundPages()
andEvents.CRBrowserContext.BackgroundPage
event on the context.Events.CRBrowserContext.ServiceWorker
event.ChromiumBrowserContext.createSession(page)
ChromiumBrowser.createBrowserSession()
References: #1101