1185038 - Include/Suggest bookmarks in in-browser search results

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Aaron Train [:aaronmt]]

Currently we cease to suggest bookmarks are clearing private data 

 Visit http://techmeme.com
 Add it as a bookmark
 Type 'tech', see it suggested
 Clear Private Data
 Type 'tech', see it not suggested

Reproducible on master (2531f5)

Comment 1

[Richard Newman [:rnewman]]

We only suggest history.

Comment 3

[Aaron Train [:aaronmt]]

Morph?

Comment 4

[Richard Newman [:rnewman]]

This will involve touching the getFilteredSites* queries, figuring out how bookmarks affect frecency, where to put bookmarks that have no visits, and also badging results with a star.

Comment 5

[Karen Rudnitski [:kar]]

Targeting 1.0.x would be my preference, but could fall back to 1.1 :)

Comment 6

[Richard Newman [:rnewman]]

See also Bug 1185543 for showing remote tabs.

Comment 7

[Aaron Train [:aaronmt]]

Pains me we don't have this yet but 2.0-

Comment 8

[Aaron Train [:aaronmt]]

Requested https://github.com/mozilla/firefox-ios/issues/1552

Comment 9

[Aaron Train [:aaronmt]]

Dependency resolved/re-summarize so I can find this better

Comment 10

[Robin Andersen [:tecgirl]]

Attachment: search-Bookmarks-2.png

Surface Bookmarks to the top of the list, in order of last-accessed if there are multiple results. Add Bookmark star to cell, vertically centered, 16px from right edge. Truncate long titles with an ellipsis if they reach within 16px to the left of the Bookmark star.

Comment 11

[Robin Andersen [:tecgirl]]

Attachment: bookmark-awesomelist.zip

Comment 12

[Richard Newman [:rnewman]]

Robin: what's the limit on returned bookmarks? E.g., if I search for "http", at what point should we split and start showing history?

Comment 13

[Robin Andersen [:tecgirl]]

(In reply to Richard Newman [:rnewman] from comment #12)
> Robin: what's the limit on returned bookmarks? E.g., if I search for "http",
> at what point should we split and start showing history?

I think 5 is sufficient. That's enough to fill the viewport on our smallest supported device when the keyboard drops on tap and isn't overwhelming.

Comment 14

[Richard Newman [:rnewman]]

I've got this done apart from showing the star in results and excluding bookmarks from the subsequent history rows.

Comment 15

[Richard Newman [:rnewman]]

Attachment: Pull req.

I noticed that table rows have an accessory view already, so I skipped the heavyweight approach in Bug 1168849 and decided to use that.

The data access part of this isn't 100% polished:

* We don't seem to retrieve favicons correctly.
* We don't de-dupe bookmarks on URL.
* We don't decorate history items that are bookmarked but aren't in the initial bookmark set (part of Bug 1168849's scope).
* We don't de-dupe history items that are also in the initial bookmark set, so they can be shown twice.

I don't know if those limitations should stop us landing this -- it's better than nothing, but fixing those might involve another database change. We could land in two parts.

Comment 16

[Richard Newman [:rnewman]]

Robin gave ui+ over IRC. Tested on 5S and 6+.

Comment 17

[Richard Newman [:rnewman]]

Note that another way of doing this storage-wise is to FULL OUTER JOIN bookmarks to history, rather than to union two subqueries. That'd fix some of those limitations, with some cost and some edge cases (e.g., what do we GROUP BY on?).

Comment 18

[Emily Toop (:fluffyemily)]

Comment on attachment 8734959 [details] [review]
Pull req.

Sorry for the delayed review. just a couple of nits. LGTM.

Comment 19

[Richard Newman [:rnewman]]

Thanks, Emily! I'll file follow-ups for anything I don't address immediately.

Comment 20

[Richard Newman [:rnewman]]

I'm just wrapping this up with a commit that fetches icons.

Comment 21

[Richard Newman [:rnewman]]

Merged:

https://github.com/mozilla/firefox-ios/commit/ae949e63eb9f8d6fa302527131f0284246a3036c