1073728 - Add to homescreen uses the wrong icon

Status: VERIFIED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

STR:

Open the browser.
Load www.nytimes.com.
Before that finishes, go to the address bar and navigate to tantek.com instead.
Add to homescreen.

Actual results:

The offered icon is the New York Times Icon

Expected results

The offered icon is the same as the icon offered when launching tantek.com directly from the browser app.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

qawanted for branch checks

Comment 2

[Cody Roesch [:croesch]]

This bug repro's on Flame KK builds: Flame 2.2 KK, Flame 2.1 KK

Actual Results: Incorrect webpage icon is seen.

Repro Rate: 4/4

Environmental Variables:
Device: Flame Master
BuildID: 20140926042841
Gaia: 4e9aa22d15b77e5b3d7e763128ead2caffd3b801
Gecko: 68f76e3c5787
Version: 35.0a1 (Master) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20140926071340
Gaia: 9ebd12fb519dd675ff04f7c6298522a40d92e6d7
Gecko: 2b181f0523c8
Version: 34.0a2
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

-----------------------------------------------------------------
-----------------------------------------------------------------

This bug does NOT repro on Flame kk build: Flame 2.0 KK

Actual Result: Correct icon is seen when navigating to the second webpage and trying to add it to the homepage.

Repro Rate: 0/2

Environmental Variables:
Device: Flame 2.0 KK
BuildID: 20140926071941
Gaia: c1aa7829548e65360472c31544dbe2839eaf5be1
Gecko: 61d78a6855fb
Version: 32.0 (2.0) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 3

[Kevin Grandon :kgrandon]

Regarding regressionwindow: After looking at the code I have a hunch that this was caused by bug 968156. Finding a regression window might be a bit difficult from 2.0 -> 2.1 as we have a completely new browsing experience. 

I'm currently working on a fix, and if it turns out it's the right fix, we may not need the regression window.

Comment 4

[Kevin Grandon :kgrandon]

Attachment: Github pull request

WIP - working on tests.

Comment 5

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Now has a unit test.

Dale, Alive - Could you guys review this? This resets the tracked favicons in the appWindow, and also moves the icon fetching logic after the user presses the bookmark button as it's more relevant there, and gives icons more time to load.

Comment 6

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

Comment on attachment 8496372 [details]
Github pull request

I'd like to let Dale decide what to do here.

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 8496372 [details]
Github pull request

This looks good, I want to move the getIcon code into the bookmark app itself, however requires the shared helper (working on it now), missed the clearing favicons object though which was a bad miss.

Comment 8

[Kevin Grandon :kgrandon]

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

Comment 9

[Gregor Wagner [:gwagner]]

Regression

Comment 10

[Joshua Mitchell (Inactive)]

Issue is resolved - regression window not needed

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8496372 [details]
Github pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature work, most likely bug 968156.
[User impact] if declined: Incorrect icons when adding to home screen.
[Testing completed]: Manual and unit tests.
[Risk to taking this patch] (and alternatives if risky): Low risk, mostly moving some code and adding a one-liner to reset favicons.
[String changes made]: None.

Comment 12

[Gabriela [:gaby2300]]

I can repro this bug in my flatfish device using the latest good build (September 26).

STR

1) Type clarin.com in the rocketbar.
2) Add to homescreen.
3) Notice clarin's icon is added to the homescreen.
4) Type cnn.com in the rocketbar.
5) Add to homescreen.
6) Notice clarin's icon added instead of cnn's.

Actual result:
Clarin's icon is added instead of cnn's.

Expected result:
Cnn's icon should be added instead of clarin's.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Yeojin Chung [:YeojinC]]

This issue is verified on Flame 2.2 and Flame 2.1:

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

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

The icon is displayed properly when added to homescreen.