1404334 - Highlights query should return bookmarks with no visits.

Status: RESOLVED WONTFIX

(Firefox :: New Tab Page, defect)

Description

[Andrei Oprea [:andreio]]

We need to return bookmarks with no visits so that the highlight section displays Pocket bookmarks.

Comment 1

[Andrei Oprea [:andreio]]

This needs to land with https://github.com/mozilla/activity-stream/pull/3623.

Comment 2

[Andrei Oprea [:andreio]]

Attachment: Bug 1404334 - Change NewTabUtils Bookmarks query to fetch bookmarks with no visits.

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

Comment 3

[Ed Lee :Mardak]

Comment on attachment 8913685 [details]
Bug 1404334 - Change NewTabUtils Bookmarks query to fetch bookmarks with no visits.

https://reviewboard.mozilla.org/r/185084/#review190124

::: toolkit/modules/NewTabUtils.jsm:1009
(Diff revision 1)
>          AND b.title NOTNULL
>          AND b.type = :bookmarkType
>          AND p.parent <> :tagsFolderId
> -        ${this._commonPlacesWhere}
> +        AND hidden = 0
> +        AND (SUBSTR(url, 1, 6) == "https:"
> +          OR SUBSTR(url, 1, 5) == "http:")

Instead of copying stuff from `commonPlacesWhere`, this query should be unchanged and remove the undesired condition from the shared part and added to the queries that do want it.

Comment 4

[Andrei Oprea [:andreio]]

Comment on attachment 8913685 [details]
Bug 1404334 - Change NewTabUtils Bookmarks query to fetch bookmarks with no visits.

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

Comment 5

[Andrei Oprea [:andreio]]

Comment on attachment 8913685 [details]
Bug 1404334 - Change NewTabUtils Bookmarks query to fetch bookmarks with no visits.

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

Comment 6

[Ed Lee :Mardak]

Comment on attachment 8913685 [details]
Bug 1404334 - Change NewTabUtils Bookmarks query to fetch bookmarks with no visits.

https://reviewboard.mozilla.org/r/185084/#review190800

You'll need to update tests probably just `getHighlights`. Also, could you verify that the bookmarks query is still appropriately querying on an index without the condition? I'm pretty sure it will still use the index because of the `ORDER BY dateAdded` but I've seen some strange things when touching queries…

::: toolkit/modules/NewTabUtils.jsm:862
(Diff revision 3)
>        ) NOTNULL
>      ) AS bookmarkGuid`,
>  
>    /**
>     * Shared WHERE expression filtering out undesired pages, e.g., hidden,
>     * unvisited, and non-http/s urls. Assumes moz_places is in FROM / JOIN.

This comment needs to be updated

Comment 7

[Andrei Oprea [:andreio]]

Comment on attachment 8913685 [details]
Bug 1404334 - Change NewTabUtils Bookmarks query to fetch bookmarks with no visits.

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

Comment 8

[Andrei Oprea [:andreio]]

Updated. Query still uses index:

> 0|0|0|SEARCH TABLE moz_bookmarks AS b USING INDEX moz_bookmarks_dateaddedindex (dateAdded>?)
> 0|1|1|SEARCH TABLE moz_bookmarks AS p USING INTEGER PRIMARY KEY (rowid=?)
> 0|2|2|SEARCH TABLE moz_places AS h USING INTEGER PRIMARY KEY (rowid=?)

Comment 9

[Ed Lee :Mardak]

Comment on attachment 8913685 [details]
Bug 1404334 - Change NewTabUtils Bookmarks query to fetch bookmarks with no visits.

https://reviewboard.mozilla.org/r/185084/#review191612

Comment 10

[Andrei Oprea [:andreio]]

Comment on attachment 8913685 [details]
Bug 1404334 - Change NewTabUtils Bookmarks query to fetch bookmarks with no visits.

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/680e3de137fb
Change NewTabUtils Bookmarks query to fetch bookmarks with no visits. r=Mardak

Comment 13

[Boris Zbarsky [:bzbarsky]]

Backed out for causing all sorts of tests to try to load planet.mozilla.org in automation (from the new tab page or something?) and failing because loads of external URIs are not allowed in automation.

Backout push: https://hg.mozilla.org/integration/autoland/rev/330fc30dc2b0c64c34f3d364d6fe25a3ca0132e3

Typical push with failures due to this (most of the failures on this push, in fact): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=67bd2a7d97798312b7cf25f87d6be2e4a27d9d55

Comment 14

[Andrei Oprea [:andreio]]

Looking at the revision the bookmarks threshold patch landed https://hg.mozilla.org/integration/autoland/file/67bd2a7d97798312b7cf25f87d6be2e4a27d9d55/browser/extensions/activity-stream/lib/HighlightsFeed.jsm#l98 so I assume:

>  try to load planet.mozilla.org in automation

Is due to a threshold that is too small and trying to load default bookmarks in Highlights.
Maybe this needs to wait for https://github.com/mozilla/activity-stream/issues/3633 to get fixed.
Does this sound right?

Comment 15

[Ed Lee :Mardak]

Could be. You can try increasing the time. Also, there's other callers to getHighlights that might be triggering the network request. Looks like you should be able to reproduce this locally on any platform.

Comment 16

[Andrei Oprea [:andreio]]

Fixed by https://github.com/mozilla/activity-stream/pull/3734.

Comment 17

[Ed Lee :Mardak]

(In reply to Andrei Oprea [:andreio] from comment #16)
> Fixed by https://github.com/mozilla/activity-stream/pull/3734.
This is the opposite of fixed as we went with an approach that does not need the highlights query to return bookmarks with no visits.

This is because including unvisited bookmarks results in default bookmarks that can be tricky to get the correct recent time cutoff to exclude, etc.