1377295 - Migrate or re-use pinned sites from old top-sites panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Comment 1

[Chenxia Liu [:liuche]]

Our old Top Sites pinned sites had an index for being pinned, but the new AS Top Sites only has a boolean (isPinned), and is stored separately. Talked to ahunt, looks like switching to just a boolean was a UX decision, so I'm migrating the old pinned sites forward to the AS Top Sites with a DB upgrade. I don't think it's worthwhile continuing to use the old pinned sites and tying ourselves to a split system.

In order to not lose any sites after the migration, we'll also track sites added through old-TS, but won't do "backward compatibility" for sites pinned through AS.

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Bug 1377295 - Use old Top Sites pinned sites in AS panel.

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

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8888050 [details]
Bug 1377295 - Use old Top Sites pinned sites in AS panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158940/diff/1-2/

Comment 4

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

Comment on attachment 8888050 [details]
Bug 1377295 - Use old Top Sites pinned sites in AS panel.

https://reviewboard.mozilla.org/r/158940/#review164386

liuche said there was another patch coming up but this one looks good. A few notes:
* I had a question so here's the answer for future travellers: despite having a constant position, AS sites will always appear in the same order because we order by (postition, url).
* I haven't thought too hard about it because it'd only affect our small testing audience for in-between release builds and I'm okay with the behavior, but if a user has pinned a top site in old Top Sites & the same one in Activity Stream, we may get some weird behavior.

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8888050 [details]
Bug 1377295 - Use old Top Sites pinned sites in AS panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158940/diff/2-3/

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8888050 [details]
Bug 1377295 - Use old Top Sites pinned sites in AS panel.

One line change - added handling for user-entered content.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8888050 [details]
Bug 1377295 - Use old Top Sites pinned sites in AS panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158940/diff/3-4/

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8888050 [details]
Bug 1377295 - Use old Top Sites pinned sites in AS panel.

mcomella requested I revert the user-entered: change because it's not really related to pinned sites - I filed bug 1382489, and carried over the r+ from before.

Comment 9

[Chenxia Liu [:liuche]]

> * I haven't thought too hard about it because it'd only affect our small
> testing audience for in-between release builds and I'm okay with the
> behavior, but if a user has pinned a top site in old Top Sites & the same
> one in Activity Stream, we may get some weird behavior.

This would actually be fine - if a user unpins and then repins a site in AS, the site will be bumped to the front, per expected AS-TS behavior. If they go back to old Top Sites (which would only be for people who are using this as an experiment and can actually switch back), the pin would disappear from old Top Sites because the position would become -1, but this would only affect a small number of Nightly users, so I'm not going to worry about that case.

The behavior for the migrated pinned sites won't be exactly the same as the AS pinned sites - they'll keep their position, so they won't be at the front the way AS-TS top sites are. I think that's fine for this case of migrating top sites, because those pins will still be visible in the top 6 AS-TS tiles.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6503ba3ed9a4
Use old Top Sites pinned sites in AS panel. r=mcomella

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6503ba3ed9a4