Closed Bug 1278644 Opened 4 years ago Closed 4 years ago

Adapters behind History and TopSites home panels need stable getItemId implementations

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 --- fixed
fennec + ---
firefox50 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

Details

Attachments

(4 files, 2 obsolete files)

Similarly to Bug 1270068, icon/thumbs for websites blink multiple times when website is opened in a new tab.
One workaround might be to implement getItemId() and force hasStableIds(true) for both CombinedHistoryAdapter and ClientsAdapter. After some quick tests, this seems to help with recycling views.

CombinedHistoryAdapter's getItemId is simple - we have either history_id or bookmark_id we can use. ClientsAdapter is a little trickier, since our underlying data there is a list of tuples (client_guid, tab_id).
tracking-fennec: --- → ?
tracking-fennec: ? → +
I'd really like to see this get fixed, this looks pretty janky. I also see this happen if your history is syncing while you're looking at the history panel.

Maybe another good bug for Daniel?
Summary: Website icons blink multiple times in the History panel when websites are opened in a new tab → Website icons blink multiple times in the History panel when data set updates
(In reply to :Margaret Leibovic from comment #2)
> I'd really like to see this get fixed, this looks pretty janky. I also see
> this happen if your history is syncing while you're looking at the history
> panel.
> 
> Maybe another good bug for Daniel?

Grisha, you mentioned this earlier, were you looking at this?
Flags: needinfo?(gkruglov)
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Flags: needinfo?(gkruglov)
VisitedAdapter in TopSitesPanel has a related problem (results in blinking of icons on the top sites panel) - it's relying on default getItemId implementation, which returns value of the _id column, which in case of top sites data is always 0 (since it's backed by Combined view, and that's the sort of thing Combined view does). Going to address it in this ticket as well.
Summary: Website icons blink multiple times in the History panel when data set updates → Adapters behind History and TopSites home panels need stable getItemId implementations
Attachment #8766110 - Flags: review?(s.kaspari) → review+
Comment on attachment 8766110 [details]
Bug 1278644 - Stable getItemId implementation for CombinedHistoryAdapter

https://reviewboard.mozilla.org/r/61130/#review58380
Comment on attachment 8766111 [details]
Bug 1278644 - Stable getItemId implementation for VisitedAdapter

https://reviewboard.mozilla.org/r/61132/#review58382
Attachment #8766111 - Flags: review?(s.kaspari) → review+
Comment on attachment 8766112 [details]
Bug 1278644 - Stable getItemID implementation for ClientsAdapter

https://reviewboard.mozilla.org/r/61134/#review58384
Attachment #8766112 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/integration/fx-team/rev/304c5fbd71618e5659d6d3b4c02537bca5d71625
Bug 1278644 - Stable getItemId implementation for CombinedHistoryAdapter r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/9e1003b7d84f0f83241d476e28fd8cfcdf43091f
Bug 1278644 - Stable getItemId implementation for VisitedAdapter r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/24f4b4e4e22dd5ab16c411133100b31afa1d0cb1
Bug 1278644 - Stable getItemID implementation for ClientsAdapter r=sebastian
Note to self - this probably needs to be uplifted to 49.
Flags: needinfo?(gkruglov)
https://hg.mozilla.org/mozilla-central/rev/304c5fbd7161
https://hg.mozilla.org/mozilla-central/rev/9e1003b7d84f
https://hg.mozilla.org/mozilla-central/rev/24f4b4e4e22d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Attached patch stable-ids-for-home-panels.patch (obsolete) — Splinter Review
Approval Request Comment
[Feature/regressing bug #]: Generated favicons were introduced in Bug 1228680, which landed in 49.

[User impact if declined]: When on Top Sites (interacting with items outside of the grid) or History panels (interacting with any history item), if user opens history a item in a background tab, they will see all visible generated favicons blink once or multiple times. 

[Describe test coverage new/current, TreeHerder]: Local testing.

[Risks and why]: Seems low risk; some logic was added to generate stable IDs for three adapters behind these home panels; at worse, we might end up recycling views incorrectly, but we were already doing that.

[String/UUID change made/needed]: none
Flags: needinfo?(gkruglov)
Attachment #8768677 - Flags: approval-mozilla-aurora?
Comment on attachment 8768677 [details] [diff] [review]
stable-ids-for-home-panels.patch

Fix for favicon blinking, regression from 49. Let's try it in aurora.
Attachment #8768677 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Seems that I've missed a dependency on Bug 1251362. Specifically, this bit of code https://hg.mozilla.org/mozilla-central/rev/0c997d222636#l1.106

It should be trivial to create a patch which works around this conflict. However, I'm not clear as to what happens when Bug 1251362 hits aurora - the code in question will need to be modified again (to its present state). Carsten, could you please clarify how to proceed in this case? Thanks!
Flags: needinfo?(gkruglov) → needinfo?(cbook)
(In reply to :Grisha Kruglov from comment #19)
> Seems that I've missed a dependency on Bug 1251362. Specifically, this bit
> of code https://hg.mozilla.org/mozilla-central/rev/0c997d222636#l1.106
> 
> It should be trivial to create a patch which works around this conflict.
> However, I'm not clear as to what happens when Bug 1251362 hits aurora - the
> code in question will need to be modified again (to its present state).
> Carsten, could you please clarify how to proceed in this case? Thanks!

Sylvestre can you take a look at this, seems this will cause problems when we uplift this bug here and we have the general merge day.
Flags: needinfo?(cbook) → needinfo?(sledru)
I need more information to understand the situation. Could you explain a bit more? Thanks
Flags: needinfo?(sledru)
11:46 <~Tomcat|sheriffduty> Sylvestre: so for bug 1278644 i think the problem is that the developer can fix his bustage, but when this bug is uplifted to aurora and central moves to aurora this will cause again a bustage and would need a new fix again

thats at least how i understand comment #19
Flags: needinfo?(sledru)
(In reply to :Grisha Kruglov from comment #19)
> Seems that I've missed a dependency on Bug 1251362. Specifically, this bit
> of code https://hg.mozilla.org/mozilla-central/rev/0c997d222636#l1.106
> 
> It should be trivial to create a patch which works around this conflict.
> However, I'm not clear as to what happens when Bug 1251362 hits aurora - the
> code in question will need to be modified again (to its present state).
> Carsten, could you please clarify how to proceed in this case? Thanks!

But bug 1251362 is not going to get uplifted, so what's the problem?

On merge day the state of mozilla-central is becoming "Aurora" and this state already includes your changes as well as the ones from bug 1251362.
Whatever you uplift to aurora now will then be beta and is not affected by this.
(In reply to Sebastian Kaspari (:sebastian) from comment #23)
> (In reply to :Grisha Kruglov from comment #19)
> > Seems that I've missed a dependency on Bug 1251362. Specifically, this bit
> > of code https://hg.mozilla.org/mozilla-central/rev/0c997d222636#l1.106
> > 
> > It should be trivial to create a patch which works around this conflict.
> > However, I'm not clear as to what happens when Bug 1251362 hits aurora - the
> > code in question will need to be modified again (to its present state).
> > Carsten, could you please clarify how to proceed in this case? Thanks!
> 
> But bug 1251362 is not going to get uplifted, so what's the problem?
> 
> On merge day the state of mozilla-central is becoming "Aurora" and this
> state already includes your changes as well as the ones from bug 1251362.
> Whatever you uplift to aurora now will then be beta and is not affected by
> this.

yeah than i guess its ok to uplift, we would need however a new patch because of the first one with the bustage
Attached patch stable-ids-aurora.patch (obsolete) — Splinter Review
Ah, my mental model of how release channels & uplifts worked was slightly off, hence my confusion.

Attached is an updated patch, which removes mention of RECENT_TABS (dependency on Bug 1251362) and builds on top of current aurora.
Attachment #8768677 - Attachment is obsolete: true
Please fill the uplift request if needed!
Flags: needinfo?(sledru)
Updated uplift patch with merge conflicts resolved.

[Feature/regressing bug #]: Generated favicons were introduced in Bug 1228680, which landed in 49.

[User impact if declined]: When on Top Sites (interacting with items outside of the grid) or History panels (interacting with any history item), if user opens history a item in a background tab, they will see all visible generated favicons blink once or multiple times. 

[Describe test coverage new/current, TreeHerder]: Local testing.

[Risks and why]: Seems low risk; some logic was added to generate stable IDs for three adapters behind these home panels; at worse, we might end up recycling views incorrectly, but we were already doing that.

[String/UUID change made/needed]: none
Attachment #8774385 - Attachment is obsolete: true
Attachment #8774471 - Flags: approval-mozilla-aurora?
Comment on attachment 8774471 [details] [diff] [review]
stable-ids-aurora.patch

Let's take it in aurora, thanks!
Attachment #8774471 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1296411
You need to log in before you can comment on or make changes to this bug.