Skip to content

Conversation

@samdenty
Copy link
Contributor

@samdenty samdenty commented Feb 29, 2024

This PR fixes #206345 & #206296.

Before the startupKind was only implemented for electron, but we can also do it on web (only found out cause chatgpt suggested). This makes the editor restoration logic better (and i'm sure other parts that use startupKind too)


Review PR in StackBlitz Submitted with StackBlitz.

@samdenty samdenty changed the title fix Feb 29, 2024
@bpasero bpasero added this to the Backlog milestone Mar 1, 2024
@samdenty samdenty requested a review from bpasero March 1, 2024 18:31
@bpasero bpasero modified the milestones: Backlog, March 2024 Mar 4, 2024
@bpasero
Copy link
Member

bpasero commented Mar 4, 2024

@samdenty any thoughts on https://web.dev/articles/navigation-and-resource-timing#acquiring_timings_in_application_code. I wonder about the actual perf impact of calling performance.getEntriesByType('navigation'). In my testing on macOS, I could not find this call to take long, but I wanted to point out the blog post that suggests a observer pattern.

@samdenty
Copy link
Contributor Author

samdenty commented Mar 4, 2024

iterating over a large number of entries, or even repeatedly polling the performance buffer to find new entries

I think since this we're calling performance.getEntriesByType('navigation') there should only ever be one event, so browsers shouldn't have a hard time resolving that. If it was a different type, then yeah that could be an issue.

The observer pattern seems to be for when you want to subscribe to the latest events, instead of polling the API

@bpasero
Copy link
Member

bpasero commented Mar 4, 2024

👍 , I think this can go in, I pushed some slight 👄 changes.

One thing that will be received as some kind of regression potentially is that in web, we no longer just restore the previous view you had opened (you removed the isWeb check). But on the other hand, that is consistent with the behaviour on desktop and as such actually the intended behaviour. I hope this will not backfire though...

@bpasero
Copy link
Member

bpasero commented Mar 4, 2024

Interesting: 5727c82

@bpasero bpasero merged commit bd60317 into microsoft:main Mar 4, 2024
yiliang114 pushed a commit to yiliang114/vscode that referenced this pull request Mar 6, 2024
* fixes microsoft#206345

* fixes microsoft#206296

* fix: don't hard check isWeb

* chore: review

* cleanup

* cleanup

---------

Co-authored-by: Benjamin Pasero <benjamin.pasero@microsoft.com>
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

3 participants