939060 - Incorrect thumbnail is sometimes displayed on the last empty grid position

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Flaviu Cos]

Attachment: The file attached is a screen capture

Environment:
Build: Nightly 28.0a1 2013-11-14;
Device: HTC Desire(Android 2.2).

Steps to reproduce:
1. Start with a clean profile or create one if needed;
2. Start the app.

Expected results:
In the Top Sites grid only the first 2 positions are displayed.

Actual results:
The last grid position which is empty display the thumbnail for firefox support.

Notes:
Please check the screen capture attached.

Comment 1

[Richard Newman [:rnewman]]

This is a follow-on from the partial solution in Bug 926430. There's still some kind of racing view recycling going on.

My hope is that this simply disappears as we make improvements to the thumbnail and favicon loading code -- e.g., see Bug 938141 Comment 4. 

Tweaking the subject to indicate that (a) on some devices/versions this isn't readily reproducible, and (b) it's only Firefox Support because that's one of our pre-loaded bookmarks.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Check URL associated with grid item before displaying favicon (r=rnewman)

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8334494 [details] [diff] [review]
Check URL associated with grid item before displaying favicon (r=rnewman)

This is likely caused by the fact that we don't check if the target grid item has been bound to another URL before displaying the favicon.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8334494 [details] [diff] [review]
Check URL associated with grid item before displaying favicon (r=rnewman)

Review of attachment 8334494 [details] [diff] [review]:
-----------------------------------------------------------------

The loadID check is supposed to avoid the need for this kind of thing, but I suppose there is a window in which this could occur, which would explain the bug.

This looks like a mirror of Bug 920331, so if this fixes the problem we should make a note in that bug that there are two places to change.

Time to test this patch on my repro-ing device…

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8334494 [details] [diff] [review]
Check URL associated with grid item before displaying favicon (r=rnewman)

This looks good to me. I'll take care of landing this later today, 'cos lucasr should be asleep :D

Comment 6

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/faaaafbf6626

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8334494 [details] [diff] [review]
Check URL associated with grid item before displaying favicon (r=rnewman)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Old stuff; partial fix in Bug 926430. This completes it.
 
User impact if declined: 
  First-run user experience shows the wrong favicon in the Top Sites view on some devices. (Timing-related.)

Testing completed (on m-c, etc.): 
  Manually tested. Just about to land on m-c for more verification.

Risk to taking this patch (and alternatives if risky): 
  None.

String or IDL/UUID changes made by this patch:
  None.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/faaaafbf6626

Comment 9

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ce5d00f1aa8

Comment 10

[Flaviu Cos]

Verified as fixed in build:
- Aurora 27.0a2 (2013-11-24);
- Nightly 28.0a1 (2013-11-24);
Device: HTC Desire (Android 2.2)