-
Notifications
You must be signed in to change notification settings - Fork 16.9k
fix: avoid redundant page-favicon-updated events on setBounds
#49464
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
base: main
Are you sure you want to change the base?
fix: avoid redundant page-favicon-updated events on setBounds
#49464
Conversation
|
💖 Thanks for opening this pull request! 💖 Semantic PR titlesWe use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Commit signingThis repo enforces commit signatures for all incoming PRs. PR tipsThings that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
page-favicon-updated events on setBounds
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.
Confirmed this corrects the issue. Please add a test here to validate the fix. You also need to sign your commit.
| iter->icon_url.is_valid()) | ||
| unique_urls.insert(iter->icon_url); | ||
| } | ||
| // Deduplicate: only emit if favicon URLs actually changed |
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.
| // Deduplicate: only emit if favicon URLs actually changed | |
| // Only emit if favicon URLs actually changed |
|
I wonder if this just works around an upstream issue that should really be fixed in Chromium. Why would |
| private: | ||
| // Store last emitted favicon URLs to avoid duplicate page-favicon-updated | ||
| // events | ||
| base::flat_set<GURL> last_favicon_urls_; |
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.
(nit) Needs #include "base/containers/flat_set.h" at the top of the header file
Description of Change
Closes #49392
Fixes an issue where calling
setBoundson aWebContentsViewunintentionally triggers thepage-favicon-updatedevent even when the favicon has not actually changed.Currently, layout-related updates such as resizing or repositioning a
WebContentsViewcan cause redundant emissions ofpage-favicon-updated, which is unexpected and leads to duplicate callbacks in user code. This change ensures that the event is only emitted when there is a real change in the favicon data, preventing unnecessary and misleading notifications.This makes the
page-favicon-updatedevent more semantically correct and avoids side effects caused by purely visual or layout updates.Checklist
npm testpassesRelease Notes
Notes: Fixed an issue where calling
setBoundson aWebContentsViewcould trigger redundantpage-favicon-updatedevents even when the favicon had not changed.