968156 - Store multiple icons from mozbrowsericonchange events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Description

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

We should be storing different icons that we receive on mozbrowsericonchange event so that we can later select the best icon to show in different scenarios.

Comment 1

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Attachment: GitHub PR

Store an array of icons for each URL if necessary. It probably needs more tests, but I'd like to know if the approach is valid.

Comment 2

[Dale Harvey (:daleharvey)]

Sergi, apologies I dont really know what happened with this review, but looking to get this functionality implemented at some point, so you fancy reviving this or want me to?

I am not 100% on storing the sizes as integer representations, I think we should probably store things as directly as possible and let the client do any logic / processing, so I think we should probably just use the sizes as the key

Sadly there isnt a decent solution for testing datastore yet so I think this would have to use a mocked datastore

Comment 3

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi Dale,

I don't even remember what I did there. I will take a look again and get back to you :) Thanks for the reply though!

Sergi

Comment 4

[Dale Harvey (:daleharvey)]

Sorry, screwed up the blocking, this is gonna block enabling places as changing it after would require a schema change / duck typing, Sergi I am gonna steal since we want to get places enabled within the next few days, apologies

Comment 5

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/22035

I have changed search/js/providers/places.js and browser_context_menu.js to just use the first icon it finds, these will be fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1042027

This changes app_window to maintain a list of icons for a current session of a mozbrowser window, places.js uses that list to construct the places store and other components (browser_context_menu), can use that to pick the highest quality available icon

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8460222 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/22035

Let's
* Store favicon in AppWindow.prototype.favicons
* have unit test in app_window_test
* implement getIconBySize() not necessary but if you want to fetch icon by size in the future.

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 8460222 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/22035

Thanks for the comments

I renamed and moved to app.favicons and added unit tests for app_window

I didnt add the getIcon function, we need to share the implementation between apps that dont have access to app_window (the search app), so at the least it will need to be a shared function, the implementation is being done in https://bugzilla.mozilla.org/show_bug.cgi?id=1042027 so I think best discussed there

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8460222 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/22035

r+ with nit: don't put object in prototype :/ Initial it in mozbrowsericonchange handler would be fine.

Comment 9

[Dale Harvey (:daleharvey)]

Fixed nit, cheers for the review

landed in: https://github.com/mozilla-b2g/gaia/commit/e80dc912903ecafbeff4229af5d6bd4daaf7812b

Comment 10

[Zac C (:zac)]

This patch has caused https://bugzilla.mozilla.org/show_bug.cgi?id=1044227