Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Ship hard-coded search engine favicons #12689

Closed
tildelowengrimm opened this issue Jan 17, 2018 · 5 comments · Fixed by #12698
Closed

Ship hard-coded search engine favicons #12689

tildelowengrimm opened this issue Jan 17, 2018 · 5 comments · Fixed by #12698
Assignees
@diracdeltas
Labels
misc/favicon privacy QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Milestone
0.20.x (Release C...

Comments

@tildelowengrimm
Copy link

tildelowengrimm commented Jan 17, 2018 •
edited by bsclifton
Loading

Test plan

#12698

Original issue description

Per @liunkae's comment on #12632, we automatically try to fetch favicons for search engines shipped in about:preferences#search. We shouldn't make any non-user-initiated requests to those sites. I'd like to shipped cached favicons for those sites, and have the search UI only read out of the favicon cache. That way, we can always show icons shown in the search pane, they'll always be as current as our latest release. If a user regularly visits a site and that site updates their icon, the user will see a newer icon in the search pane based on the timing of their recent visits.
@tildelowengrimm
Copy link
Author

We'll need the favicon cache from #2697 to update them based on user activity. Until that's available, I'd prefer to ship hardcoded icons for these sites to avoid making non-user-initiated requests.

@diracdeltas
Copy link
Member

related to #11108

@diracdeltas
Copy link
Member

shipping hardcoded icons is easy. i think we will probably do that first and have a follow up issue for cache invalidation

@diracdeltas
Copy link
Member

diracdeltas commented Jan 17, 2018 •
edited
Loading

@flamsmark unless i'm misunderstanding, let's use #2697 for the caching issue (which would also apply to bookmarks, etc.) and use this issue for hard-coding favicons in the browser search panel

@diracdeltas diracdeltas self-assigned this Jan 17, 2018
@diracdeltas diracdeltas mentioned this issue Jan 17, 2018
Closed
diracdeltas added a commit that referenced this issue Jan 17, 2018
@diracdeltas
Load search favicons locally in about:preferences
ac59329 
fix #12689
fix #11108

Test Plan:
1. go to about:preferences and open the 'network requests' tab in devtools
2. click on the search preferences tab. you should not see any network requests to remote sites.
3. all the search favicons should load correctly
@diracdeltas diracdeltas mentioned this issue Jan 17, 2018
Merged
10 tasks
@tildelowengrimm tildelowengrimm mentioned this issue Jan 18, 2018
Open
@tildelowengrimm tildelowengrimm mentioned this issue Jan 18, 2018
Open
@tildelowengrimm tildelowengrimm changed the title Ship cached search engine favicons Ship hard-coded search engine favicons Jan 18, 2018
@diracdeltas
Copy link
Member

Going to mark for 0.20.x since it is an easy privacy win and relatively low-risk code change

@diracdeltas diracdeltas added this to the 0.20.x (Beta Channel) milestone Jan 18, 2018
This was referenced Jan 24, 2018
Closed
Closed
Closed
@kjozwiak kjozwiak mentioned this issue Jan 31, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@diracdeltas diracdeltas

Labels
misc/favicon privacy QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Projects
None yet
Milestone
0.20.x (Release Channel)
Development

Successfully merging a pull request may close this issue.

Load search favicons locally in about:preferences brave/browser-laptop
6 participants
@diracdeltas @tildelowengrimm @kjozwiak @bsclifton @srirambv @LaurenWags