1278644 - Adapters behind History and TopSites home panels need stable getItemId implementations

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Grisha Kruglov]

Similarly to Bug 1270068, icon/thumbs for websites blink multiple times when website is opened in a new tab.

Comment 1

[:Grisha Kruglov]

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).

Comment 2

[:Margaret Leibovic]

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?

Comment 3

[Daniel Lim [:dlim]]

(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?

Comment 4

[:Grisha Kruglov]

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.

Comment 5

[:Grisha Kruglov]

Attachment: Bug 1278644 - Stable getItemId implementation for CombinedHistoryAdapter

Review commit: https://reviewboard.mozilla.org/r/61130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61130/

Comment 6

[:Grisha Kruglov]

Attachment: Bug 1278644 - Stable getItemId implementation for VisitedAdapter

Review commit: https://reviewboard.mozilla.org/r/61132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61132/

Comment 7

[:Grisha Kruglov]

Attachment: Bug 1278644 - Stable getItemID implementation for ClientsAdapter

Review commit: https://reviewboard.mozilla.org/r/61134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61134/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8766110 [details]
Bug 1278644 - Stable getItemId implementation for CombinedHistoryAdapter

https://reviewboard.mozilla.org/r/61130/#review58380

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8766111 [details]
Bug 1278644 - Stable getItemId implementation for VisitedAdapter

https://reviewboard.mozilla.org/r/61132/#review58382

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8766112 [details]
Bug 1278644 - Stable getItemID implementation for ClientsAdapter

https://reviewboard.mozilla.org/r/61134/#review58384

Comment 11

[:Grisha Kruglov]

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

Comment 12

[:Grisha Kruglov]

Note to self - this probably needs to be uplifted to 49.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/304c5fbd7161
https://hg.mozilla.org/mozilla-central/rev/9e1003b7d84f
https://hg.mozilla.org/mozilla-central/rev/24f4b4e4e22d

Comment 14

[:Grisha Kruglov]

Attachment: stable-ids-for-home-panels.patch

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

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ff2ed0592d8

Comment 17

[Carsten Book [:Tomcat]]

sorry backed out for bustage https://treeherder.mozilla.org/logviewer.html#?job_id=3008934&repo=mozilla-aurora

Comment 18

[:Grisha Kruglov]

Specific failure: https://treeherder.mozilla.org/logviewer.html#?job_id=3008934&repo=mozilla-aurora#L21417

Comment 19

[:Grisha Kruglov]

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!

Comment 20

[Carsten Book [:Tomcat]]

(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.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

I need more information to understand the situation. Could you explain a bit more? Thanks

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 24

[Carsten Book [:Tomcat]]

(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

Comment 25

[:Grisha Kruglov]

Attachment: stable-ids-aurora.patch

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.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Please fill the uplift request if needed!

Comment 27

[:Grisha Kruglov]

Attachment: stable-ids-aurora.patch

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

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8774471 [details] [diff] [review]
stable-ids-aurora.patch

Let's take it in aurora, thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e3d202c9c230
https://hg.mozilla.org/releases/mozilla-aurora/rev/db67e1188e8d