Closed Bug 1185063 Opened 7 years ago Closed 7 years ago

Use site icon in task manager cards

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
FxOS-S4 (07Aug)

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file)

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.
Assignee: nobody → sfoster
Target Milestone: --- → FxOS-S3 (24Jul)
Blocks: pin-the-web
Target Milestone: FxOS-S3 (24Jul) → FxOS-S4 (07Aug)
(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 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?
Attachment #8640325 - Flags: review?(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.
Attachment #8640325 - Flags: review?(etienne) → review+
Comments, test, nits all fixed and landed on master: 
https://github.com/mozilla-b2g/gaia/commit/c042015a22643c3a39f26e299dc3f9033cc79125
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.