1185063 - Use site icon in task manager cards

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Task Manager, defect)

Description

[Sam Foster [:sfoster] (he/him)]

bug 1174813 added a getSiteIconUrl method to AppWindow to provide the best available icon for web content. Task Manager Cards should make use of this.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] sfoster:card-siteicon-bug-1185063 > mozilla-b2g:master

Comment 2

[Sam Foster [:sfoster] (he/him)]

(In reply to Autolander from comment #1)
> Created attachment 8640325 [details] [review]
> [gaia] sfoster:card-siteicon-bug-1185063 > mozilla-b2g:master

Implementation + unit tests are done, I'm currently looking at a couple marionette tests

Comment 3

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8640325 [details] [review]
[gaia] sfoster:card-siteicon-bug-1185063 > mozilla-b2g:master

The patch makes use of AppWindow's getSiteIconUrl in task manager cards, replacing the CardHelper stuff with associated unit tests. 

For the integration tests, I needed a way to verify the card was showing the expected icon - that didn't rely on string matching URLs. So I've added a set of color:size matched images and a favicon.ico that bundles them all, and the assertions sample the color to determine if the expected icon was loaded. 

We'll need more integration tests of the pin-the-web icon functionality as a whole, but this seems like a reasonable start. 

Oh I rolled in a change to WebManifestHelper to reject when you get a bad JSON response (e.g. trailing comma) that threw me. Maybe that belongs in a different patch?

Comment 4

[Etienne Segonzac (:etienne)]

Comment on attachment 8640325 [details] [review]
[gaia] sfoster:card-siteicon-bug-1185063 > mozilla-b2g:master

Awesome!
r=me with the comments addressed and the test fixed.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comments, test, nits all fixed and landed on master: 
https://github.com/mozilla-b2g/gaia/commit/c042015a22643c3a39f26e299dc3f9033cc79125