Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Make website icon(s) available through Engine/Session #494

Closed
pocmo opened this issue Jul 25, 2018 · 2 comments
Closed

Make website icon(s) available through Engine/Session #494

pocmo opened this issue Jul 25, 2018 · 2 comments
Assignees
Labels
<engine-gecko> Component: browser-engine-gecko <engine-system> Component: browser-engine-system (WebView) 🌟 feature New functionality and improvements 🐉 Fenix Feature needed for Fenix needs:gv To implement/fix this we need a new API in GeckoView 🚀 Rocket Requirements for Firefox Rocket <session> Component: browser-session
Milestone

Comments

@pocmo
Copy link
Contributor

pocmo commented Jul 25, 2018

This is a tricky one. There are a gazillion web standards that provide icons. Different use cases may need different types of icons or sizes. Sometimes we want to do requests to get an icon, other times we only want to pull from the cache (e.g. for privacy reasons).

The code in Fennec is quite complex and tries its best to download, save, load, decode, .. icons and do things like color extraction:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/icons

I spend quite some time with that code and there are a bunch of pitfalls when it comes to icons.

┆Issue is synchronized with this Jira Task

@pocmo pocmo added 🌟 feature New functionality and improvements <session> Component: browser-session <engine-gecko> Component: browser-engine-gecko <engine-system> Component: browser-engine-system (WebView) 🚀 Rocket Requirements for Firefox Rocket labels Jul 25, 2018
@jonalmeida jonalmeida added this to the 0.24 🚀 milestone Sep 14, 2018
@jonalmeida jonalmeida self-assigned this Sep 14, 2018
@csadilek csadilek modified the milestones: 0.24 🚀, 0.25 🐼 Sep 21, 2018
@jonalmeida
Copy link
Contributor

jonalmeida commented Sep 21, 2018

We're thinking that since importing this icon code, is a rather large project, we can make it simpler in a few ways:

  1. Add them into a separate component: browser-icons
    • This would be separate from the ui-icons because it doesn't include any UI elements, nor is it a feature components that contains dependent components, and it can be de-coupled from a specific engine*.
  2. Include just the basics of this module which include downloading, and storing (in-memory) the favicons.
    • There are more complicated things to do here as well that involve an disk LRU cache which have third-party dependencies that we would like to avoid using.

*The fennec implementation retrieves the icon path to be retrieved. To get this to work on system-engine as well we would need another abstraction.

Misc

  • One of the issues that turned up during the Fennec implementation, is because we're using HttpUrlConnection in the Java world to retrieve the icon, there are policies that the browser is aware off that we aren't, when we want to make requests.

Once we have this working, we can iterate on top this to include the more complicated caching and policies.

Favicons are going to be fun to implement. ;)


^ valid favicon.

@pocmo
Copy link
Contributor Author

pocmo commented Sep 21, 2018

Some random annoyances I remember from the Fennec icon code:

  • Different features want different icon sizes (e.g. displaying an icon in the list of bookmarks vs. a Android home screen shortcut). I think in Fennec we try to save a rather larger icon to disk (but not too large) and load a resized version. With memory caching this gets complicated when you need different sizes at the same time.

  • Favicons (and others) are defined in the DOM and this is not static. As the DOM can change so can the icon. Extreme example: https://www.favitimer.com/

  • This bug was a bigger refacotring of the code and some of the linked issues contain some of the problems and reasons: https://bugzilla.mozilla.org/show_bug.cgi?id=1290014 - see meta bug too: https://bugzilla.mozilla.org/show_bug.cgi?id=1265712

@pocmo pocmo modified the milestones: 0.25 🐼, 0.26 Sep 26, 2018
@pocmo pocmo modified the milestones: 0.26 🚴, 0.27 🕹 Oct 5, 2018
@csadilek csadilek modified the milestones: 0.27 🕹, 0.28 Oct 15, 2018
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Oct 21, 2018
@pocmo pocmo modified the milestones: 0.28 😻, 0.29 🏁, 0.30 Oct 22, 2018
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Nov 5, 2018
@pocmo pocmo modified the milestones: 0.30 🐧, 0.31 Nov 5, 2018
@pocmo pocmo modified the milestones: 0.31 🥧, 0.32 Nov 12, 2018
@pocmo pocmo removed this from Sprint Backlog in A-C: Android Components Sprint Planning Nov 19, 2018
@pocmo pocmo removed this from the 0.32 🎿 milestone Nov 19, 2018
@jonalmeida jonalmeida removed their assignment Jan 21, 2019
@vesta0 vesta0 added the needs:gv To implement/fix this we need a new API in GeckoView label Feb 13, 2019
@pocmo pocmo added this to Backlog in A-C: Website icons Feb 20, 2019
@pocmo pocmo added the 🐉 Fenix Feature needed for Fenix label Feb 20, 2019
@pocmo pocmo moved this from Backlog to In progress in A-C: Website icons Apr 5, 2019
@pocmo pocmo self-assigned this Apr 5, 2019
pocmo added a commit to pocmo/android-components that referenced this issue Apr 5, 2019
A-C: Website icons automation moved this from In progress to Done Apr 5, 2019
@pocmo pocmo added this to the 0.50.0 🦑 milestone Apr 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
<engine-gecko> Component: browser-engine-gecko <engine-system> Component: browser-engine-system (WebView) 🌟 feature New functionality and improvements 🐉 Fenix Feature needed for Fenix needs:gv To implement/fix this we need a new API in GeckoView 🚀 Rocket Requirements for Firefox Rocket <session> Component: browser-session
Projects
No open projects
Development

No branches or pull requests

4 participants