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

Fix #243: Serve vendor favicons locally instead of remotely. #287

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

Osmose
Copy link
Contributor

@Osmose Osmose commented Nov 28, 2018

Tracking protection is blocking the eBay favicon from being loaded in private
browsing mode, but serving it locally instead of fetching it from the internet
avoids being blocked.

Copy link
Collaborator

@biancadanforth biancadanforth left a comment

Choose a reason for hiding this comment

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

R+!

It is curious that Ebay's favicon is blocked, when Amazon's is not, while both ebay.com and amazon.com are on the Disconnect blocklist that is the basis for Tracking Protection. @groovecoder , any idea why that would be?

Thinking about the future, if we ever were to open the extension to all sites (or a really large number of sites), would we want to use nsIFaviconService as mythmon suggested in PR #207 ? In general, how might we fix this if we were supporting * for our allowList pref?

@groovecoder
Copy link
Member

Looks like amazon.com is in the Content category, alexametrics.com is in the Analytics category, while domains like amazon-adsystem.com are in Advertising. So, amazon.com isn't in the default TP list for PBM. (default TP list for PBM doesn't include Content category)

ebay.com is in the Advertising category, so it's in the default TP list for PBM.

Good change, regardless. Disconnect may not have categorized amazon.com as a tracking domain, but reducing 3rd party requests is always great.

@Osmose
Copy link
Contributor Author

Osmose commented Nov 28, 2018

Thinking about the future, if we ever were to open the extension to all sites (or a really large number of sites), would we want to use nsIFaviconService as mythmon suggested in PR #207 ? In general, how might we fix this if we were supporting * for our allowList pref?

That's one way, and probably the way we want. There's also JS libraries that claim to be able to fetch the favicon for a webpage, or we assume that favicons are at domain.com/favicon.ico and only support that. But those seem worse than using the favicon service.

Tracking protection is blocking the eBay favicon from being loaded in private
browsing mode, but serving it locally instead of fetching it from the internet
avoids being blocked.
@Osmose Osmose merged commit a0c327f into mozilla:master Nov 28, 2018
@Osmose Osmose deleted the private-icon branch November 28, 2018 23:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants