1073188 - BMW site icon missing when saving to Home Screen

Status: VERIFIED FIXED

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

Description

[Marcia Knous [:marcia]]

Attachment: 2014-09-25-13-21-05.png

Flame, while using:

Gaia   8061ab487d42cbc49b329fd68b9ca90e0fe477e6
SourceStamp e970bc96f8b5
BuildID 20140925000204
Version 34.0a2
base: v180

STR:
1. Load http://www.bmwusa.com/
2. Use the menu to save to Homescreen

Actual: Attached screenshot -> Icon is missing
Expected: Icon would be there

Not sure what it is about that site, but other sites I save show the icon fine.

Comment 1

[Kevin Grandon :kgrandon]

Few issues.

1 - The bmw site does not serve an icon that we can use. This is an evangelism bug. When you navigate to bmwusa.com, it redirects you to m.bmwusa.com. We then can't find a proper icon, and when we try to fallback to the favicon at http://m.bmwusa.com/favicon.ico, it does a 301 redirect back to the homepage. 

2 - We receive a valid response for the favicon, but fail to generate an icon for it as we should. In this case we should default back to the default "rocketship" icon.

Comment 2

[Kevin Grandon :kgrandon]

[Blocking Requested - why for this release]: If this is appearing for a site like BMW, it could be a fairly widespread issue. I believe that this is not an issue or much less of an issue in 2.0 due to the fact that we did not process favicons. Now that we process favicons, we are returning invalid icons much more frequently and leading to breakage.

The difference is that in 2.0 we displayed a rocketship, and in 2.1 the icon breaks and we display nothing.

Comment 3

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8495569 [details] [review]
Github pull request

Cristian - could you help me review this one? I've added a guard in the case of an image not loading, it was an oversight before. Also added a marionette test to make sure we don't break this. Thanks!

Comment 5

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8495569 [details] [review]
Github pull request

LGTM, thanks a lot, no comments it is ok for me

Comment 6

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/ac29bbbb4aff4eb4ceb78ea7942038fd6042fc43

Comment 7

[Gregor Wagner [:gwagner]]

Basic functionality.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.1 approval on this when you get a chance.

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8495569 [details] [review]
Github pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 2.1 feature work, unsure of exact bug.
[User impact] if declined: Some sites may not show icons when bookmarking.
[Testing completed]: Manual and integration tests.
[Risk to taking this patch] (and alternatives if risky): Fairly low risk. The fix is 5 lines and contained to a single file. Most of the patch is tests.
[String changes made]: No.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/1a013d7fc5336d80e9cc255004712d98d562b732

Comment 11

[Yeojin Chung [:YeojinC]]

This issue is verified on Flame 2.1 and Flame 2.2:

Flame 2.1 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.1
BuildID: 20141002000202
Gaia: 94dcc25f2e34a4900ea58310c26be52bcb089161
Gecko: baaa0c3ab8fd
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Flame 2.2 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20141001040205
Gaia: 0e280591881d44b80f456bc27e12d9114c218868
Gecko: 14665b1de5ee
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

The rocketship icon is displayed when BMW USA website is added to homescreen.

Comment 12

[Kevin Grandon :kgrandon]

An integration test was written with the patch.