1320806 - Allow removing Top Sites items without removing underlying history item

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Activity Stream, defect, P5)

Description

[:Grisha Kruglov]

We should consider removing Top Sites items separately from removing history.

Currently the only way to remove something from Top Sites is to remove a corresponding history item, which seems a bit drastic. As a user, I still want to see a particular page in my awesomebar results and in my history panel, but I might not want to see it show up in Top Sites.

We already have a bug to store metadata about a top site ("display name") - Bug 1314732, and highlights support a blocklist (Bug 1314718), giving us a couple of avenues to store this additional metadata.

Comment 1

[:Grisha Kruglov]

There would be no way to sync this "blocklist" information across different AS clients. But that's the same problem we have for Highlights blocklist, it's local to a particular device.

Comment 2

[:Grisha Kruglov]

After looking at the context menu usage data from home panels (see https://bugzilla.mozilla.org/show_bug.cgi?id=1320798#c3), it looks like one of the actions people take fairly often is removing things.

It feels to me that we should just have one "removal" action for both top sites and highlights - "Dismiss", or whatever else it might be called - and that action shouldn't perform anything non-obvious or destructive (like removing things from history). Currently we have "Dismiss" and "Delete from History", both of which remove an item from the AS screen, but one also has a side-effect, and the other's meaning isn't necessarily obvious.

This also lets us free up a slot in the context menu, as a side bonus.

Comment 3

[:Grisha Kruglov]

Implementation-wise, we could follow the highlights blocklist route (Bug 1298783) and implement a separate "top sites blocklist" and duplicate bunch of logic, or we could consolidate the two into one table, "activity stream blocklist", and add a "type" key to differentiate. Either way we're going to have to run a migration, and the latter approach seems simpler and easier to maintain.

Comment 4

[Barbara Bermes [:barbara] - NI please!]

> Currently the only way to remove something from Top Sites is to remove a corresponding history item, which seems a bit drastic.

Grisha, are you saying this is a technical limitation or a UX limitation?

Highlights and Top Sites both should have the same options, however, right now, Topsites only shows "Delete from history" and Highlights also has a "Dismiss" item.

In addition, I would not like to see us offering different functionality/UX for Mobile vs. Desktop. Unless Desktop is also considering consolidating these two actions, I think we *need* to keep them.

Proposal for Highlights and Top Sites, if technical possible:
a) "Delete from history" (will remove item from history on all synced devices, if possible?)
b) "Dismiss" (won't remove it from history)

I wonder if we have any data/feedback from users on these two items on AS Desktop? Heather, Brian, would you know?

Comment 5

[Heather McGaw]

Hi Barbara and Grisha, 

We've conducted research on whether people understand the difference between "Dismiss" and "Delete from History" on Activity Stream, and the distinction between those two actions was clear to participants.

Comment 6

[Andrzej Hunt :ahunt]

(In reply to :Grisha Kruglov from comment #3)
> Implementation-wise, we could follow the highlights blocklist route (Bug
> 1298783) and implement a separate "top sites blocklist" and duplicate bunch
> of logic, or we could consolidate the two into one table, "activity stream
> blocklist", and add a "type" key to differentiate. Either way we're going to
> have to run a migration, and the latter approach seems simpler and easier to
> maintain.

Topsites and highlights are mutually exclusive though (except perhaps for the first few days that a new user is using firefox): topsites will have many visits, and we only show highlights for pages with less than 3 visits.

Which means that we could in theory reuse the same list without a "type" column. That does however seem quite hacky, I'd prefer the type-column approach myself.

Comment 7

[Andrzej Hunt :ahunt]

Attachment: WIP Bug 1320806 - Add type column to ActivityStream blocklist table

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

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8828281 [details]
WIP Bug 1320806 - Add type column to ActivityStream blocklist table

This is still an untested WIP - I was wondering if you had any thoughts on trying a different upgrade process - i.e. using the iOS upgrade methodology in future?

I.e. on iOS upgrades are done per table, instead of our DB schema version based iterative process, which seems to simplify upgrades:

Android version:
for (schemaVersions : oldVersion..newVersion) {
    case N:
        createTableFoo();
        break;
    case N+X:
        addSomethingToTableFoo1(); // this might iterate over the whole table
        break;
    case N+X+Y:
        addSomethingToTableFoo2(); // same here
        break;
}

If we do this, we either need to freeze createTableFoo(), and add createTableFooN+X() which is used for new databases being created at version N+X (and similarly for N+X+Y). This approach is wasteful since we might have to iterate over the data in that table multiple times, if we run every single intermediate migration.

Or we can make addSomethingToTableFoo1() aware of the version of the table created in createTableFoo(), so that it isn't run if we've already created tableFoo with an up to date schema containing whatever data that migration would add (e.g. if we upgrade from pre-N to N+X+). Which can be tricky to manage correctly, since you need to keep each permutation in mind - and you also need to modify old migrations to handle multiple versions of input-data.

I.e. you have the choice of wasteful multiple iterations, or complicated migration code.

We can already see this mess with variables such as "didCreateCurrentReadingListTable" and "didCreateTabsTable". The domains table migration also has to handle this.

The iOS approach is to instead have one migration path for each input "version", i.e.:

if (inputVersion < N) {
    // We know the table doesn't exist yet, so we just create the latest schema
    createTableFoo();
} else if (inputVersion >= N && inputVersion < N+X) {
    // We know that the table exists, but hasn't had any of the new migrations
    // performed
    upgradeTableFooFromOriginalVersion();
} else if (inputVersion < N+X+Y) {
    upgradeTableFooFromV2();
}
// etc.

In this approach, every schema change means that you have to modify all the existing migration methods to correctly output the latest schema - but the advantage is that the input data format for a given migration method stays constant (which would seem to result in simpler migration code).

Do you think it's worth adding this migration style for new tables (see patch), or do you think we should stick with what we have? (It will be slightly confusing to have two different migration methodologies in the codebase, but we could conceivably move old migrations over as and when that's needed.)

Comment 9

[Andrzej Hunt :ahunt]

Note to self: don't forget to measure the performance costs of adding another subquery to the topsites query.

Comment 10

[:Grisha Kruglov]

(In reply to Andrzej Hunt :ahunt from comment #9)
> Note to self: don't forget to measure the performance costs of adding
> another subquery to the topsites query.

And keep an eye out on the histograms for perf regressions in the wild. Thankfully these queries are instrumented.

Comment 11

[:Grisha Kruglov]

My apologies in advance, I feel like this is a case of "if I had more time, I would have written a shorter letter"...

TLDR: let's generally stick with what we have, but do it better.

To help me understand two approaches to migrations better, I'll step back a little.

Consider overall database schema state S at time "t". Let's call it St (where "t" is our current "db version"). There are two ways to obtain it:
1) define St outright. This represents a "create fresh database" step.
2) derive St from previous state Sn. That is, St = Sn + M(n+1) + M(n+2) + ... + Mt. This represents "starting at Sn, run (t-n) migrations". Migration could be thought of as a "db state transition".
3) you can also define a set of direct transitions from any Sn to "current" state St, and evolve this set over time. E.g. for S3, you'll have three transitions: S0->3, S1->3, S2->3.

You can think of each database state S as a composition of states of tables. E.g. Sn = VisitsN + BookmarksN + ... + HistoryN.
Each migration is then a composition of individual table migrations, or "table state transitions".
E.g.: VisitsT = VisitsN + VisitsMigration(n+1) + VisitsMigration(n+2) + ... + VisitsMigration(t).

On one hand, iOS migration system recognizes this hierarchy, and makes it explicit by looking at database transitions on a more granular level of table transitions. Table versions are explicitly defined, and an overall database version on iOS is implied. Table state transitions are defined and run. Final state is a result of running all of the necessary table transitions. e.g. S = T1v1 + T1v2 + T2v1 + T3v1 + T3v2.

However, we're missing something here: we're dealing with a relational database, so tables are interconnected. If you're iOS migration code, you find yourself in a situation that nicely broken down individual table migrations are intertwined. Schema changes and data migrations for table X might need to happen in context of what happens to tables Y and Z. And then you end up with something like BrowserTable.swift, a "table class" which "owns" multiple "db tables" to make this possible, which is more or less the same mess that you see on Android: https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/SQL/BrowserTable.swift

I'm not familiar with how iOS migration code developed over time, but looking at it now it seems that it's coming full circle to what inspired it to change in the first place (Android migration code). After all, it did start off with a nice, generic BrowserDB.swift: https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/SQL/BrowserDB.swift#L96

Android migration code explicitly defines a database version, and the table versions are implied. E.g. at db version 24, our tabs table is at version 1. Database state transitions are defined and run. Each database transition might affect any one table's state, incrementing that table's implied version. This forces you to pay more attention, but it also allows you to move forward multiple interconnected tables at the same time.

Now, messy things such as "didCreateTabsTable" flags can and should be avoided. Let's think about it more. It's used in 24->25 migration, and you can avoid using it like this:
updateFrom24To25():
 if (originalVersion == 24) {
  // fix up foreign key constraints, we screwed it up in 23
 } else {
  // originalVersion must be < 24, that is, table was just created in 23 with a correct, "current", definition. So we have nothing to fix here, skip this migration.
 }

Amusingly enough, you will see the same pattern appear in iOS migration code: https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/SQL/BrowserTable.swift#L777 -> e.g. migration 14->15 _only_ runs to fix up a screw up in migration 13, but otherwise it won't run since the views are generally created using a correct, "current", definition.

However, we can't quite do this on Android currently without changing migration code a little bit, since the "original" db version is not exposed to individual migrations, but often that is what they really care about. In case of a "didCreateTabsTable" flag, it's just a clumsy way to peek at the original db version.

So perhaps this could be a good small actionable item to improve our migrations: exposing the "original db version", so that migrations have more context about what's going on and don't need to rely on annoying bespoke flags.

Comment 12

[:Grisha Kruglov]

Comment on attachment 8828281 [details]
WIP Bug 1320806 - Add type column to ActivityStream blocklist table

Clearing the (very) old flag for now. This functionality won't be part of the MVP.

Comment 13

[bbell]

> This also lets us free up a slot in the context menu, as a side bonus.

There's a compelling use case for removing a Top-Site and or a Highlight without expelling all reference to it from a users history.  The case for this becomes stronger in the future as we add new capabilities to the New Tab Page like Reminders, Pin to New Tab, and Pin Site as a Top Site. Users will grow to use this page as a flexible and semi permanent clipboard which will require an option to unclip items from the New-Tab. If unclipping items also meant removing all history of this page ever being loaded, that would diminish the Awesome Bar™, and the ability to get back to something they already signaled to us as being important to them.

Comment 14

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

All open Activity Stream bugs are moving from the whiteboard tag, "[mobileAS]", to the Firefox for Android component, "Activity Stream", so that I can keep better track of these bugs as the new triage owner; I will send out an email shortly with additional details, caveats, etc.

Comment 16

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 17

[Firefox Bug Husbandry Bot]

Reverting spam.

Comment 18

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.