Bug 1298918 - Show Highlights in Activity Stream Panel #2084

sleroux · 2016-09-07T20:06:05Z

First version of implementing a similiar algorithm for determining 'highlights' from a combination of a user's history and bookmarks. The query mostly implements the pseudo query Sebastian wrote here [1] with the except of a host blacklist. Unfortunately we don't store when a bookmark was created on iOS and is not something that comes down from sync bookmarks [2]. The best we can do is use the last_modified time of the bookmark. Probably not the worst thing as in most cases would reflect creation time otherwise it would indicate some source of activity on the bookmark which is significant in itself. One question I have with this approach is that the query assumes local_modified and server_modified come from the same clock which isn't true. Are these comparable in any way?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1293710#c5
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=676563

rnewman · 2016-09-08T00:48:13Z

One question I have with this approach is that the query assumes local_modified and server_modified come from the same clock which isn't true. Are these comparable in any way?

They're not comparable, but:

A user who doesn't use Sync will only have local_modified.
A user who does use Sync, and who is syncing bidirectionally, will eventually have a server_modified for every record, and no local modification time at all. See https://github.com/mozilla/firefox-ios/blob/068ad3cc790247f7d9f2faf1994ec6ca1fe0db43/Storage/SQL/SQLiteBookmarksSyncing.swift#L1039-L1039
Usually an iOS user's device will have a somewhat accurate clock, so these will be vaguely comparable.

rnewman · 2016-09-08T01:00:32Z

Storage/SQL/SQLiteHistoryRecommendations.swift

+import XCGLogger
+import Deferred
+
+/// Small enum to help strength parameter requirements for potential bookmark highlights query below.


rnewman · 2016-09-08T01:24:04Z

First-pass feedback done, @sleroux. Lemme know when you want me to look again!

sleroux · 2016-09-13T14:45:25Z

@rnewman Thanks for the suggestion to the view on local bookmarks. With the changes to the query and the adjusted view, this looks a lot simpler and seems to be a much faster query. I'll follow up with some measureBlock tests on the query. When you get a chance, could you give this another pass?

sleroux · 2016-09-13T14:51:35Z

Updated query for easier reading: https://gist.github.com/sleroux/07f060b594903eb23bdfc3df642493a9

Check whether you need this wrapper select.

I guess SQLite doesn't like handling LIMIT on queries that are about to be UNION'ed so the suggestion was to wrap those queries in a SELECT * query to work around that.

farhanpatel · 2016-09-16T18:09:17Z

Are you planning to add support for Hiding items from Highlights? Similar to how TopSites work.

sleroux · 2016-09-16T18:19:50Z

Are you planning to add support for Hiding items from Highlights? Similar to how TopSites work.

Good question. I'm not actually sure. Also makes me think - does highlights also include what we want to show for Top Sites or are we keeping that algorithm the same?

cc @bryanbell

bkmunar · 2016-09-16T19:54:47Z

@sleroux We do want to support hiding items for highlights per Bryan Bell's request to add a "dismiss" option for the context menu. Also, idk to me I feel like Top Sites algo is the same b/c my desktop top sites seem to be my phone top sites

rnewman

That UNION needs a look, @sleroux. Otherwise good.

rnewman · 2016-09-19T15:36:44Z

Storage/SQL/BrowserTable.swift

@@ -100,7 +100,7 @@ private let log = Logger.syncLogger
 * We rely on SQLiteHistory having initialized the favicon table first.
 */
 public class BrowserTable: Table {
-    static let DefaultVersion = 16    // Bug 1185038.
+    static let DefaultVersion = 17    // Bug 1185038.


Change the bug number.

rnewman · 2016-09-19T15:38:08Z

Storage/SQL/SQLiteHistoryRecommendations.swift

+extension SQLiteHistory: HistoryRecommendations {
+    public func getHighlights() -> Deferred<Maybe<Cursor<Site>>> {
+        let limit = 20
+        let microsecondsPerMinute: Timestamp = 60_000_000 // 1000 * 1000 * 60


rnewman · 2016-09-19T15:38:35Z

Storage/SQL/SQLiteHistoryRecommendations.swift

+        let thirtyMinutesAgo = now - 30 * microsecondsPerMinute
+        let threeDaysAgo = now - (60 * microsecondsPerMinute) * 24 * 3
+        let bookmarkLimit = 1
+        let historyLimit = limit - bookmarkLimit


Move all the limit stuff together.

rnewman · 2016-09-19T15:40:00Z

Storage/SQL/SQLiteHistoryRecommendations.swift

+            "   FROM (",
+            "       SELECT siteID AS s, max(date) AS visitDate",
+            "       FROM \(TableVisits)",
+            "       WHERE date < \(thirtyMinutesAgo)",


thirtyMinutesAgo is the only dynamic part of this query. If you extract that as a value in args, the compiler should be able to avoid dynamically concatenating this string. (Same below for threeDaysAgo.)

In the future, if we need to squeeze more perf out of the system, that will also allow us to pre-parse statements rather than parsing SQL each time we fetch.

rnewman · 2016-09-19T15:43:36Z

Storage/SQL/SQLiteHistoryRecommendations.swift

+            "   WHERE visitCount <= 3 AND title NOT NULL AND title != ''",
+            "   LIMIT \(historyLimit)",
+            ")"
+        ].joinWithSeparator(" ")


Implied: do this with string concatenation instead of dynamically using joinWithSeparator. I think the Swift compiler is much better at type analysis now, so this shouldn't blow up compile time, but probably not smart enough to recognize joinWithSeparator on constants.

rnewman · 2016-09-19T15:47:22Z

Storage/SQL/SQLiteHistoryRecommendations.swift

+            "   FROM (",
+            "       SELECT bmkUri",
+            "       FROM \(ViewBookmarksLocalOnMirror)",
+            "       WHERE \(ViewBookmarksLocalOnMirror).server_modified > \(threeDaysAgo) OR \(ViewBookmarksLocalOnMirror).local_modified > \(threeDaysAgo)",


Bear in mind that you'll need to enable bidirectional syncing in order to test this feature.

rnewman · 2016-09-19T15:52:46Z

Storage/SQL/SQLiteHistoryRecommendations.swift

+            "SELECT \(highlightProjection)",
+            "FROM (",
+                bookmarkHighlights,
+                "UNION",


This UNION might not do what you expect.

Your bookmark highlights always return NULL AS visitDate. Your non-recent history query always returns a max visit date. visitDate is in highlightProjection, so this UNION is always equivalent to UNION ALL, and you're relying on GROUP BY url below to randomly select one of the two rows in the event that you have a bookmark that's been visited more than three times more than 30 minutes ago.

The observed result is probably that sometimes you'll have a date in a row, and sometimes you won't.

sleroux · 2016-09-19T19:01:55Z

@rnewman I've rebased from master to support Xcode 8/Swift 2.3 and address the UNION issue. I think by using max(visitDate) in with the GROUP BY statement we'll always select the one with the date if there is a bookmark as well. I've also added a isBookmarked flag to more explicitly indicate that the visit is indeed a bookmark instead of relying on a NULL visitDate for that info.

I haven't tested this yet with remote bookmarks since I'm seeing an issue on iOS 10 with sync but I'll look into that this afternoon.

rnewman · 2016-09-20T16:16:25Z

Figured I'd flesh out my review comment for posterity.

In order to use UNION (or even DISTINCT!) to get unique results, you need to either narrow the set of columns so that UNION removes the duplicates (which means discarding all of the stuff that makes bookmarks and history differ!), use an enclosing query to transform the result set, or…

Conceptually you're trying to do a FULL OUTER JOIN: that is, give me things from this set, and things from this set, and if they overlap on a given criterion, merge them together.

SQLite doesn't support FOJ, and emulating it isn't cheap, but you could do it.

The approach you've taken here is an enclosing query, using GROUP BY to collapse rows together when they refer to the same URL. That needs a teensy bit more work, because the behavior when you return a non-aggregate value from a grouped result set is undefined. Comment inline…

rnewman · 2016-09-20T16:26:02Z

Storage/SQL/SQLiteHistoryRecommendations.swift

+            ")"
+
+        let highlightsQuery =
+            "SELECT historyID, url, title, guid, visitCount, max(visitDate) AS visitDate, isBookmarked, iconID, iconURL, iconType, iconDate, iconWidth " +


Some of these values come from individual rows, and differ on the row. url is fixed by virtue of being the grouping characteristic, but visitCount might come from the bookmark row, or isBookmarked might come from the history row!

Here you see why this is really a FULL OUTER JOIN: we want to merge the two together.

Now, we know there's at most one history row, and at most one bookmark URL, for each URL. If a bookmark row is returned, it'll already be joined against history, so the title etc. will match. The remaining column is isBookmarked, so we can try to phrase this projection in terms of aggregates:

SELECT historyID, url, title, guid, visitCount, max(visitDate) AS visitDate, max(isBookmarked) AS isBookmarked, iconID, iconURL, iconType, iconDate, iconWidth

However, we're half-way to just doing a FOJ — you're selecting bookmarks and LEFT JOINing them against history. Another option here, then, is for you to alter the history side of the UNION to be history items that match but are not bookmarked. You can then safely avoid grouping by anything, and just use a UNION ALL.

Another option here, then, is for you to alter the history side of the UNION to be history items that match but are not bookmarked. You can then safely avoid grouping by anything, and just use a UNION ALL.

That's smart. I feel like I can avoid a lot of hassle doing that instead since I know that the bookmark/recent history queries will be mutually exclusive.

I think I can modify the query to include a bookmark check in the project like so (excludes the date checks):

SELECT historyID, url, title, latest, visitCount FROM ( SELECT history.id as historyID, url, title, latest, (SELECT COUNT(1) FROM visits WHERE s = visits.siteID) AS visitCount, (SELECT COUNT(1) FROM view_bookmarksLocal_on_mirror WHERE view_bookmarksLocal_on_mirror.bmkUri == url) AS bookmarked FROM ( SELECT siteID AS s, max(date) AS latest FROM visits GROUP BY siteID ) LEFT JOIN history ON history.id = s WHERE visitCount <= 3 AND title NOT NULL AND title != '' AND bookmarked == 0 LIMIT 19 )

rnewman

This looks good, but remember you can now use UNION ALL for a little speed boost.

You might also consider restoring isBookmarked, with hardcoded 0/1 values, so that highlightsQuery includes that for each row — I imagine the UI wants to throw a little star in there, or know whether to offer to bookmark the highlight or not…

… query

… avoid dupes

…and cleans up unused test

rnewman reviewed Sep 8, 2016
View reviewed changes

sleroux force-pushed the sleroux/Bug1298918-ActivityStreamHighlights branch from 9eb26cf to ce12b46 Compare September 13, 2016 14:47

rnewman suggested changes Sep 19, 2016

View reviewed changes

sleroux force-pushed the sleroux/Bug1298918-ActivityStreamHighlights branch from ce12b46 to 3b10982 Compare September 19, 2016 18:59

sleroux force-pushed the sleroux/Bug1298918-ActivityStreamHighlights branch from 3b10982 to 61b5fea Compare September 20, 2016 15:56

rnewman reviewed Sep 20, 2016

View reviewed changes

rnewman approved these changes Sep 23, 2016

View reviewed changes

sleroux force-pushed the sleroux/Bug1298918-ActivityStreamHighlights branch from 4d30e71 to 213efaf Compare September 23, 2016 17:20

Stephan Leroux added 9 commits September 23, 2016 14:18

Part 1: Adds new HistoryRecomendations and getHighlights query

a2a3f80

Part 2: Updates UI to have Highlight title instead of Recent Activity

d405d59

Part 3: Cleans up the SQLite factories into another file/extension

73d9225

Nit: Adds newlines to the end of new files

4f58c34

Cleans up query to be more efficent and correct

b8c41c8

Updates local bookmarks view to include modified times for highlights…

97786b2

… query

Removes unnecessary enum from HistoryRecomendations

f79f720

Fixes review comments and UNION query selection

f5bd0d2

(WIP) testing highlight

d8bf829

Stephan Leroux added 4 commits September 23, 2016 14:18

Modifies UNION query to link bookmarked/non-bookmark history items to…

4978043

… avoid dupes

Adds in isBookmark property so we can expose on a Highlight later on …

44e6f9e

…and cleans up unused test

Adds perf test for measuring highglights query

4f09c5f

Moves localized string from AS Panel to Strings.swift

ac7715b

sleroux force-pushed the sleroux/Bug1298918-ActivityStreamHighlights branch from e07ce72 to ac7715b Compare September 23, 2016 18:19

Merge issue

469fe8e

sleroux merged commit 1207536 into master Sep 23, 2016

sleroux deleted the sleroux/Bug1298918-ActivityStreamHighlights branch September 23, 2016 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1298918 - Show Highlights in Activity Stream Panel #2084

Bug 1298918 - Show Highlights in Activity Stream Panel #2084

sleroux commented Sep 7, 2016

rnewman commented Sep 8, 2016 •

edited

rnewman Sep 8, 2016

rnewman commented Sep 8, 2016

sleroux commented Sep 13, 2016

sleroux commented Sep 13, 2016

farhanpatel commented Sep 16, 2016

sleroux commented Sep 16, 2016

bkmunar commented Sep 16, 2016

rnewman left a comment

rnewman Sep 19, 2016

rnewman Sep 19, 2016

rnewman Sep 19, 2016

rnewman Sep 19, 2016 •

edited

rnewman Sep 19, 2016

rnewman Sep 19, 2016

rnewman Sep 19, 2016

sleroux commented Sep 19, 2016

rnewman commented Sep 20, 2016

rnewman Sep 20, 2016

sleroux Sep 20, 2016

rnewman left a comment

Bug 1298918 - Show Highlights in Activity Stream Panel #2084

Bug 1298918 - Show Highlights in Activity Stream Panel #2084

Conversation

sleroux commented Sep 7, 2016

rnewman commented Sep 8, 2016 • edited

Choose a reason for hiding this comment

rnewman commented Sep 8, 2016

sleroux commented Sep 13, 2016

sleroux commented Sep 13, 2016

farhanpatel commented Sep 16, 2016

sleroux commented Sep 16, 2016

bkmunar commented Sep 16, 2016

rnewman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewman Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleroux commented Sep 19, 2016

rnewman commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewman left a comment

Choose a reason for hiding this comment

rnewman commented Sep 8, 2016 •

edited

rnewman Sep 19, 2016 •

edited