Skip to content

Browser favicons: better error handling & data URL support#305244

Merged
kycutler merged 2 commits intomainfrom
kycutler/faviconfixes
Mar 26, 2026
Merged

Browser favicons: better error handling & data URL support#305244
kycutler merged 2 commits intomainfrom
kycutler/faviconfixes

Conversation

@kycutler
Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings March 26, 2026 16:36
@vs-code-engineering
Copy link
Copy Markdown
Contributor

vs-code-engineering bot commented Mar 26, 2026

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@jruales

Matched files:

  • src/vs/platform/browserView/electron-main/browserView.ts

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Electron-main BrowserView favicon pipeline to better handle real-world favicon sources by supporting data: URLs directly and rejecting non-image responses when fetching remote favicon URLs.

Changes:

  • Treat data:image/* favicon URLs as already-resolved and return them directly.
  • Validate that fetched favicon responses have an image/* content-type before converting to a data: URL.
Comments suppressed due to low confidence (1)

src/vs/platform/browserView/electron-main/browserView.ts:183

  • The content-type check is case-sensitive (startsWith('image/')), but MIME types are case-insensitive. Consider lowercasing the header value (or otherwise doing a case-insensitive check) after trimming/parsing it, to avoid incorrectly rejecting valid image responses like Image/PNG.
						if (!type?.startsWith('image/')) {
							throw new Error(`Favicon is not an image: ${type}`);

@kycutler kycutler merged commit 82287d8 into main Mar 26, 2026
27 of 29 checks passed
@kycutler kycutler deleted the kycutler/faviconfixes branch March 26, 2026 21:01
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.

4 participants