-
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
chore(tracing): fix some of the start/stop scenarios #6337
Conversation
c56ad94
to
a271dd0
Compare
@@ -432,10 +432,12 @@ export function frameSnapshotStreamer(snapshotStreamer: string, snapshotBinding: | |||
}; | |||
|
|||
let allOverridesAreRefs = true; | |||
for (const sheet of this._allStyleSheetsWithUrlOverride) { | |||
for (const sheet of this._staleStyleSheets) { |
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 think we need to send all stylesheets that ever had an override every time, even when they are not stale (in this case it will be a number ref). Otherwise, the renderer will use the non-modified resource for this snapshot, instead of the last modified one.
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 won't claim I understand this part, but I don't change behavior here. Maybe you can follow up?
this._interval = interval; | ||
if (interval) | ||
this._streamSnapshot(); | ||
const visitNode = (node: Node | ShadowRoot) => { |
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.
In theory, we can replace the cache with WeakMap and just clear it here. Up to you.
src/server/frames.ts
Outdated
@@ -197,10 +198,11 @@ export class FrameManager { | |||
|
|||
frame._onClearLifecycle(); | |||
const navigationEvent: NavigationEvent = { url, name, newDocument: frame._currentDocument }; | |||
frame.emit(Frame.Events.Navigation, navigationEvent); | |||
this._page.frameNavigatedToNewDocument(frame); |
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.
Let's undo this line 😄
src/server/frames.ts
Outdated
if (!initial) { | ||
debugLogger.log('api', ` navigated to "${url}"`); | ||
this._page.frameNavigatedToNewDocument(frame); | ||
frame.emit(Frame.Events.Navigation, navigationEvent); |
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.
And this one 😄
a271dd0
to
6c9b308
Compare
No description provided.