Closed
Bug 1293710
Opened 8 years ago
Closed 8 years ago
Obtain a list of "highlights" and display them in the AS panel
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox51 fixed)
RESOLVED
FIXED
Firefox 51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: sebastian, Assigned: sebastian)
References
()
Details
(Whiteboard: [MobileAS])
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
ahunt
:
review+
Grisha
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ahunt
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ahunt
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ahunt
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ahunt
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ahunt
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ahunt
:
review+
|
Details |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Oops, accidentally linked iOS bug.
Updated•8 years ago
|
Rank: 2
Updated•8 years ago
|
Whiteboard: [MobileAS backlog] → [MobileAS]
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Priority: P3 → P1
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 3•8 years ago
|
||
https://github.com/mozilla/activity-stream/blob/e975875984c5b3e883584e163d3819e7f020128d/common/constants.js#L14
> // Thresholds for highlights query
> HIGHLIGHTS_THRESHOLDS: {
> created: "-3 day",
> visited: "-30 minutes"
>},
Assignee | ||
Comment 4•8 years ago
|
||
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}'`);
Assignee | ||
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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 hidden (mozreview-request) |
Updated•8 years ago
|
Comment 8•8 years ago
|
||
mozreview-review |
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!
Assignee | ||
Comment 9•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 12•8 years ago
|
||
Are you going to also implement the new hotness that is weighted highlights?
Assignee | ||
Comment 13•8 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•8 years ago
|
||
mozreview-review |
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.
Attachment #8785945 -
Flags: review?(gkruglov) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8785945 [details]
Bug 1293710 - Obtain a list of "highlights" for the Activity Stream panel.
https://reviewboard.mozilla.org/r/74968/#review74660
Attachment #8785945 -
Flags: review?(ahunt) → review+
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8786859 [details]
Bug 1293710 - Display highlights in activity stream panel.
https://reviewboard.mozilla.org/r/75736/#review74662
Attachment #8786859 -
Flags: review?(ahunt) → review+
Comment 28•8 years ago
|
||
mozreview-review |
Comment on attachment 8787597 [details]
Bug 1293710 - Activity Stream Highlights: Load icons and restructured layout.
https://reviewboard.mozilla.org/r/76318/#review74664
Attachment #8787597 -
Flags: review?(ahunt) → review+
Comment 29•8 years ago
|
||
mozreview-review |
Comment on attachment 8787697 [details]
Bug 1293710 - Activity Stream Highlights: Consider bookmarks without history too.
https://reviewboard.mozilla.org/r/76386/#review74666
Attachment #8787697 -
Flags: review?(ahunt) → review+
Comment 30•8 years ago
|
||
mozreview-review |
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
Attachment #8787698 -
Flags: review?(ahunt) → review+
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8787710 [details]
Bug 1293710 - Group activity stream highlights by URL to avoid duplicates.
https://reviewboard.mozilla.org/r/76390/#review74670
Attachment #8787710 -
Flags: review?(ahunt) → review+
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8787711 [details]
Bug 1293710 - Add unit tests for highlights query.
https://reviewboard.mozilla.org/r/76404/#review74674
Attachment #8787711 -
Flags: review?(ahunt) → review+
Assignee | ||
Comment 33•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 41•8 years ago
|
||
Pushed by s.kaspari@gmail.com:
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•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•8 years ago
|
Iteration: --- → 1.3
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•