Skip to content
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

Fixed web plugin favicons, binary size reduction, perf optimizations #97

Merged
merged 12 commits into from
Mar 30, 2022

Conversation

mmstick
Copy link
Member

@mmstick mmstick commented Mar 29, 2022

  • Enable LTO, panic=abort, set strip-true by default to reduce binary size
  • Swap the default system allocator for mimalloc, which is 2.6x faster and more secure
  • Fixed the symbolic link names installed by the justfile
  • Fixed missing favicons on some websites, like Amazon
    • It seems Google isn't correctly checking the root of a domain for the favicon anymore

Binary size should now be reduced from ~8MB to ~6MB, which is shared memory to all plugins.

Closes #98

@mmstick mmstick requested review from a team March 29, 2022 14:57
@jacobgkau jacobgkau linked an issue Mar 29, 2022 that may be closed by this pull request
@jacobgkau
Copy link
Member

This fixes favicon caching for Amazon and Crates.io. However, it breaks the icon for Flathub.

Screenshot from 2022-03-29 11-07-00

Interestingly, the DEV icon has changed. Before, it used the same favicon that Firefox recognizes:

Screenshot from 2022-03-29 11-08-53

After, it's different:

Screenshot from 2022-03-29 11-07-03

@mmstick
Copy link
Member Author

mmstick commented Mar 29, 2022

Seems to be the icon they're using here: https://dev.to/favicon.ico

@jacobgkau
Copy link
Member

Seems to be the icon they're using here: https://dev.to/favicon.ico

They have their shortcut icon set to a favicon.ico on a different domain:

image

@jackpot51
Copy link
Member

  • Swap reqwest for ureq for significant reduction in dependencies and binary size

I do not recommend this. I have noticed reqwest has better performance than ureq.

@mmstick
Copy link
Member Author

mmstick commented Mar 29, 2022

I've reverted the change to keep reqwest

jackpot51
jackpot51 previously approved these changes Mar 29, 2022
@jacobgkau
Copy link
Member

Amazon and Crates.io are showing the Google API's default favicon instead of their own icons again on 09d126e.

@mmstick
Copy link
Member Author

mmstick commented Mar 29, 2022

Seems the correct order would be first scraping the page for the favicon path, then checking the root path, and falling back on Google as a last resort.

@jacobgkau
Copy link
Member

On ca4eec7, the lib.rs and ArXiv icons don't load:

Screenshot from 2022-03-30 09-23-17

Screenshot from 2022-03-30 09-25-36

The dev icon doesn't load, and doesn't show a placeholder space either, and it shows a backtrace:

Screenshot from 2022-03-30 09-23-50

Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

On e5f381b, all favicons are showing as expected again. Regression testing passed:
launcher-pr97-impish.txt
launcher-pr97-focal.txt

@mmstick mmstick merged commit 0893990 into master Mar 30, 2022
@mmstick mmstick deleted the launcher branch March 30, 2022 17:06
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.

Desktop entries plugin: unknown cmd Some default web favicons not loading properly
3 participants