1293710 - Obtain a list of "highlights" and display them in the AS panel

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

We want to show "highlights" in the ActivityStream panel.

That's what the desktop add-on does:
https://github.com/mozilla/activity-stream/blob/master/lib/PlacesProvider.js#L590

Let's try to copy that behavior or do something very similar. This bug is more about getting and displaying the data and not about the actual design of the highlights/panel.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Oops, accidentally linked iOS bug.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Updated link:
https://github.com/mozilla/activity-stream/blob/9eb9f451b553bb62ae9b8d6b41a8ef94a2e020ea/addon/PlacesProvider.js#L578

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

https://github.com/mozilla/activity-stream/blob/e975875984c5b3e883584e163d3819e7f020128d/common/constants.js#L14
>  // Thresholds for highlights query
>  HIGHLIGHTS_THRESHOLDS: {
>    created: "-3 day",
>    visited: "-30 minutes"
>},

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

https://github.com/mozilla/activity-stream/blob/e975875984c5b3e883584e163d3819e7f020128d/common/constants.js#L3
> LINKS_QUERY_LIMIT: 20,

https://github.com/mozilla/activity-stream/blob/9eb9f451b553bb62ae9b8d6b41a8ef94a2e020ea/addon/PlacesProvider.js#L37
> const REV_HOST_BLACKLIST = [
>   "moc.elgoog.www.",
>   "ac.elgoog.www.",
>   "moc.elgoog.radnelac.",
>   "moc.elgoog.liam.",
>   "moc.oohay.liam.",
>   "moc.oohay.hcraes.",
>   "tsohlacol.",
>   "oc.t.",
>   "."
> ].map(item => `'${item}'`);

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

In pseudo SQL code with some constants resolved the query is:

> SELECT DISTINCT FROM:
> 
> SELECT
>   bookmarks
>  WHERE
>   created in the last 3 days
>   AND not in block list
>   AND not visited more than 3 times
>   ORDER BY date added (desc)
>   LIMIT 1
> 
> UNION ALL
> 
> SELECT
>   history
> WHERE
>   not visited in the last 30 minutes
>   AND not in block list
>   AND not visited more than 3 times
>   AND has title
>   AND host not in blacklist
>   GROUP by host
>   ORDER BY date visited (desc)
>   LIMIT 19

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

I have a first basic version of the query.

Only thing I can't add easily is a host blacklist based on the 'rev_host' field of the places db (host of URL reversed) and a grouping of URLs based on 'rev_host' as well.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

This query resembles how the Activity Stream add-on for desktop Firefox selectes highlights[1].
The desktop version uses the 'rev_host' (reversed host) column to filter the results based
on a blacklist and group results by hostname. This has been omitted here (See bug 1298785 and
bug 1298786).

Additionally the desktop version uses a block list to filter "highlights" that have been removed
manually by the user. This is not added here until needed from the UI side (see bug 1298783).

[1] https://github.com/mozilla/activity-stream/blob/9eb9f451b553bb62ae9b8d6b41a8ef94a2e020ea/addon/PlacesProvider.js#L578

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

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

https://reviewboard.mozilla.org/r/74968/#review72946

I was wondering if it would be easier to build this query against the Combined view (which would avoid the duplicates issue) - but I realised Combined doesn't include bookmark creation time which is needed in this query.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDB.java:189
(Diff revision 1)
> +     * Obtain a set of links for highlights from bookmarks and history.
> +     *
> +     * @param cr The content resolver to use.
> +     * @param limit Maximum number of results to return.
> +     */
> +    Cursor getHighlights(ContentResolver cr, int limit);

Could we use the new CursorLoader pattern here? (See Bug 1239491 for background)

I.e. BrowserDB/LocalBrowserDB can return a CursorLoader instead of a Cursor (instead of "return cr.query(uri, ...)", use "return new CursorLoader(cr, uri, ...)"). This then means we can avoid implementing a new "HighlightsCursorLoader extends SimpleCursorLoader" (deprecated).

I've got an example of this in:
https://reviewboard.mozilla.org/r/70500/diff/4#index_header

It might be easier to do this as a followup bug, and make the changes once Bug 1293790 lands, which would also make it easier to add telemetry for highlights?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1102
(Diff revision 1)
> +        final long bookmarkLimit = 1;
> +
> +        // Select recent bookmarks that have not been visited much
> +        final String bookmarksQuery = "SELECT * FROM (SELECT " +
> +                DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + ", " +
> +                "0 AS " + Combined.HISTORY_ID + ", " +

The current convention seems to be to use "-1" as the history_id for pages that have no history, that might be a better choice here too?

E.g. for the "combined" view:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java#322

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1121
(Diff revision 1)
> +        final long historyLimit = totalLimit - bookmarkLimit;
> +
> +        // Select recent history that has not been visited much.
> +        final String historyQuery = "SELECT * FROM (SELECT " +
> +                History._ID + " AS " + Combined.HISTORY_ID + ", " +
> +                "0 AS " + Combined.BOOKMARK_ID + ", " +

Similary, I feel like using "0" as the bookmark_id could be confusing (I believe 0 is a valid bookmark ID), the Combined table uses "null" for history items that aren't bookmarked, although it might be worth changing the convention since we're implementing new views to handle this data - i.e. the "combined" table seems to be coupled to TwoLinePageRow:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java#586

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1134
(Diff revision 1)
> +                // TODO: Implement domain black list (bug 1298786)
> +                // TODO: Group by host (bug 1298785)
> +                "ORDER BY " + History.DATE_LAST_VISITED + " DESC " +
> +                "LIMIT " + historyLimit + ")";
> +
> +        final String query = "SELECT DISTINCT * FROM (" + bookmarksQuery + " UNION ALL " + historyQuery + ");";

I'm worried we might get duplicates this way, especially because we're generating fake history_id's for bookmarks (that might have matching history), and fake bookmark_id's for history that might be bookmarked - this means the same URL might not be distinct because of the differing ID's:

Maybe we can get around this with one of the following options:

Option A - simpler to implement?)
- Exclude bookmarks from historyQuery, or history from bookmarksQuery (excluding history from bookmarks is more likely to substantially affect the ordering of the final Cursor?)

Option B  - seems complex) 
- We already have "Select distinct", so we just need to ensure identical URL's receive identical ID's by:
- Ensuring we use the real history_id if available in bookmarksQuery (I think the combined view query in BrowserDatabaseHelper has some conditionals that would help with this)
- Ensure we use the real bookmark_id if available in historyQuery (would require a join against bookmarks?).

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1149
(Diff revision 1)
>          final int match = URI_MATCHER.match(uri);
>  
>          if (match == TOPSITES) {
>              return getTopSites(uri);
> +        } else if (match == HIGHLIGHTS) {
> +            return getHighlights(uri);

Could we move HIGHLIGHTS down to the switch below (where BOOKMARKS_FOLDER_ID, BOOKMARKS_ID, etc.) are located?

We only have TOPSITES here because the topsites query needs a writable DB (whereas all the other queries use a readable DB, which is obtained just below with getReadableDatabase()).

I've filed Bug 1298968 to add a comment explaining this!

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #8)
> I was wondering if it would be easier to build this query against the
> Combined view (which would avoid the duplicates issue) - but I realised
> Combined doesn't include bookmark creation time which is needed in this
> query.

Yep, that's right. I wanted to use the combined view too, but not all columns are available there.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

https://reviewboard.mozilla.org/r/74968/#review72946

> I'm worried we might get duplicates this way, especially because we're generating fake history_id's for bookmarks (that might have matching history), and fake bookmark_id's for history that might be bookmarked - this means the same URL might not be distinct because of the differing ID's:
> 
> Maybe we can get around this with one of the following options:
> 
> Option A - simpler to implement?)
> - Exclude bookmarks from historyQuery, or history from bookmarksQuery (excluding history from bookmarks is more likely to substantially affect the ordering of the final Cursor?)
> 
> Option B  - seems complex) 
> - We already have "Select distinct", so we just need to ensure identical URL's receive identical ID's by:
> - Ensuring we use the real history_id if available in bookmarksQuery (I think the combined view query in BrowserDatabaseHelper has some conditionals that would help with this)
> - Ensure we use the real bookmark_id if available in historyQuery (would require a join against bookmarks?).

Oh yeah, the bookmark and history IDs was one of the last things I added because the UI side might need it (to display whether the result comes frm bookmarks or the history).

I wonder if instead of "SELECT DISTINCT *" I could just use "GROUP BY url". I guess it is time to write some unit tests.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

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

Comment 12

[:Grisha Kruglov]

Are you going to also implement the new hotness that is weighted highlights?

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Grisha Kruglov from comment #12)
> Are you going to also implement the new hotness that is weighted highlights?

Yeah, but not as part of this bug. :)

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1293710 - Display highlights in activity stream panel.

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

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

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

Comment 16

[:Grisha Kruglov]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

https://reviewboard.mozilla.org/r/74968/#review73668

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1155
(Diff revision 3)
> +     * https://github.com/mozilla/activity-stream/blob/9eb9f451b553bb62ae9b8d6b41a8ef94a2e020ea/addon/PlacesProvider.js#L578
> +     */
> +    public Cursor getHighlights(final SQLiteDatabase db, String limit) {
> +        final int totalLimit = limit == null ? 20 : Integer.parseInt(limit);
> +
> +        final long threeDaysAgo = System.currentTimeMillis() - (1000 * 60 * 60 * 24 * 3);

There's TimeUnit, if you care to use it - although these numbers are pretty expressive by themselves https://developer.android.com/reference/java/util/concurrent/TimeUnit.html

`TimeUnit.MILLISECONDS.convert(3L, TimeUnit.DAYS)`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1159
(Diff revision 3)
> +
> +        final long threeDaysAgo = System.currentTimeMillis() - (1000 * 60 * 60 * 24 * 3);
> +        final long bookmarkLimit = 1;
> +
> +        // Select recent bookmarks that have not been visited much
> +        final String bookmarksQuery = "SELECT * FROM (SELECT " +

Is it necessary to wrap queries in a SELECT * FROM (...)?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1169
(Diff revision 3)
> +                "FROM " + Bookmarks.TABLE_NAME + " " +
> +                "LEFT JOIN " + History.TABLE_NAME + " ON " +
> +                    DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.URL) + " = " +
> +                    DBUtils.qualifyColumn(History.TABLE_NAME, History.URL) + " " +
> +                "WHERE " + DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.DATE_CREATED) + " > " + threeDaysAgo + " " +
> +                "AND " + DBUtils.qualifyColumn(History.TABLE_NAME, History.VISITS) + " <= 3 " +

Might want to consider doing something around local/remote visits here (in a separate ticket). I'm not quite sure what it'll look like from either technical POV nor from the product POV:
- are highlights just based on per-device browsing interesting? more so than "mixed" highlights?
- could we have separate rows for local vs remote highlights?

We do have (some) data available to play with these ideas: LOCLA_VISITS, REMOTE_VISITS, as well as LOCAL_DATE_LAST_VISITED and REMOTE_DATE_LAST_VISITED aggregate fields on History.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1293710 - Activity Stream Highlights: Load icons and restructured layout.

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

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

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

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8786859 [details]
Bug 1293710 - Display highlights in activity stream panel.

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

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1293710 - Activity Stream Highlights: Consider bookmarks without history too.

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

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1293710 - Activity Stream Highlights: Only select actual bookmarks (no folders and other special types).

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

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8786859 [details]
Bug 1293710 - Display highlights in activity stream panel.

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

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787597 [details]
Bug 1293710 - Activity Stream Highlights: Load icons and restructured layout.

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

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1293710 - Group activity stream highlights by URL to avoid duplicates.

The same URL could be picked as highlight from history and from bookmarks.

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

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1293710 - Add unit tests for highlights query.

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

Comment 26

[Andrzej Hunt :ahunt]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

https://reviewboard.mozilla.org/r/74968/#review74660

Comment 27

[Andrzej Hunt :ahunt]

Comment on attachment 8786859 [details]
Bug 1293710 - Display highlights in activity stream panel.

https://reviewboard.mozilla.org/r/75736/#review74662

Comment 28

[Andrzej Hunt :ahunt]

Comment on attachment 8787597 [details]
Bug 1293710 - Activity Stream Highlights: Load icons and restructured layout.

https://reviewboard.mozilla.org/r/76318/#review74664

Comment 29

[Andrzej Hunt :ahunt]

Comment on attachment 8787697 [details]
Bug 1293710 - Activity Stream Highlights: Consider bookmarks without history too.

https://reviewboard.mozilla.org/r/76386/#review74666

Comment 30

[Andrzej Hunt :ahunt]

Comment on attachment 8787698 [details]
Bug 1293710 - Activity Stream Highlights: Only select actual bookmarks (no folders and other special types).

https://reviewboard.mozilla.org/r/76388/#review74668

Comment 31

[Andrzej Hunt :ahunt]

Comment on attachment 8787710 [details]
Bug 1293710 - Group activity stream highlights by URL to avoid duplicates.

https://reviewboard.mozilla.org/r/76390/#review74670

Comment 32

[Andrzej Hunt :ahunt]

Comment on attachment 8787711 [details]
Bug 1293710 - Add unit tests for highlights query.

https://reviewboard.mozilla.org/r/76404/#review74674

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

https://reviewboard.mozilla.org/r/74968/#review73668

> Is it necessary to wrap queries in a SELECT * FROM (...)?

(This is copied from the desktop query) Afaik without that the query parser understood the query differently (WHERE statement applied to UNION or something like that).

Comment 34

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.

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

Comment 35

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8786859 [details]
Bug 1293710 - Display highlights in activity stream panel.

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

Comment 36

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787597 [details]
Bug 1293710 - Activity Stream Highlights: Load icons and restructured layout.

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

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787697 [details]
Bug 1293710 - Activity Stream Highlights: Consider bookmarks without history too.

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

Comment 38

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787698 [details]
Bug 1293710 - Activity Stream Highlights: Only select actual bookmarks (no folders and other special types).

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

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787710 [details]
Bug 1293710 - Group activity stream highlights by URL to avoid duplicates.

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

Comment 40

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787711 [details]
Bug 1293710 - Add unit tests for highlights query.

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

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02c5c53bd1f8
Obtain a list of "highlights" for the Activity Stream panel. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/8f5162613934
Display highlights in activity stream panel. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/d5c476180ab0
Activity Stream Highlights: Load icons and restructured layout. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/b08a883b70f4
Activity Stream Highlights: Consider bookmarks without history too. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/8bd1bb9a4fd0
Activity Stream Highlights: Only select actual bookmarks (no folders and other special types). r=ahunt
https://hg.mozilla.org/integration/autoland/rev/e7a3dc48a799
Group activity stream highlights by URL to avoid duplicates. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/20809b09b2ff
Add unit tests for highlights query. r=ahunt

Comment 42

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/02c5c53bd1f8
https://hg.mozilla.org/mozilla-central/rev/8f5162613934
https://hg.mozilla.org/mozilla-central/rev/d5c476180ab0
https://hg.mozilla.org/mozilla-central/rev/b08a883b70f4
https://hg.mozilla.org/mozilla-central/rev/8bd1bb9a4fd0
https://hg.mozilla.org/mozilla-central/rev/e7a3dc48a799
https://hg.mozilla.org/mozilla-central/rev/20809b09b2ff