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

chore(tracing): fix some of the start/stop scenarios #6337

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

pavelfeldman
Copy link
Member

No description provided.

@@ -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) {
Copy link
Contributor

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.

Copy link
Member Author

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) => {
Copy link
Contributor

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.

@@ -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);
Copy link
Contributor

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 😄

if (!initial) {
debugLogger.log('api', ` navigated to "${url}"`);
this._page.frameNavigatedToNewDocument(frame);
frame.emit(Frame.Events.Navigation, navigationEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one 😄

@pavelfeldman pavelfeldman merged commit 922d9ce into microsoft:master Apr 27, 2021
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