919234 - Regression: Unpinning a site flashes thumbnail into neighbour thumbnail spot

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently when one unpins a site, the thumbnail will flash into it's neighbour spot. See video: http://www.youtube.com/watch?v=NqQqztgVjsM

--
Nightly (09/21)
LG Nexus 4 (Android 4.3)

Comment 1

[Aaron Train [:aaronmt]]

Regression from new-new-about-home

hg.mozilla.org/mozilla-central/pushloghtml?fromdate=2013-09-20&todate=2013-09-21

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-09-20&enddate=2013-09-21

Regression window link correction for future travelers:

https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-09-20&enddate=2013-09-21

Our most likely culprit:

https://hg.mozilla.org/mozilla-central/rev/2839a4856902

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> Our most likely culprit:

I lied, as Aaron mentioned comment 1, this is the whole new-new-about-home change so something was probably lost in translation (e.g. [1]).

[1]: https://hg.mozilla.org/mozilla-central/rev/a7921e0be999

Comment 4

[Aaron Train [:aaronmt]]

Yeah not sure of the exact changeset, but it was introduced during the work week.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #4)
> Yeah not sure of the exact changeset, but it was introduced during the work
> week.

This probably related to the fact that we refresh the list of topsites when you unpin a site. But the refresh is supposed to happen in one go. The glitch seems to be caused by some sort of intermediary state before the grid finally settles on the new list of sites.

Comment 6

[Wesley Johnston (:wesj)]

I wonder if we could use a hack here :) Pull the items that would be in the grid out of the cursor into an Array and have the Adapter look in that array for these items instead of in the cursor.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Do not restart Loader when thumbnails load.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 818168 [details] [diff] [review]
Part 1: Do not restart Loader when thumbnails load.

This solves the issue that the comment warns about and seems to be the appropriate way to use the loader.

I'm going to try to audit the Adapter and ContentResolver code a bit more, but if you don't hear from me, assume it's all good.

Feel free to defer this review if you want to lower your queue or know anyone better to do it.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 818168 [details] [diff] [review]
Part 1: Do not restart Loader when thumbnails load.

Nevermind - I just realized that only the favicon thumbnails were drawing - not the screenshots.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This seems hard to repro as of 10/17's Nightly - I believe it is due to the patch in bug 926574.

On a Galaxy Nexus (4.2.2) and a Galaxy Chat (4.0.4), I can't see any flicker. On a Motorola Droid 2 (2.3.4), I see a very fast flicker (seemingly negligible though to be fair, it's a clean profile). I wonder if the issue is just related to the Android version now.

I think it's a good idea to leave this bug open for a week or so to see if the issue returns and if not, WONTFIX it. What do you think, Aaron?

Comment 11

[Aaron Train [:aaronmt]]

I still see this very badly on Aurora but not Nightly, so something needs uplifting to Aurora (Fx26). If it's that bug you mentioned, I would vouch for that. Happy to close this once I can verify that it's fixed on Aurora.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I locally backed out the patch in bug 926574 and can confirm it is what caused the changes.

Comment 13

[Aaron Train [:aaronmt]]

verifyme on aurora and then close this out

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Works for me on the latest Aurora (10/23).

Comment 15

[Lucas Rocha (:lucasr)]

Closing.

Comment 16

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 26.0a2 (2013-10-27) and Firefox for Android 27.0a1 (2013-10-28)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1