445955 - Put search bar keyworded searches in location bar autocomplete

Status: RESOLVED WORKSFORME

(Firefox :: Address Bar, defect, P3)

Description

[Ed Lee :Mardak]

Now that bug 392143 is fixed, keyworded bookmarks show up in the autocomplete. Something similar can be done with keyworded search engines.

The tricky part is that some searches might contain POST data and not work correctly.. but it seems like the default set of search engines all use GET, so the URLs show up correctly.

The Show Keywords add-on already provides this functionality:
https://addons.mozilla.org/en-US/firefox/addon/7755/

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

but then do not forget to eliminate showing up the keyworded items duplicate times ;)

Comment 2

[Ed Lee :Mardak]

Attachment: draft v1

Here's an initial stab, but I'm leaning more towards a separate .jsm that keeps track of both bookmark keywords and search engine keywords. It would know how to regenerate its local hash of keywords when things change. It would provide an interface to get keyword data that lazy loads its local hash.

So nsKeywordSearch would just do something like..

startSearch: function() {
  let keywordData = Keywords.getData(keyword);

This would be more useful down the line if we do something like..
autocompletesearch="keyword identity history"

Where identity just shows an entry of what you typed.. only if it isn't a keyword. (For bug 373352.)

identity("sdwilsh") => Search for "sdwilsh" (use the default search functionality by letting the "url" be "sdwilsh")

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 346229 [details] [diff] [review]
draft v1

>diff --git a/toolkit/components/places/src/nsKeywordSearch.js b/toolkit/components/places/src/nsKeywordSearch.js
vim line modes please (like nsPlacesDBFlush.js)

>+  _loadKeywords: function KS__loadKeywords()
>+  {
>+    // Get keyworded bookmarks
>+    let query = Cc["@mozilla.org/browser/nav-history-service;1"].
>+      getService(Ci.nsPIPlacesDatabase).DBConnection.createStatement(
>+        "SELECT k.keyword, b.title, (" +
>+          "SELECT f.url " +
>+          "FROM moz_places " +
>+          "JOIN moz_favicons f ON f.id = favicon_id " +
>+          "WHERE rev_host = (" +
>+            "SELECT rev_host " +
>+            "FROM moz_places " +
>+            "WHERE id = b.fk) " +
>+          "ORDER BY frecency DESC LIMIT 1) " +
>+        "FROM moz_keywords k " +
>+        "JOIN moz_bookmarks b ON b.keyword_id = k.id");
This query needed to be updated from the fsync changes

>+    while (query.executeStep())
>+      this._keywords[query.getString(0)] =
>+        [query.getString(1), query.getString(2)];
So, we get the wrapper for free now, so I'd prefer it if you used .step() and .row.{name} too here.  The code becomes much more readable.

r=sdwilsh with those changes.  This actually makes our autocomplete code a lot simpler :)  You could probably do the jsm approach when we do the other stuff that you talk about.

Comment 4

[Ed Lee :Mardak]

(In reply to comment #3)
> >+          "SELECT f.url " +
> >+          "FROM moz_places " +
> >+          "JOIN moz_favicons f ON f.id = favicon_id " +
> >+          "WHERE rev_host = (" +
> >+            "SELECT rev_host " +
> >+            "FROM moz_places " +
> >+            "WHERE id = b.fk) " +
> >+          "ORDER BY frecency DESC LIMIT 1) " +
> This query needed to be updated from the fsync changes
The only downside of not using moz_places_temp is that we might not have a favicon for a recently added bookmark. Not super critical and avoids recreating BEST_FAVICON_FOR_REVHOST in js.

> So, we get the wrapper for free now, so I'd prefer it if you used .step() and
> .row.{name} too here.  The code becomes much more readable.

O.o query.getColumnName(2) => (SELECT f.url FROM moz_places JOIN moz_favicons f ON f.id = favicon_id WHERE rev_host = (SELECT rev_host FROM moz_places WHERE id = b.fk) ORDER BY frecency DESC LIMIT 1)

Aliased to url! :p

> You could probably do the jsm approach when we do the other stuff
Well, this patch isn't completely done as it doesn't handle bookmark changes or search engine changes. Might as well refactor the _loadKeywords into the jsm that updates itself on uri change, bookmark added/removed. Needs to implement nsINavBookmarkObserver and then nsIObserver for app quit to remove listeners as well as engine-added engine-removed notifications.

Comment 5

[Dietrich Ayala (:dietrich)]

adding uiwanted to get some ux input.

Comment 11

[Paul [pwd]]

set a flag given that its got a patch.

Comment 12

[Shawn Wilsher :sdwilsh]

Tracking flags are meant for fixed bugs or regressions.

Comment 13

[Joe Wilson]

Does this bug cover keyword suggestions shown in location bar?
If so - bug 577226 may be a duplicate of this one.
If not - could you please add it as blocking bug to this one and take a look at it?
There is already an extension, and it's code may be used to fix that bug.

Comment 14

[Marco Bonardo [:mak]]

I think the current state with token aliases, aliases and search mode supersedes this change.