455555 - Use asynchronous queries for places autocomplete

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Shawn Wilsher :sdwilsh]

We can not block the main thread while processing, for starters.  This includes reads, which will hit folks on slow devices as well.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

For your viewing pleasure.  This doesn't even work yet, but it's close.

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v0.2

Got the query aggregator done today.  I haven't tested it, but the logic is all there as far as I can tell.  Should have something working to play with tomorrow.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v0.3

This doesn't handle keywords and doesn't escape URI's properly, but it's good for a first pass review.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 342004 [details] [diff] [review]
v0.3

a first pass at this, when you'll have implemented keywords i'll start doing some functionality test.

diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h
--- a/toolkit/components/places/src/nsNavHistory.h
+++ b/toolkit/components/places/src/nsNavHistory.h
@@ -128,42 +118,31 @@ class nsNavHistory : public nsSupportsWe
 class nsNavHistory : public nsSupportsWeakReference,
                      public nsINavHistoryService,
                      public nsIObserver,
                      public nsIBrowserHistory,
                      public nsIGlobalHistory3,
                      public nsIDownloadHistory,
                      public nsICharsetResolver
                    , public nsPIPlacesDatabase

is this comma move from fsync patches? we will end having commas flying around!

@@ -639,122 +618,19 @@ protected:
-  nsCOMPtr<nsIAutoCompleteObserver> mCurrentListener;
-  nsCOMPtr<nsIAutoCompleteSimpleResult> mCurrentResult;
-#endif
-
-  MatchType mCurrentMatchType;
-  MatchType mPreviousMatchType;
-  nsDataHashtable<nsStringHashKey, PRBool> mCurrentResultURLs;
-  PRInt32 mCurrentChunkOffset;
-  PRInt32 mPreviousChunkOffset;
-
   nsDataHashtable<nsTrimInt64HashKey, PRBool> mLivemarkFeedItemIds;
   nsDataHashtable<nsStringHashKey, PRBool> mLivemarkFeedURIs;

I guess you can get rid of these hashes, they were used by autocomplete to
remove feed items from results, i doubt they are still needed

diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
@@ -0,0 +1,1192 @@
+////////////////////////////////////////////////////////////////////////////////
+//// Constants
+
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+const Cr = Components.results;
+
+// this is just a helper for the next constant
+function sql_str_fragment(aName, aColumn, aComparison, aGetMostRecent)
+{
+  return "(" +
+    "SELECT " + aColumn + " " +
+    "FROM moz_bookmarks b " +
+    "JOIN moz_bookmarks t " +
+      "ON t.id = b.parent " +
+      "AND t.parent " + aComparison + " ?1 " +
+    "WHERE b.type = " + Ci.nsINavBookmarksService.TYPE_BOOKMARK + " " +
+    "AND b.fk = h.id " +

indent additional conditions

+    (aGetMostRecent ? "ORDER BY b.lastModified DESC LIMIT 1" : "") +
+  ") AS " + aName;
+}
+const BOOK_TAG_SQL =
+  sql_str_fragment("parent", "b.parent", "!=", true) + ", " +
+  sql_str_fragment("bookmark", "b.title", "!=", true) + ", " +
+  sql_str_fragment("tags", "GROUP_CONCAT(t.title, ',')", "=", false);

a comment to understand what this does without having to expand the full "macro" would be helpful
Original comment was too much "cryptic"

+
+// We don't have macros in JS...

this suppose the code reader knows that this is a conversion from cpp...
better commenting on what is this "macro" for

+function BEST_FAVICON_FOR_REVHOST(aTableName)
+{
+  return "(" +
+    "SELECT f.url " +
+    "FROM " + aTableName + " " +
+    "JOIN moz_favicons f " +
+    "ON f.id = favicon_id " +

indent ON or put on the same line as join

+ *
+ * @param aChar
+ *        The unicode character to check against.
+ * @returns true if the character is considered a word boundary, false otherwise
+ */
+function isWordBoundary(aChar)
+{
+  // Only check lowercase alphabetic characters so we can match CamelCase words.
+  // This means that matches will happen after and upper-case character.

mixed up comment "will happen after and upper-case" should probably be "will happen also after an upper-case"

+/**
+ * Searches aSourceString for aToken anywhere in the string.
+ *
+ * @param aToken
+ *        The string to search for.
+ * @param aSourceString
+ *        The string to search.
+ * @returns true if found, false otherwise.
+ */
+function findAnywhere(aToken, aSourceString)
+{
+  return aSourceString.indexOf(aToken) != -1;
+}

i've not read if you do transformations on the tokens later, hwv the original
findAnywhere function was case-insensitive, while indexOf IIRC is case.sensitive
If transformation to lowercase is done later a comment about it here would help.

+/**
+ * Tests if aSourceString starts with aToken.
+ *
+ * @param aToken
+ *        The string to search for.
+ * @param aSourceString
+ *        The string to search.
+ * @returns true if found, false otherwise.
+ */
+function findBeginning(aToken, aSourceString)
+{
+  return aSourceString.indexOf(aToken) == 0;
+}

as before (i'll stop commenting on this)

+/**
+ * Tests if aToken is found on a word boundary in aSourceString.
+ *
+ * @param aToken
+ *        The string to search for.
+ * @param aSourceString
+ *        The string to search.
+ * @returns true if found, false otherwise.
+ */
+function findOnBoundary(aToken, aSourceString)
+{
+  // No need to do work if there is nothing to match
+  if (aSourceString.length < aToken.length)
+    return false;
+
+  // We need same case strings for the token and the source to do comparisons
+  let lcToken = aToken.toLocaleLowerCase();
+  let lcSource = aSourceString.toLocaleLowerCase();
+
+  let [tokenStartInd, tokenEndInd] = [0, aToken.length];
+  let [sourceStartInd, sourceEndInd] = [0, aSourceString.length];

i'm not sure i like reading Ind suffix, losing 2 chars has less benefits than use Index, imho
+
+  // The start of aSourceString is considered a word boundary, so start there.
+  do {
+    // We are on a word boundary, so start by copying the indexes
+    let [testTokenInd, testSourceInd] = [tokenStartInd, sourceStartInd];
+
+    // Keep trying to match the token one by on until it doesn't match

typo "one by on"

+////////////////////////////////////////////////////////////////////////////////
+//// nsPlacesAutoComplete class
+
+function nsPlacesAutoComplete()
+{
+  //////////////////////////////////////////////////////////////////////////////
+  //// Shared Constants for Smart Getters
+
+  // Define common pieces of various queries
+  // XXX bug 412736
+  // in case of a frecency tie, break it with h.typed and h.visit_count
+  // which is better than nothing.  This is slow, so not doing it yet...

i think we should update this comment adding the fact that table partitioning
makes splitting more slower due to the fact we cannot use indexes


+  this.__defineGetter__("_tagsFolder", function() {
+    delete this._tagsFolder;
+    return this._tagsFolder = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+                              getService(Ci.nsINavBookmarksService).
+                              tagsFolder;
+  });

would not be better getting tagsFolder from bookmarks service every time instead of caching it?
we cannot be sure it will not change while the app is running (a backup/restore?)


+
+  //////////////////////////////////////////////////////////////////////////////
+  //// nsPlacesAutoComplete
+
+  /**
+   * Used to unescape encoded URI strings, and drop information that we do not
+   * care about for searching.
+   *
+   * @param aURIString
+   *        The text to unescape and modify.
+   * @returns the modified uri.
+   */
+  _fixupURIText: function PAC_fixupURIText(aURIString)
+  {
+    let uri = aURIString;
+
+    if (uri.indexOf("http://") == 0)
+      uri = uri.slice(7);
+    else if (uri.indexOf("https://") == 0)
+      uri = uri.slice(8);
+    else if (uri.indexOf("ftp://") == 0)
+      uri = uri.slice(6);
+
+    return this._textURIService.unEscapeURIForUI("UTF-16", uri);
+  },

case-sensitive?

+    this._matchOnlyTyped = safeGetter("matchOnlyTyped", "Bool", false);
+    this._matchBehavior = safeGetter("matchBehavior", "Int", MATCH_BOUNDARY_ANYWHERE);
+    this._filterJavascript = safeGetter("filter.javascript", "Bool", true);
+    this._maxRichResults = safeGetter("maxRichResults", "Int", 25);
+    this._restrictHistoryToken = safeGetter("restrict.history", "Char", "^");
+    this._restrictBookmarkToken = safeGetter("restrict.bookmark", "Char", "*");
+    this._restrictTagToken = safeGetter("restrct.tag", "Char", "+");
+    this._matchTitleToken = safeGetter("match.title", "Char", "#");
+    this._matchURLToken = safeGetter("match.url", "Char", "@");
+
+    // Validate matchBehavior; default to MATCH_BOUNDARY_ANYWHERE.
+    if (this._matchBehavior != MATCH_ANYWHERE &&
+        this._matchBehavior != MATCH_BOUNDARY &&
+        this._matchBehavior != MATCH_BEGINNING)
+      this._matchBehavior = MATCH_BOUNDARY_ANYWHERE;
+
+    // register observer
+    if (aRegisterObserver) {
+      let pb = prefs.QueryInterface(Ci.nsIPrefBranch2);
+      pb.addObserver("", this, false);

why not observing only changes to "browser.urlbar." branch?

+  /**
+   * Given an array of tokens, this function determines which query should be
+   * ran.  It also removes any special search tokens.
+   *
+   * @param aTokens
+   *        An array of search tokens.
+   * @returns the correctly optimized query to search the database with and the
+   *          new list of tokens to search with.  The query has all the needed
+   *          parameters bound, so

so...?

+
+    // Establish which query we should use
+    for (let i = 0; i < aTokens.length; i++) {
+      let token = aTokens[i];
+
+      if (token == this._restrictHistoryToken)
+        restrictHistory = true;
+      else if (token == this._restrictBookmarkToken)
+        restrictBookmark = true;
+      else if (token == this._restrictTagToken)
+        restrictTag = true;
+      else if (token == this._matchTitleToken)
+        matchTitle = true;
+      else if (token == this._matchURLToken)
+        matchURL = true;
+    }
+
+    // matchTitle and matchURL need to be set on the object - we use them later
+    this._matchTitle = matchTitle;
+    this._matchURL = matchURL;
+
+    // Filter out special search tokens
+    function isNotSpecialSearchToken(aElement) {
+      return aElement != this._restrictHistoryToken &&
+             aElement != this._restrictBookmarkToken &&
+             aElement != this._restrictTagToken &&
+             aElement != this._matchTitleToken;
+    }
+    let tokens = aTokens.filter(isNotSpecialSearchToken);

i'm not sure, but should not matchURLToken be here?
couldn't you filter out in the previous for loop as in the original code?


+    // We use more optimized queries for restricted searches, so we will always
+    // return the most restrictive one to the least restrictive one if more than
+    // one token is found.
+    let query = restrictTag ? this._tagsQuery :
+                restrictBookmark ? this._starQuery :
+                restrictHistory ? this._historyQuery :
+                this._defaultQuery;
+
+    // Bind the right parameters to the query so it is useful
+    query.bindInt32Parameter(0, this._tagsFolder);

comment is somehow confusing

+   * Processes a mozIStorageRow to generate the proper data for the AutoComplete
+   * result.  This will add an entry to the current result if it matches the
+   * criteria.
+   *
+   * @param aRow
+   *        The row to process.
+   * @returns true if the row is accepted, and false if not.
+   */
+  _processRow: function PAC_processRow(aRow)
+  {
+    // We only want to filter javascript: URIs if the search does not start with
+    // "javascript:"
+    let filterJavascript = this._filterJavascript &&
+                           this._currentSearchString.indexOf("javascript:") != 0;

what if i search "javaScript:" ? (apart that i'm crazy)

+    let escapedEntryURL = aRow.getResultByIndex(kAutoCompleteIndex_URL);
+
+    // If we need to filter javascript and have a javascript URI, skip it
+    if (filterJavascript && escapedEntryURL.indexOf("javascript:") == 0)
+      return false;

as above, apart if escapedEntryURL is lc

+function QueryAggregator(aQueries, aCallback)
+{
+  this._listener = aCallback;
+
+  // To make the code in PendingQueryListener simpler, we create an empty result
+  // object that has no results and is done.
+  this._results = [];
+  this._results[0] = new IndividualResults();
+  this._results[0].done = true;
+  this._results[0].notified = true;
+
+  // Used to keep track of what has been notified to avoid duplicates.  This is
+  // an array of place id's.
+  this._results.notified = [];
+
+  // Now we create result objects for each query, and a pending query listener
+  // while will run the query.

confusing comment

+
+      // However, if anything before us is isn't done, we can return early
+      if (!data.done)
+        return;

typo "is isn't"

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> is this comma move from fsync patches? we will end having commas flying around!
Happened before - not really wanting to change it here (too much churn already)

> i think we should update this comment adding the fact that table partitioning
> makes splitting more slower due to the fact we cannot use indexes
I was just copying existing comments over for this part...

> would not be better getting tagsFolder from bookmarks service every time
> instead of caching it?
> we cannot be sure it will not change while the app is running (a
> backup/restore?)
That's bad, since nsNavHistory even caches it...
If that is the case, we need to fix everything else that caches it.

> +  _fixupURIText: function PAC_fixupURIText(aURIString)
> case-sensitive?
The old code was, and I don't want to change behavior.

> +    if (aRegisterObserver) {
> +      let pb = prefs.QueryInterface(Ci.nsIPrefBranch2);
> +      pb.addObserver("", this, false);
> why not observing only changes to "browser.urlbar." branch?
The prefs object is representing the branch for "browser.urlbar.", so it should only be observeing that.

> couldn't you filter out in the previous for loop as in the original code?
I found this code to be a cleaner and easier to understand what was going on.

> +    let filterJavascript = this._filterJavascript &&
> +                           this._currentSearchString.indexOf("javascript:") !=
> 0;
> what if i search "javaScript:" ? (apart that i'm crazy)
This is the same as the native code - didn't want to change behavior.

Fixed everything else.

Comment 6

[Ed Lee :Mardak]

(In reply to comment #5)
> > +  _fixupURIText: function PAC_fixupURIText(aURIString)
> > case-sensitive?
> The old code was, and I don't want to change behavior.

> > +    let filterJavascript = this._filterJavascript &&
> > +                           this._currentSearchString.indexOf("javascript:") !=
> > 0;
> > what if i search "javaScript:" ? (apart that i'm crazy)
> This is the same as the native code - didn't want to change behavior.
Actually, everything in the original code is touching stuff case insensitively ;) The user input is converted to lowercase and the backend shouldn't be storing "javaSCRIPT:" uris anyway -- uri("javaSCRIPT:").spec -> "javascript:"

cpp:
ToLowerCase(mOrigSearchString, mCurrentSearchString);
mCurrentSearchString = FixupURIText(mCurrentSearchString);

js:
    this._currentSearchString = this._fixupURIText(this._originalSearchString).toLocaleLowerCase();

So the ordering is important.

> Fixed everything else.
Did you mean to attach an updated patch?

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> > Fixed everything else.
> Did you mean to attach an updated patch?
Not until I did some benchmarks, but this is the latest version:
http://hg.mozilla.org/users/sdwilsh_shawnwilsher.com/storage-async/file/tip/places.async-location-bar

Comment 8

[Ed Lee :Mardak]

Comment on attachment 342004 [details] [diff] [review]
v0.3

>+  _getSearch: function PAC_getSearch(aTokens)
>+    // matchTitle and matchURL need to be set on the object - we use them later
>+    this._matchTitle = matchTitle;
>+    this._matchURL = matchURL;
I got suspicious here..

>+  _processRow: function PAC_processRow(aRow)
>+    // Make sure we match all of the filter requirements.  If a given restrict
>+    // is active, make sure a corresponding condition is *not* true.
>+    let matchAll = !((this._restictHistory && entryVisitCount == 0) ||
>+                     (this._restrictBookmark && !entryParentId) ||
>+                     (this._restrictTag && !entryTags));
this._restrict* should be saved as well :)

Comment 9

[Ed Lee :Mardak]

Comment on attachment 342004 [details] [diff] [review]
v0.3

>+  startSearch: function PAC_startSearch(aSearchString, aSearchParam,
>+    result.setSearchString = this._currentSearchString;
Noticed this while I was making a keyword search result thing. Function call not assignment.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v0.4

unbit-rotted, but it's missing stuff that has since landed with the autocomplete stuff.  I need to look at all that again, but wanted to get an updated patch in this bug.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Shawn, are you still targetting 1.9.1?

Comment 12

[[e-mail]]

Comment on attachment 347862 [details] [diff] [review]
v0.4

>+  startSearch: function PAC_startSearch(aSearchString, aSearchParam,
>+                                        aPreviousResult, aListener)
>+  {
...
>+    result.setSearchString = this._currentSearchString;
Apart from the fact that searchString is an attribute, not a method, the string should be aSearchString, i.e.
result.searchString = aSearchString;

Comment 13

[Ed Lee :Mardak]

It's actually a method in this case.. probably because the the base interface nsIAutoCompleteResult has a readonly attribute searchString and nsIAutoCompleteSimpleResult uses setSearchString.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/public/nsIAutoCompleteSimpleResult.idl#57

Comment 14

[[e-mail]]

Err, yes, sucky idl, but it's still the wrong string, which was my point...

Comment 17

[Shawn Wilsher :sdwilsh]

Attachment: v0.5

I'm posting this for the sake of getting something posted that compiles again.  I only addressed a few of the review comments, and this fails most of the autocomplete tests, but I did gut a bunch of code that isn't needed anymore.

Comment 18

[Shawn Wilsher :sdwilsh]

memo to self: updating _processRow to use _getBehavior should make test_empty_search.js pass.

memo to everyone else:  three test failing - two of which are a result of keyword searches not being properly implemented yet.  then I get to figure out what isn't tested (already have some XXX comments in code on things I think aren't tested, and I'm pretty sure are wrong as written)

Comment 19

[Shawn Wilsher :sdwilsh]

oh - also using the new query in bug 478097 because I don't see why that won't get r+.

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: v0.6

Passes all tests except for ones that need keywords to work.

Comment 21

[Shawn Wilsher :sdwilsh]

Attachment: v0.7

autocomplete/test_422277.js seems to randomly fail.  I'm not sure why yet.
unit/test_000_frecency.js seems to hang, and I have no idea why.

Almost in a reviewable state :)

Comment 22

[Shawn Wilsher :sdwilsh]

If anybody knows why nsTextToSubURI::UnEscapeURIForUI would sometimes return "ie?d" and other times return "site/%EAid" when given "site/%EAid", an explanation would be awesome.  It's causing random test failures for autocomplete/test_422277.js

Comment 23

[Shawn Wilsher :sdwilsh]

Immediately after posting comment 22, I decided to try changing the charset passed in from UTF-16 to UTF-8 like the C++ code.  Cannot get it to fail anymore.  I guess xpconnect is doing the conversion for me...

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: v0.8

Only fails test_adaptive.js now.  I had to add some code back to handle adaptive searches to nsNavHistory.

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: v0.9

Finally have all tests passing.  I'm fairly certain that this will not pass the new test in bug 479082 though.  It might be worth waiting for removing livemarks from moz_bookmarks, which mak will be working on.

I've done no performance testing here, and it's a pretty dumb implementation, but it does work.  In my testing, I didn't notice any slow downs, which is good.  This code should be pretty easy to get more performance wins out of too, so if it's already on par with the existing implementation, this is full of win.  I haven't done any benchmarking yet, but that is on tomorrow's agenda (luckily, I already have a pretty good test harness for this in bug 478097).

This patch is probably in a reviewable state, but I want to look it over myself again after stepping away for a bit to see if I spot anything else that is wrong.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Apple crash stack

Shawn, I did a test-run with your given tryserver build (http://tinyurl.com/dd46og). It already crashes for me on start after selecting a profile and before the browser window comes up. It doesn't happen with the latest Minefield nightly build. No idea how reliable the attached Apple Bug report is...

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 363241 [details] [diff] [review]
v0.9

I'm taking a look at this, looks interesting, some minor comment:

>+// This provides the parent folder (if the entry is a bookmark), if the entry is
>+// a bookmark, and any tags associated with the entry.

can you make this comment more clear while there, a list would be appreciated, something like
// This sql query fragments provide:
//   - the parent folder for bookmarked entries
//   - the bookmark title (can be used to check if entry is a bookmark)
//   - tags associated with a bookmarked entry

>+// match type constants
>+// Behavior constants
+// AutoComplete query type constants

Some additional comment upon these, would be cool

+ * Generates the SQL subquery to get the best favicon for a given revhost.

make explicit what we consider "best"

+function findOnBoundary(aToken, aSourceString)

this could maybe be made faster using regexps? i'm not sure char by char comparison is so fast

+  //////////////////////////////////////////////////////////////////////////////
+  //// nsIAutoCompleteSimpleResultListener
+
+  onValueRemoved: function PAC_onValueRemoved(aResult, aValue, aRemoveFromDB)
+  {

aValue is a bit generic, should be aURIString in our case

+  handleError: function PAC_handleError(aError)
+  {
+    Components.utils.reportError(aError);
+  },

maybe add a "PlacesAutoComplete: " prefix to the error

+  handleCompletion: function PAC_handleCompletion(aReason)
+  {
+    // If we do not have enough results, and our match type is
+    // MATCH_BOUNDARY_ANYWHERE, process the discarded results to get more.
+    let result = this._result;
+    let maxResults = this._maxRichResults;
+    if (this._matchBehavior == MATCH_BOUNDARY_ANYWHERE &&
+        result.matchCount < maxResults && !this._secondPass) {
+      this._secondPass = true;
+      let rows = this._discardedResults;
+      for (let i = 0; i < rows.length && result.matchCount < maxResults; i++)
+        this._processRow(rows[i]);
+    }
+
+    this._finishSearch(aReason != Ci.mozIStorageStatementCallback.REASON_ERROR);
+  },

I have a doubt about this, handleCompletion is called also with REASON_CANCEL, there's no risk that when we call _finishSearch to cancel, this gets called again or when it's not needed?

+  _addToResults: function PAC_addToResults(aPlaceId, aURISpec, aTitle, aStyle)
+  {

+    let uri = this._ioService.newURI(aURISpec, null, null);
+    let favicon = this._faviconService.getFaviconLinkForIcon(uri);
+
+    this._result.appendMatch(aURISpec, aTitle, favicon.spec, aStyle);
+  },

another case where bug 389090 would make sense, i think is worth fixing that bug, we have many parts of the ui that would benefit from eradicating the useless nsIURI wrap, so i second Seth's idea to provide getFaviconSpecForIconString in the interface.

+  QueryInterface: XPCOMUtils.generateQI([
+    Ci.nsIAutoCompleteSearch,
+    Ci.nsIAutoCompleteSimpleResultListener,
+    Ci.mozIStorageStatementCallback,
+  ])

this is also a nsIObserver isn't it?

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> Created an attachment (id=363271) [details]
> Apple crash stack
I've pinged joe drew about this, since I don't see how it's even possible for my changes to cause this.

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> +function findOnBoundary(aToken, aSourceString)
> 
> this could maybe be made faster using regexps? i'm not sure char by char
> comparison is so fast
Regular expressions are also slow (first to compile, then to search), so that'd debatable.  I'd like to avoid doing that for now.

> I have a doubt about this, handleCompletion is called also with REASON_CANCEL,
> there's no risk that when we call _finishSearch to cancel, this gets called
> again or when it's not needed?
handleResult also cancels when we have enough results.  We should check to make sure that this._result exists though.  If it doesn't, handleCompletion shouldn't do anything.

Comment 30

[Shawn Wilsher :sdwilsh]

Attachment: v0.10

A few more small changes.

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: v0.11

Bah - the bustage people are seeing might be a result of the new file being gone from packages-static.  I've added that in this patch, and pushed a new build to the try server.

Comment 32

[Ed Lee :Mardak]

Comment on attachment 363439 [details] [diff] [review]
v0.11

>+  this.__defineGetter__("_db", function() {
>+    delete this._db;
>+    return this._db = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                      getService(Ci.nsPIPlacesDatabase).
>+                      DBConnection;
>+  });
>...
I haven't actually tried running this but did some dynamic getter definition for the privacy pref pane.

// Make a smart getter
let makeSmart = function(aProp, aWrapped) this.__defineGetter__(aProp, function() {
  delete this[aProp];
  return this[aProp] = aWrapped();
});

// Getter for service with an optional attribute
let smartService = function(aProp, aId, aInterface, aAttr) makeSmart(aProp, function() let (service = Cc["@mozilla.org/" + aId].getService(Ci[aInterface])) aAttr ? service[aAttr] : service);

// Getter for creating query statements
let smartQuery = function(aProp, aQuery) makeSmart(aProp, function() this._db.createStatement(aQuery));

// Getter for creating a query from the base query
let smartBase = function(aProp, aReplace) smartQuery(aProp, function() SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", aReplace ? "AND " + aReplace : "", "g"));

smartService("_db", "browser/nav-history-service;1", "nsPIPlacesDatabase", "DBConnection");
smartService("_bh", "browser/global-history;2", "nsIBrowserHistory");
smartService("_textURIService", "intl/texttosuburi;1", nsITextToSubURI);
smartService("_bs", "browser/nav-bookmarks-service;1", nsINavBookmarksService);
smartService("_ioService", "network/io-service;1", nsIIOService);
smartService("_faviconService", "browser/favicon-service;1", nsIFaviconService);
smartQuery("_adaptiveQuery", "...");
smartQuery("_keywordQuery", "...");
smartBase("_defaultQuery", "");
smartBase("_historyQuery", "h.visit_count > 0");
smartBase("_starQuery", "bookmark IS NOT NULL");
smartBase("_tagsQuery", "tags IS NOT NULL");
smartBase("_typedQuery", "h.typed = 1");

Let the computer do the copy/pasting work! ;)

>+nsPlacesAutoComplete.prototype = {
>+  startSearch: function PAC_startSearch(aSearchString, aSearchParam,
>...
>+    this._currentSearchString = this._fixupURIText(this._originalSearchString)
>+                                    .toLowerCase();
The original code did lowercase before fixupURIText. This could affect user typing in "HTTP://" for some reason...

>+    // If we are not enabled, we need to return now.
>+    if (!this._enabled) {
>+      this._finishSearch(true);
>+      return;
>+    }
>+
>+    // Notify the listener that results are pending (or if we are not enabled,
>+    // that we are done)...
>+    aListener.onSearchResult(this, result);
Wrong comment now that you moved it down past the return

>+  QueryInterface: XPCOMUtils.generateQI([
>+    Ci.nsIAutoCompleteSearch,
>+    Ci.nsIAutoCompleteSimpleResultListener,
>+    Ci.mozIStorageStatementCallback,
>+    Ci.nsIObserver,
Is this alphabetical? ;) moz before ns!

>+function findAnywhere(aToken, aSourceString)
>+  return aSourceString.toLowerCase()
>+                      .indexOf(aToken.toLowerCase()) != -1;
aToken is already lowercase

>+function findBeginning(aToken, aSourceString)
>+  return aSourceString.substr(0, aToken.length).toLowerCase() ==
>+           aToken.toLowerCase();
Same here

Oh and what's up with the naming convention for best_favicon_for_revhost and sql_str_fragment.

But I suppose it could be something like..

const BOOK_TAG_SQL = let (fragThing = function(..) ...) fragThing(...) + fragThing...

But that might look pretty odd...

Comment 33

[Ed Lee :Mardak]

Comment on attachment 363439 [details] [diff] [review]
v0.11

>+    function safeGetter(aName, aType, aDefault) {
>+      // If the pref isn't set, we want to use the default.
>+      try {
>+        return prefs["get" + aType + "Pref"](aName);
You can actually just infer the type:
let type = typeof aDefault == "number" ? "Int" : typeof aDefault == "boolean" ? "Bool" : "Char";

>+    // register observer
>+    if (aRegisterObserver) {
>+      let pb = prefs.QueryInterface(Ci.nsIPrefBranch2);
>+      pb.addObserver("", this, false);
Oh? Does this just notify whenever anything under the pref branch changes? I suppose we might run into issues if we rerun stuff when loading prefs...

But I just noticed that you don't do the previous query optimization. We needed to clear out the CurrentSearchString on pref change to avoid the optimization from being triggered because the preferences might have invalidated the previous results.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shawn, I did a further test. This time with the latest tryserver build. Now the crash is gone. The following things I've noticed:

* For each character you type into the location bar the autocomplete popup is reopened again which causes massive flickering.

* When I start the build with a profile which was used in a regularly Shiretoko build before, I'm able to get the search action into an infinite loop. Simply entering "B" displays a list with proposed results but never stops searching. Entering one more character and the autocomplete popup isn't displayed anymore. I've stopped the search after some minutes without any results displayed. Quitting the browser doesn't work. It completely hangs on shutdown. The file sizye is

* Even in the current state its a great enhancement for me. With a 26MB places.sqlite file the search results are displayed without any hang in the ui thread. It looks fantastic!

Comment 35

[Ed Lee :Mardak]

(In reply to comment #34)
> * For each character you type into the location bar the autocomplete popup is
> reopened again which causes massive flickering.
Thanks for reminding me. Yeah because we do onSearchResult for every result, the popup keeps invalidating itself and readding the results. I was thinking about changing the autocomplete popup code for a separate reason outside of this async rewrite.

The popup could benefit from a delayed invalidate where it waits a little bit before getting rid of all of its results then repopulating. I suppose the popup code could get a rewrite of its own somewhere along the way..

Comment 36

[Ed Lee :Mardak]

I just noticed that for profiles that don't show any results while typing, I get these in the error console:

Error: [xpconnect wrapped mozIStorageError]
Source File: Minefield.app/Contents/MacOS/components/nsPlacesAutoComplete.js
Line: 539

And eventually there's..
Error: Async statement execution returned with '11', 'database disk image is malformed'
Source File: Minefield.app/Contents/MacOS/components/nsPlacesDBFlush.js
Line: 210

I'm guessing the nsPlacesAutoComplete.js is throwing the same error, but the debug code there doesn't expand the message.

Comment 37

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> Thanks for reminding me. Yeah because we do onSearchResult for every result,
> the popup keeps invalidating itself and readding the results. I was thinking
> about changing the autocomplete popup code for a separate reason outside of
> this async rewrite.
> 
> The popup could benefit from a delayed invalidate where it waits a little bit
> before getting rid of all of its results then repopulating. I suppose the popup
> code could get a rewrite of its own somewhere along the way..
Are you saying that the current popup code for autocomplete invalidates the whole popup and not just the parts that change when new results are given?  pain...

(In reply to comment #36)
> Error: Async statement execution returned with '11', 'database disk image is
> malformed'
> Source File: Minefield.app/Contents/MacOS/components/nsPlacesDBFlush.js
> Line: 210
More pain.  That's SQLITE_CORRUPT :(
This patch shouldn't be doing anything to corrupt the database, so that means you should be hitting that error for normal builds too.  It just happens that we get hit harder with async?

Comment 38

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #37)
> Are you saying that the current popup code for autocomplete invalidates the
> whole popup and not just the parts that change when new results are given?

bug 393902

Comment 39

[Shawn Wilsher :sdwilsh]

Checked the build logs.  The builds that went to the try server did not have the packages-static fix, which means the try server dropped my second changeset.  That also explains why people who had no AutoComplete in their builds.

Comment 40

[Ed Lee :Mardak]

Comment on attachment 363439 [details] [diff] [review]
v0.11

>+  handleResult: function PAC_handleResult(aResultSet)
When does this get called? Every time async decides it has enough results, it'll call handleResult? What order do we process them in? Javascript is single threaded (for a given script?).. relating to..

>+  {
>+    let row, haveMatches = false;
>+    while (row = aResultSet.getNextRow()) {
How many rows do we typically have in a result? Could this be many? This would block javascript calls coming in as well.. like stopSearch? So we wouldn't know to stop because we're too busy processing results..

>+      let match = this._processRow(row);
>+      if (!match && this._matchBehavior == MATCH_BOUNDARY_ANYWHERE) {
>+        // Cache the row in case we need to search again with a less strict
>+        // search function to get enough results
>+        this._discardedResults.push(row);
>+      }
>+      haveMatches = haveMatches || match;
>+
>+      if (this._result.matchCount == this._maxRichResults) {
>+        // Cancel this statement and then exit gracefully.
>+        this._pendingQuery.cancel();
>+        delete this._pendingQuery;
Why this separate logic? Why can't we just use _finishSearch(false)? Additionally, if we found a new result, we don't send a RESULT_SUCCESS (not ONGOING)?
>+        return;
>+      }
>+
>+    }
>+
>+    // Notify about results if we've gotten them.
>+    let (result = this._result) {
>+      if (haveMatches) {
>+        result.setSearchResult(Ci.nsIAutoCompleteResult.RESULT_SUCCESS_ONGOING);
>+        this._listener.onSearchResult(this, result);
So we actually only send this after a "batch" of rows not per result. Should this be inside the loop?

>+  handleCompletion: function PAC_handleCompletion(aReason)
>+  {
>+    // If we have no result object, we've already notified that we are done.
>+    let result = this._result;
>+    if (!result)
>+      return;
>+
>+    // If we do not have enough results, and our match type is
>+    // MATCH_BOUNDARY_ANYWHERE, process the discarded results to get more.
>+    let maxResults = this._maxRichResults;
>+    if (this._matchBehavior == MATCH_BOUNDARY_ANYWHERE &&
>+        result.matchCount < maxResults && !this._secondPass) {
>+      this._secondPass = true;
>+      let rows = this._discardedResults;
>+      for (let i = 0; i < rows.length && result.matchCount < maxResults; i++)
>+        this._processRow(rows[i]);
Again, this could be potentially huge to process, yeah?

>+  _finishSearch: function PAC_finishSearch(aFinished)
>+  {
>+    // Notify about results if we should
>+    if (aFinished) {
>+      let result = this._result.matchCount ?
>+        Ci.nsIAutoCompleteResult.RESULT_SUCCESS :
>+        Ci.nsIAutoCompleteResult.RESULT_NOMATCH;
>+      this._result.setSearchResult(result);
>+      this._listener.onSearchResult(this, this._result);
We would want to always let the listener know when we stop -- even if it's user triggered stopping or we got enough rows stopping. Not only when we run to the end of async completion.

These multithreaded UI as well as multithreaded? backend could lead to problems for a single thread processing this script. The UI can trigger a stop as well as async triggering multiple results?

Comment 41

[Ed Lee :Mardak]

(In reply to comment #37)
> > Error: Async statement execution returned with '11', 'database disk image is
> > malformed'
> More pain.  That's SQLITE_CORRUPT :(
I think I just did something bad when testing.. perhaps force quitting on an async build. I put in a copy of my usual places.sqlite and it's "fine" again. (As in no corrupt errors, but it still doesn't show any results.)

Comment 42

[Shawn Wilsher :sdwilsh]

(In reply to comment #40)
> When does this get called? Every time async decides it has enough results,
> it'll call handleResult? What order do we process them in? Javascript is single
> threaded (for a given script?).. relating to..
We process them in the order SQLite returns them, and do so on the main thread.  For details on how storage decides to give us data, see http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageEvents.cpp#57

> How many rows do we typically have in a result? Could this be many? This would
> block javascript calls coming in as well.. like stopSearch? So we wouldn't know
> to stop because we're too busy processing results..
Other events have the opportunity to come in.  See the previous link.

> Why this separate logic? Why can't we just use _finishSearch(false)?
> Additionally, if we found a new result, we don't send a RESULT_SUCCESS (not
> ONGOING)?
That's a bug - we really want to stop processing rows.  Also, don't call _finishSearch because it clears out all of our internal state as well.  handleCompletion will be called very soon anyway as a result of this.

> So we actually only send this after a "batch" of rows not per result. Should
> this be inside the loop?
No - I kept it outside deliberately.  I suppose we could speed up the first result, but I suspect that the difference would be negligible and not noticed by the user at all.  I figured calling back after every result was a bit much.


> >+    // If we do not have enough results, and our match type is
> >+    // MATCH_BOUNDARY_ANYWHERE, process the discarded results to get more.
> >+    let maxResults = this._maxRichResults;
> >+    if (this._matchBehavior == MATCH_BOUNDARY_ANYWHERE &&
> >+        result.matchCount < maxResults && !this._secondPass) {
> >+      this._secondPass = true;
> >+      let rows = this._discardedResults;
> >+      for (let i = 0; i < rows.length && result.matchCount < maxResults; i++)
> >+        this._processRow(rows[i]);
> Again, this could be potentially huge to process, yeah?
yeah...hadn't thought of that.  There isn't really a good way to break this up easily.  Needs investigation (although, I have other ideas about how to do this such that we don't even need to store discarded results.  That wont' happen in this bug though).

> We would want to always let the listener know when we stop -- even if it's user
> triggered stopping or we got enough rows stopping. Not only when we run to the
> end of async completion.
I sure wish that was documented somewhere :/

> These multithreaded UI as well as multithreaded? backend could lead to problems
> for a single thread processing this script. The UI can trigger a stop as well
> as async triggering multiple results?
I'm not entirely sure what all these questions are about, but the only choke point should be the processing of discarded results.  Casual testing of the builds agrees with this.

Comment 43

[Ed Lee :Mardak]

(In reply to comment #42)
> http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageEvents.cpp#57
Does the eventual ..Thread->Dispatch happen non-blocking? There could be a bunch of result sets being dispatched?

> > block javascript calls coming in as well.. like stopSearch?
> Other events have the opportunity to come in.  See the previous link.
Well, at least for me, the location bar keeps showing a spinning throbber... but that might be the below problems..

> > Additionally, if we found a new result, we don't send a RESULT_SUCCESS (not
> > ONGOING)?
> That's a bug - we really want to stop processing rows.
Yeah, RESULT_SUCCESS (or NOMATCH) causes the UI to stop showing the throbber.
> handleCompletion will be called very soon anyway as a result of this.
Fair enough.
 
> > So we actually only send this after a "batch" of rows not per result.
> No - I kept it outside deliberately.
Well, async results gives back 15 rows at most, so batching should be fine.

> > We would want to always let the listener know when we stop
> I sure wish that was documented somewhere :/
Well, there's this describing the different result statuses:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl#41

> > These multithreaded UI as well as multithreaded? backend could lead to problems
> > for a single thread processing this script. The UI can trigger a stop as well
> > as async triggering multiple results?
> I'm not entirely sure what all these questions are about, but the only choke
> point should be the processing of discarded results.  Casual testing of the
> builds agrees with this.
Well, I've been running into ~200% cpu constantly and I don't see any results, so I figured it's too busy processing all the places in my history. And it wasn't stopping if I stopped the search by blurring the location bar.

Comment 44

[Shawn Wilsher :sdwilsh]

(In reply to comment #43)
> Does the eventual ..Thread->Dispatch happen non-blocking? There could be a
> bunch of result sets being dispatched?
As long as you don't use DISPATCH_SYNC, yes.  Each thread has an event queue that it processes.

> Well, async results gives back 15 rows at most, so batching should be fine.
Right, but there is no point for dispatching it for each result we can get back.  We can wait until we process at least 15 of the results.

> Well, there's this describing the different result statuses:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl#41
Right, but nowhere is it documented that if the user cancels the search that we should notify the results that we are done.  That wasn't clear to me that we need to do that.


> Well, I've been running into ~200% cpu constantly and I don't see any results,
> so I figured it's too busy processing all the places in my history. And it
> wasn't stopping if I stopped the search by blurring the location bar.
I have dump statements in the code at the start of startSearch and stopSearch, so you can see if it's being called.  I don't see how I'm doing anything differently from before, so the only way we'd not be getting stopSearch is if the AutoComplete controller isn't telling us.
If you do enable those dump statements, you'll also find that we kick off a new search for every character typed, regardless of how fast you type it.  I haven't had a chance to look if there's a bug about that already (I suspect there is), but that blocks this as well.

Comment 45

[Shawn Wilsher :sdwilsh]

Attachment: v0.12

I think this will fix the hanging folks were seeing, but not totally sure.  Got a stack of the shutdown hang from Mook on irc, and this should prevent us from getting into that state.  I'm not sure if the hang is a SQLite bug or storage bug though.  This code shouldn't have caused it.

I've pushed this to the try server, but that'll take a while.  You can get it in add-on form here:
https://services.forerunnerdesigns.com/extensions/get/[e-mail]/0.12/
I've added an updateURL to the add-on, so you'll update as I update the patch.

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #45)
> I've pushed this to the try server, but that'll take a while.  You can get it
> in add-on form here:
> https://services.forerunnerdesigns.com/extensions/get/[e-mail]/0.12/

This gives only a blank page.

Comment 47

[Shawn Wilsher :sdwilsh]

(In reply to comment #46)
> This gives only a blank page.
It does the same for me if I click the link through gmail, but works fine from bugzilla.

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shawn, I've tested your add-on and I've noticed that when I enter a new url
inside the location bar and press enter - nothing happens for about 10-20
seconds. After that the page gets loaded. I had to stop further testing and
deactivated the add-on for now.

Comment 49

[Shawn Wilsher :sdwilsh]

Attachment: v0.14

Forgot to upload v0.13 of this patch, but released the add-on for it.  Oops.  This contains the fixes from 0.13, as well as the query from bug 479739.

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

My problem from comment 48 is still there. Trying to open websites which are probably not in the local DNS cache let the browser wait for about 10-15s until the page will be loaded.

Comment 51

[Ed Lee :Mardak]

Using addon version 14 and dietrich's places.sqlite:
async.: Finish:  16597,  17049,  17578

For comparison..
feb 14: Finish: 108990, 109859, 110970
opt #1: Finish:  63764,  64144,  64640
opt #2: Finish:  22426,  22724,  23543

I do see a definite impact on the first result when actually typing in the location bar, but that's probably related to the 100ms timeout before searching.

The following results are from my own profile:
Query: mozilla First: 11,24,45 Finish: 263,271,283
Query: mozilla bugzilla First: 8,10,13 Finish: 286,297,306
Query: mozilla bugzilla fast First: 9,10,12 Finish: 13995,14318,14682

Compared to the results in bug 479739 comment 1:
Query: mozilla First: 16,16,16 Finish: 16,16,16
Query: mozilla bugzilla First: 7,8,12 Finish: 107,118,135
Query: mozilla bugzilla fast First: 131,141,150 Finish: 15026,15086,15175

So some reason async stops searching later, but can get results pretty fast.

Comment 53

[Shawn Wilsher :sdwilsh]

Attachment: v0.15

This is an unbit-rotted v0.14

Comment 54

[Shawn Wilsher :sdwilsh]

Attachment: v0.16

Newish approach.  This does not pass tests yet.

Comment 55

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This passes tests locally.  I think it's about time we kick off the review process.  Try server builds will be posted soonish.

Comment 56

[Ed Lee :Mardak]

Comment on attachment 387001 [details] [diff] [review]
v1.0

>+++ b/browser/base/content/browser.xul
>@@ -367,6 +367,7 @@
>                  maxrows="6"
>+                 timeout="100"
Is this still necessary to double the timeout from 50 to 100? I thought you added this in because we weren't processing things fast enough, but now we have a SQL function.. ?

+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
>+// this is just a helper for the next constant
>+function book_tag_sql_fragment(aName, aColumn, aForTag)
>+{
>+  return "(" +
>+    "SELECT " + aColumn + " " +
>+    "FROM moz_bookmarks b " +
>+    "JOIN moz_bookmarks t " +
>+      "ON t.id = b.parent " +
>+      "AND t.parent " + (aForTag ? "" : "!") + "= :parent " +
>+    "WHERE b.type = " + Ci.nsINavBookmarksService.TYPE_BOOKMARK + " " +
>+      "AND b.fk = h.id " +
>+    (aForTag ? "AND LENGTH(t.title) > 0" :
>+               "ORDER BY b.lastModified DESC LIMIT 1") +
>+  ") AS " + aName;
You could just wrap a bunch of strings in an array and .join(" ") ;) Saves a + " " + or a .. " + for each line.

>+  this.__defineGetter__("_bh", function() {
>+    delete this._bh;
>+    return this._bh = Cc["@mozilla.org/browser/global-history;2"].
>+                      getService(Ci.nsIBrowserHistory);
>+  });
Is there a utils module to do this lazy getter for services? You could make things so much more compact! :)

>+  startSearch: function PAC_startSearch(aSearchString, aSearchParam,
>+    this._currentSearchString = this._fixupURIText(this._originalSearchString)
>+                                    .toLowerCase();
The original code had "toLowerCase()" first, which could affect things if the user has caps locks on and types HTTP://..

>+    result.setSearchResult(Ci.nsIAutoCompleteResult.RESULT_NOMATCH_ONGOING);
>+    this._result = result;
>+
>+    // If we are not enabled, we need to return now.
>+    if (!this._enabled) {
>+      this._finishSearch(false);
>+      return;
>+    }
Are we not doing previous query results for good or we'll see if we still need it for optimizations? But then again, with much more in js, it should be easier to write optimization code such as the caching of single letter searches.. (But then again, cached single letter helps a lot more for "prev query" if we can start from the cache when we go to 2 letters.)

>+    let [query, tokens] = this._getSearch(..
I'm starting to like the "let {query, tokens} =" syntax...

+  _getSearch: function PAC_getSearch(aTokens)
+    return [this._getBoundSearchQuery(this._matchBehavior, aTokens), aTokens];
return {
  query: this._getBoundSearchQuery(..),
  tokens: aTokens
};

>+  observe: function PAC_observe(aSubject, aTopic, aData)
>+      // Finalize the statements that we have used.
>+      let stmts = [
>+        "_defaultQuery",
>+        "_historyQuery",
>+        "_starQuery",
>+        "_tagsQuery",
>+        "_typedQuery",
>+        "_adaptiveQuery",
>+        "_keywordQuery",
Would it make sense to store this array as part of |this|? Where the array is the actual query and not just a string. this._queries["default"].finalize(), etc. You could still have the smart getters for |this|, but it'll also add the query to the array. So any entry must have been lazily gotten before.

>+  _fixupURIText: function PAC_fixupURIText(aURIString)
>+  {
>+    let uri = aURIString;
>+
>+    if (uri.indexOf("http://") == 0)
>+      uri = uri.slice(7);
>+    else if (uri.indexOf("https://") == 0)
>+      uri = uri.slice(8);
>+    else if (uri.indexOf("ftp://") == 0)
>+      uri = uri.slice(6);
uri = aURIString.replace(/^(https?|ftp):\/\//, "")

>+  _getUnfilteredSearchTokens: function PAC_unfilteredSearchTokens(aSearchString)
>+    return aSearchString.length ? aSearchString.split(" ") : [];
I don't know what's the explicit vs implicit policy nowadays.. but "str.length > 0" vs just "str.length (not falsy)"?

>+  _finishSearch: function PAC_finishSearch(aCancel)
>+    // Clear our state
>+    delete this._originalSearchString;
>+    delete this._currentSearchString;
>+    delete this._searchTokens;
>+    delete this._listener;
>+    delete this._result;
>+    delete this._usedPlaceIds;
>+    delete this._pendingQueries;
Similar to the array-ing queries, it could be useful to have an array of state to group together those related values. But I suppose having this list here also groups them together... :p

>+    function safeGetter(aName, aType, aDefault) {
>+        return prefs["get" + aType + "Pref"](aName);
You could save an arg by doing typeof aDefault -> "Bool", "Int", "Char"

>+  _getSearch: function PAC_getSearch(aTokens)
>+      if (token == this._restrictHistoryToken)
>+        this._setBehavior(kBehaviorHistory);
>+      else if (token == this._restrictBookmarkToken)
>+        this._setBehavior(kBehaviorBookmark);
>+      else if (token == this._restrictTagToken)
>+        this._setBehavior(kBehaviorTag);
>+      else if (token == this._matchTitleToken)
>+        this._setBehavior(kBehaviorTitle);
>+      else if (token == this._matchURLToken)
>+        this._setBehavior(kBehaviorUrl);
>+      else if (token == this._restrictTypedToken)
>+        this._setBehavior(kBehaviorTyped);
>+      else
>+        removeToken = false;
Array of Tokens! ;) removeToken = tokens.some(function(flag) {
  if (token == flag) {
    this._setBehavior(flag);
    return true;
  }
  return false;
}, this);

>+  _processRow: function PAC_processRow(aRow)
>+    let showTags = entryTags ? true : false;
showTags = !!entryTags ?

>+  _inResults: function PAC_inResults(aPlaceId)
>+    return (aPlaceId in this._usedPlaceIds);
unnecessary parens?

>+  _addToResults: function PAC_addToResults(aPlaceId, aURISpec, aTitle,
>+    // Obtain the favicon for this URI.
>+    if (aFaviconSpec) {
>+      let uri = this._ioService.newURI(aFaviconSpec, null, null);
>+      var favicon = this._faviconService.getFaviconLinkForIcon(uri).spec;
>+    }
>+    else
>+      var favicon = this._faviconService.defaultFavicon.spec;
let favicon = this._faviconService.defaultFavicon.spec;
if (aFaviconSpec)
  favicon = this._faviconService...spec ?

>+  _getBehavior: function PAC_getBehavior(aType)
>+    return (this._behavior & aType);
parens

>+++ b/toolkit/components/places/src/nsPlacesSQLFunctions.cpp
>+  MatchAutoCompleteFunction::fixupURISpec(const nsDependentCString &aURISpec)
Do we have a comment linking this and the js implementation?

>+  MatchAutoCompleteFunction::OnFunctionCall(mozIStorageValueArray *aArguments,
>+    // Check JavaScript URIs - right now we do this in the JS end.  Doing it
>+    // here would require us to pass in the preference to the function.
>+    // TODO do we want to do this in C++?
We could pass it in as another arg to MATCH_AUTOCOMPLETE like we do for the other behaviors. Or make it part of the behavior somehow?

Comment 57

[Shawn Wilsher :sdwilsh]

(In reply to comment #56)
> Is this still necessary to double the timeout from 50 to 100? I thought you
> added this in because we weren't processing things fast enough, but now we have
> a SQL function.. ?
Probably not needed anymore.  Removed.

> Is there a utils module to do this lazy getter for services? You could make
> things so much more compact! :)
No, there is no such module that I know of.

> Are we not doing previous query results for good or we'll see if we still need
> it for optimizations? But then again, with much more in js, it should be easier
> to write optimization code such as the caching of single letter searches.. (But
> then again, cached single letter helps a lot more for "prev query" if we can
> start from the cache when we go to 2 letters.)
I think we should save it for a follow-up if it's a problem.  Performance tests (in a bug blocking this) will tell us if we really really need to worry about this.

> >+    let [query, tokens] = this._getSearch(..
> I'm starting to like the "let {query, tokens} =" syntax...
So it creates an object instead of an array?  I kinda like to use variables instead of an object (unless that outputs into two different variables?)

> Would it make sense to store this array as part of |this|? Where the array is
> the actual query and not just a string. this._queries["default"].finalize(),
> etc. You could still have the smart getters for |this|, but it'll also add the
> query to the array. So any entry must have been lazily gotten before.
Are you saying that each smarter getter would add the query to the array?  I think it doesn't make sense to have that extra bookkeeping for something we can easily check when we shut down.

> uri = aURIString.replace(/^(https?|ftp):\/\//, "")
regular expressions are slow - especially ones that have capturing parens since they can't even be traced by the regular expression tracer.  The current code is also clearer IMHO.
 
> I don't know what's the explicit vs implicit policy nowadays.. but "str.length
> > 0" vs just "str.length (not falsy)"?
0 evaluates as false, so I'm not sure it matters.

> Similar to the array-ing queries, it could be useful to have an array of state
> to group together those related values. But I suppose having this list here
> also groups them together... :p
I'm gonna leave it grouped here - don't need the extra memory overhead.

> Array of Tokens! ;)
I can't make this look nice, so I'm going to keep it the way it is in the current patch.

> >+    return (aPlaceId in this._usedPlaceIds);
> unnecessary parens?
I find this to be more readable with parens.

> let favicon = this._faviconService.defaultFavicon.spec;
> if (aFaviconSpec)
>   favicon = this._faviconService...spec ?
That means we always have to get that property (we could cache it I suppose) though.  I'd rather not do that.

> >+    return (this._behavior & aType);
> parens
ditto on readability

> Do we have a comment linking this and the js implementation?
They are slightly different and used differently though.  I'm changing the name of the one in the JS file.  We have unit tests covering this code though - if it gets messed up they fail pretty hard.

> We could pass it in as another arg to MATCH_AUTOCOMPLETE like we do for the
> other behaviors. Or make it part of the behavior somehow?
Yeah - just not sure if we want to do that here or not.  Adding it to the behaviors isn't a bad idea.  I'll see what Marco thinks about this.

Comment 58

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses a few more of Mardak's comments, as well as a bunch I got on irc.

Comment 59

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 387001 [details] [diff] [review]
v1.0

these comments are for the previous patch, it took some time, i'll check the interdiff for the new version.

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -89,19 +90,19 @@
> #include "nsIClassInfoImpl.h"
> #include "nsThreadUtils.h"
> 
>-#include "mozIStorageConnection.h"
>-#include "mozIStorageFunction.h"
>-#include "mozIStoragePendingStatement.h"
>-#include "mozIStorageService.h"
>-#include "mozIStorageStatement.h"
>-#include "mozIStorageValueArray.h"
>-#include "mozStorageCID.h"
>-#include "mozStorageHelper.h"
>-#include "mozIStorageError.h"

> #include "nsAppDirectoryServiceDefs.h"

attach to the includes above, does not need to be particularly noticeable

>+using namespace mozilla::places;
>+

actually if the methods are only used in the file they are defined we could use an empty named namespace to limit them to this file, and looks like is the case. Do we want to be so explicit for the small utils that are defined there?

>@@ -1148,11 +1085,15 @@ nsNavHistory::InitViews()
> nsresult
> nsNavHistory::InitFunctions()
> {
>-  nsresult rv;
>-
>-  rv = mDBConn->CreateFunction(
>-      NS_LITERAL_CSTRING("get_unreversed_host"), 1, 
>-      new mozStorageFunctionGetUnreversedHost);
>+  nsCOMPtr<mozIStorageFunction> func =
>+    new mozStorageFunctionGetUnreversedHost;
>+  NS_ENSURE_TRUE(func, NS_ERROR_OUT_OF_MEMORY);
>+  nsresult rv = mDBConn->CreateFunction(
>+    NS_LITERAL_CSTRING("get_unreversed_host"), 1, func
>+  );
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = MatchAutoCompleteFunction::create(mDBConn);
>   NS_ENSURE_SUCCESS(rv, rv);

why is initing those 2 functions so different, could you make them use a common pattern/helper?

>+#ifdef MOZ_XUL
>+
>+// Used to notify a topic to system observers on async execute completion.
>+// Will throw on error.
>+class AutoCompleteStatementCallbackNotifier : public mozIStorageStatementCallback

can we put this into the namespace?

>+nsresult
>+nsNavHistory::AutoCompleteFeedback(PRInt32 aIndex,
>+                                   nsIAutoCompleteController *aController)
>+{
>+  // we don't track user choices in the awesomebar in private browsing mode

while here ucfirst and end comment with .

>+  if (InPrivateBrowsingMode())
>+    return NS_OK;
>+
>+  mozIStorageStatement *stmt = GetDBFeedbackIncrease();
>+  mozStorageStatementScoper scope(stmt);
>+
>+  nsAutoString input;
>+  nsresult rv = aController->GetSearchString(input);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = stmt->BindStringParameter(0, input);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsAutoString url;
>+  rv = aController->GetValueAt(aIndex, url);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = stmt->BindStringParameter(1, url);

urls are usually considered utf8 all around and saved as such, i guess if this would not need to be converted and binded as UTF8 String.

>+mozIStorageStatement*
>+nsNavHistory::GetDBFeedbackIncrease()
>+{
>+  if (mDBFeedbackIncrease)
>+    return mDBFeedbackIncrease;
>+
>+  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+    // Leverage the PRIMARY KEY (place_id, input) to insert/update entries

while here ucfirst and end comment with .

>+    "INSERT OR REPLACE INTO moz_inputhistory "
>+      // use_count will asymptotically approach the max of 10

ditto, i won't comment further on this, if you want to cleanup comments formatting in touched code, please do :)


>diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h
>--- a/toolkit/components/places/src/nsNavHistory.h
>+++ b/toolkit/components/places/src/nsNavHistory.h
>@@ -54,11 +54,6 @@
> #include "nsPIPlacesHistoryListenersNotifier.h"
> #ifdef MOZ_XUL
> #include "nsIAutoCompleteController.h"

is this still needed in the header or can we include this in the cpp instead?

>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
>@@ -0,0 +1,947 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ * vim: sw=2 ts=2 sts=2 expandtab
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2008

2009

>+// this is just a helper for the next constant

ucfirst, end with .
Even if this is just an helper, what about a javadoc comment?

>+function book_tag_sql_fragment(aName, aColumn, aForTag)
>+{
>+  return "(" +
>+    "SELECT " + aColumn + " " +
>+    "FROM moz_bookmarks b " +
>+    "JOIN moz_bookmarks t " +
>+      "ON t.id = b.parent " +
>+      "AND t.parent " + (aForTag ? "" : "!") + "= :parent " +
>+    "WHERE b.type = " + Ci.nsINavBookmarksService.TYPE_BOOKMARK + " " +
>+      "AND b.fk = h.id " +
>+    (aForTag ? "AND LENGTH(t.title) > 0" :
>+               "ORDER BY b.lastModified DESC LIMIT 1") +
>+  ") AS " + aName;
>+}
>+
>+// This SQL query fragment provides the following:
>+//   - the parent folder for bookmarked entries (kQueryIndexParent)
>+//   - the bookmark title, if it is a bookmark (kQueryIndexBookmarkTitle)
>+//   - the tags associated with a bookmarked entry (kQueryIndexTags)
>+const BOOK_TAG_SQL =
>+  book_tag_sql_fragment("parent", "b.parent", false) + ", " +
>+  book_tag_sql_fragment("bookmark", "b.title", false) + ", " +
>+  book_tag_sql_fragment("tags", "GROUP_CONCAT(t.title, ',')", true);
>+
>+// observer topics
>+const kQuitApplication = "quit-application";
>+const kPrefChanged = "nsPref:changed";
>+
>+// Match type constants.  These indicate what type of search function we should
>+// be using.
>+const MATCH_ANYWHERE = 0;
>+const MATCH_BOUNDARY_ANYWHERE = 1;
>+const MATCH_BOUNDARY = 2;
>+const MATCH_BEGINNING = 3;

>+// Behavior constants.  These indicate what we should be searching.
>+const kBehaviorHistory = 1 << 0;
>+const kBehaviorBookmark = 1 << 1;
>+const kBehaviorTag = 1 << 2;
>+const kBehaviorTitle = 1 << 3;
>+const kBehaviorUrl = 1 << 4;
>+const kBehaviorTyped = 1 << 5;

would be cool to share these values


>+// AutoComplete query type constants.  Describes that various types of queries
>+// that we can process.
>+const kQueryTypeKeyword = 0;
>+const kQueryTypeFiltered = 1;
>+
>+// This separator is used as an RTL-friendly way to split the title and tags.
>+// It can also be used by an nsIAutoCompleteResult consumer to re-split the
>+// "comment" back into the title and the tag.
>+const TITLE_TAGS_SEPARATOR = " \u2013 ";

why do you use 2 different naming conventions for constants? you should use uppercase or kPrefix everywhere, imo.

>+// The list of columns in the moz_places table.  This should match the
>+// definition in nsPlacesTables.h.
>+const MOZ_PLACES_COLUMNS =
>+  "id, url, title, rev_host, visit_count, hidden, typed, favicon_id, " +
>+  "frecency, last_visit_date"

ugh, it's a pity we cannot share the value, please add a note in nsPlacesTables.h specifying anyone updating that should also update this file. Or alternatively, simply select all the columns you need below, so we can get rid of this dependancy.

>+////////////////////////////////////////////////////////////////////////////////
>+//// Global Functions
>+
>+/**
>+ * Generates the SQL subquery to get the best favicon for a given revhost.  This
>+ * is the favicon for the most recent visit.
>+ *
>+ * @param aTableName
>+ *        The table to join with.  This must have a column called favicon_id.

join what? maybe "join favicons table with"

>+function nsPlacesAutoComplete()
>+{
>+  //////////////////////////////////////////////////////////////////////////////
>+  //// Shared Constants for Smart Getters
>+
>+  // Define common pieces of various queries
>+  // XXX bug 412736

this is TODO rather than XXX

>+  // in case of a frecency tie, break it with h.typed and h.visit_count
>+  // which is better than nothing.  This is slow, so not doing it yet...
>+  // Note: h.frecency is only selected because we need it for ordering.
>+  const SQL_BASE =
>+    "SELECT h.url, h.title, f.url, " + BOOK_TAG_SQL + ", h.visit_count, " +
>+           "h.typed, h.id, :query_type " +
>+    "FROM ( " +
>+      "SELECT " + MOZ_PLACES_COLUMNS + " FROM moz_places_temp " +
>+      "UNION ALL " +
>+      "SELECT " + MOZ_PLACES_COLUMNS + " FROM moz_places " +
>+      "ORDER BY frecency DESC " +

we are not using limit 1 here because the code is already removing duplicates? I cant' recall.

>+
>+  this.__defineGetter__("_starQuery", function() {

can we call this _bookmarkQuery? star is something defined in the frontend, while this is toolkit.

>+    delete this._starQuery;
>+    let replacementText = "AND bookmark IS NOT NULL";
>+    return this._starQuery = this._db.createStatement(
>+      SQL_BASE.replace("{ADDITIONAL_CONDITIONS}", replacementText, "g")
>+    );
>+  });

>+nsPlacesAutoComplete.prototype = {
>+  //////////////////////////////////////////////////////////////////////////////
>+  //// nsIAutoCompleteSearch
>+
>+  startSearch: function PAC_startSearch(aSearchString, aSearchParam,
>+                                        aPreviousResult, aListener)
>+  {
>+    // Note: We don't use aPreviousResult to make sure ordering of results are
>+    //       consistent.  See bug 412730 for more details

nit: missing ending .

>+
>+    // We want to store the original string with no leading or trailing
>+    // whitespace for case sensitive searches.

nit: whitespaces?

>+    this._originalSearchString = aSearchString.trim();
>+
>+    this._currentSearchString = this._fixupURIText(this._originalSearchString)
>+                                    .toLowerCase();
>+
>+    this._listener = aListener;
>+    let result = Cc["@mozilla.org/autocomplete/simple-result;1"].
>+                 createInstance(Ci.nsIAutoCompleteSimpleResult);
>+    result.setSearchString(aSearchString);
>+    result.setDefaultIndex(-1);
>+    result.setListener(this);
>+    result.setSearchResult(Ci.nsIAutoCompleteResult.RESULT_NOMATCH_ONGOING);
>+    this._result = result;
>+
>+    // If we are not enabled, we need to return now.
>+    if (!this._enabled) {
>+      this._finishSearch(false);
>+      return;
>+    }
>+
>+    // Notify the listener that results are pending (or if we are not enabled,
>+    // that we are done)...
>+    aListener.onSearchResult(this, result);
>+
>+    // Reset our search behavior to the default.
>+    this._behavior = this._defaultBehavior;
>+
>+    // If we have no search terms, this is a special search that should only
>+    // look for kBehaviorHistory and kBehaviorTyped.
>+    if (!this._currentSearchString) {
>+      this._setBehavior(kBehaviorHistory);
>+      this._setBehavior(kBehaviorTyped);
>+    }

to me looks like _setBehavior can also accept (kBehaviorHistory | kBehaviorTyped), it's doing a bit OR, so there should be no problem calling it once.

>+  //////////////////////////////////////////////////////////////////////////////
>+  //// mozIStorageStatementCallback
>+
>+  handleResult: function PAC_handleResult(aResultSet)
>+  {
>+    let row, haveMatches = false;
>+    while (row = aResultSet.getNextRow()) {

OT: just FYI, iirc storage docs (https://developer.mozilla.org/En/Storage) suggest to use:
# for (let row = aResultSet.getNextRow();  
#          row;  
#          row = aResultSet.getNextRow()) {  
the while is clearly far more readable, would be better in docs too, not sure if there's a reason for that strange for loop.

>+      let match = this._processRow(row);
>+      haveMatches = haveMatches || match;
>+
>+      if (this._result.matchCount >= this._maxRichResults) {

which case is covering >? is it just for sanity?

>+  handleError: function PAC_handleError(aError)
>+  {
>+    Components.utils.reportError(aError);
add some indication where the error comes from please like: "Places AutoComplete Service:" + aError



>+
>+  observe: function PAC_observe(aSubject, aTopic, aData)
>+  {
>+    if (aTopic == kQuitApplication) {
>+      this._os.removeObserver(this, kQuitApplication);
>+
>+      // Remove our preference observer.
>+      let prefs = Cc["@mozilla.org/preferences-service;1"].
>+                  getService(Ci.nsIPrefService).
>+                  getBranch("browser.urlbar.").
>+                  QueryInterface(Ci.nsIPrefBranch2);
>+      prefs.removeObserver("", this);
>+
>+      // Finalize the statements that we have used.
>+      let stmts = [
>+        "_defaultQuery",
>+        "_historyQuery",
>+        "_starQuery",
>+        "_tagsQuery",
>+        "_typedQuery",
>+        "_adaptiveQuery",
>+        "_keywordQuery",
>+      ];
>+      for (let i = 0; i < stmts.length; i++) {
>+        // We do not want to create any query we haven't already created, so
>+        // see if it is a getter first.
>+        if (this.__lookupGetter__(stmts[i]) instanceof Function)

is the instanceof needed or is for sanity? as far as i know we don't use it in the other checks like this, so if it is needed please fix other instances of it in Places code. but i think lookupGetter will return null if it's not a getter.

something i'm just thinking, do you need to reset() the statements before using them again? In cpp we had the scoper, here i don't see a clear reset() call after each use of the global statements.

>+  //////////////////////////////////////////////////////////////////////////////
>+  //// nsPlacesAutoComplete
>+
>+  /**
>+   * Used to unescape encoded URI strings, and drop information that we do not
>+   * care about for searching.
>+   *
>+   * @param aURIString
>+   *        The text to unescape and modify.
>+   * @returns the modified uri.
>+   */
>+  _fixupURIText: function PAC_fixupURIText(aURIString)
>+  {
>+    let uri = aURIString;
>+
>+    if (uri.indexOf("http://") == 0)
>+      uri = uri.slice(7);
>+    else if (uri.indexOf("https://") == 0)
>+      uri = uri.slice(8);
>+    else if (uri.indexOf("ftp://") == 0)
>+      uri = uri.slice(6);
>+
>+    return this._textURIService.unEscapeURIForUI("UTF-8", uri);
>+  },

it's a pity this code can't be shared with cpp

>+  /**
>+   * Given an array of tokens, this function determines which query should be
>+   * ran.  It also removes any special search tokens.
>+   *
>+   * @param aTokens
>+   *        An array of search tokens.
>+   * @return the correctly optimized query to search the database with and the
>+   *         new list of tokens to search with.  The query has all the needed
>+   *         parameters bound, so consumers can execute it without doing any
>+   *         additional work.
>+   */
>+  _getSearch: function PAC_getSearch(aTokens)
>+  {
>+    // Establish which query we should use
>+    for (let i = aTokens.length - 1; i >= 0; i--) {
>+      let token = aTokens[i];
>+      let removeToken = true;
>+
>+      if (token == this._restrictHistoryToken)
>+        this._setBehavior(kBehaviorHistory);
>+      else if (token == this._restrictBookmarkToken)
>+        this._setBehavior(kBehaviorBookmark);
>+      else if (token == this._restrictTagToken)
>+        this._setBehavior(kBehaviorTag);
>+      else if (token == this._matchTitleToken)
>+        this._setBehavior(kBehaviorTitle);
>+      else if (token == this._matchURLToken)
>+        this._setBehavior(kBehaviorUrl);
>+      else if (token == this._restrictTypedToken)
>+        this._setBehavior(kBehaviorTyped);
>+      else
>+        removeToken = false;
>+
>+      if (removeToken)
>+        aTokens.splice(i, 1);

please, use a switch, on default: you can continue and so avoid the removeToken bool (if you continue the splice won't be executed if token is not recognized). otherwise you can still continue on else and still avoid the removeToken bool.

>+  /**
>+   * Obtains the search query to used based on the previously set search

nit: to used based

>+  /**
>+   * Obtains the keyword query with the properly bound parameters.
>+   *
>+   * @param aTokens
>+   *        The array of search tokens to check against.
>+   * @returns the bound keyword query.

nit: @return is the correct javadoc tag

>+  /**
>+   * Processes a mozIStorageRow to generate the proper data for the AutoComplete
>+   * result.  This will add an entry to the current result if it matches the
>+   * criteria.
>+   *
>+   * @param aRow
>+   *        The row to process.
>+   * @returns true if the row is accepted, and false if not.
>+   */
>+  _processRow: function PAC_processRow(aRow)
>+  {
>+    // We only want to filter javascript: URIs if the search does not start with
>+    // "javascript:"
>+    let filterJavascript = this._filterJavascript &&
>+                           this._currentSearchString.indexOf("javascript:") != 0;

i guess if a substr would not be faster than searching javascript: in the full string, or at least comparing length before.


>+    // Before we do any more work, make sure this entry isn't already to our
>+    // results.

"to our result" should be "in out result"


>+  /**
>+   * Enables the desired AutoComplete behavior.
>+   *
>+   * @param aType
>+   *        One of the kBehavior* constants.
>+   */
>+  _setBehavior: function PAC_setBehavior(aType)

looks like it can also accept a bitfield of the above constants, see above.

>+
>+  classDescription: "AutoComplete result generator for places",

nit: ucfirst Places

>diff --git a/toolkit/components/places/src/nsPlacesModule.cpp b/toolkit/components/places/src/nsPlacesModule.cpp
>--- a/toolkit/components/places/src/nsPlacesModule.cpp
>+++ b/toolkit/components/places/src/nsPlacesModule.cpp
>@@ -37,12 +37,6 @@ static const nsModuleComponentInfo compo
>     nsNavHistoryConstructor,
>     NS_NAVHISTORY_CLASSINFO },
> 
>-  { "Browser Navigation History",
>-    NS_NAVHISTORYSERVICE_CID,
>-    "@mozilla.org/autocomplete/search;1?name=history",
>-    nsNavHistoryConstructor,
>-    NS_NAVHISTORY_CLASSINFO },
>-
>   { "Download Navigation History",
>     NS_NAVHISTORYSERVICE_CID,
>     NS_DOWNLOADHISTORY_CONTRACTID,

>diff --git a/toolkit/components/places/src/nsPlacesSQLFunctions.cpp b/toolkit/components/places/src/nsPlacesSQLFunctions.cpp
>new file mode 100644

>+  NS_IMETHODIMP
>+  MatchAutoCompleteFunction::OnFunctionCall(mozIStorageValueArray *aArguments,
>+                                            nsIVariant **_result)
>+  {

>+
>+    // Check JavaScript URIs - right now we do this in the JS end.  Doing it
>+    // here would require us to pass in the preference to the function.
>+    // TODO do we want to do this in C++?
>+
>+    // Clean up our URI spec and prepare it for searching.
>+    nsString fixedURI = fixupURISpec(url);

this is used largely below this point, could be useless work if we early return.

>diff --git a/toolkit/components/places/src/nsPlacesSQLFunctions.h b/toolkit/components/places/src/nsPlacesSQLFunctions.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/src/nsPlacesSQLFunctions.h
>@@ -0,0 +1,210 @@
>+/* vim: sw=2 ts=2 et lcs=trail\:.,tab\:>~ :
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Places code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Shawn Wilsher <[e-mail]> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#ifndef nsPlacesSQLFunctions_h_
>+#define nsPlacesSQLFunctions_h_
>+
>+/**
>+ * This file contains functions that places adds to the database handle that are
>+ * accessible in SQL queries.
>+ */

places should be uppercase, i'm not english mother-tongue but the phrase, like it is, looks awful. Probably needs a period in the middle, like "contains functions added by Places to the database handle.  These will be accessible by SQL queries."

>+  /**
>+   * Handy Dandy Typedefs

uhm?

>+   */
>+  typedef bool (*searchFunctionPtr)(const nsDependentSubstring &aToken,
>+                                    const nsAString &aSourceString);
>+
>+  enum MatchBehavior {
>+    MATCH_ANYWHERE = 0
>+  , MATCH_BOUNDARY = 2
>+  , MATCH_BEGINNING = 3
>+  };

i don't like an incomplete enum like this, MATCH_BOUNDARY_ANYWHERE is missing, even if it's actually not used, in future someone could use it, or wrongly think 1 is a free value.

>+  /**
>+   * Determines if aChar is a word boundary.  A 'word boundary' is anything that
>+   * is not used to build up a word from a string of characters.  We are very
>+   * conservative here because anything that we do not list will be treated as a
>+   * word boundary.  This means searching for that not-actually-a-word-boundary
>+   * character can still be matched in the middle of a word.
>+   *
>+   * @param aChar
>+   *        The Unicode character to check against.
>+   * @return true if the character is considered a word boundary, false
>+   *          otherwise.
>+   */
>+  static inline bool isWordBoundary(const PRUnichar &aChar);
>+
>+  /**
>+   * Fixes a URI's spec so that it is ready to be searched.

nit: what about a comma after spec?

Comment 60

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 387056 [details] [diff] [review]
v1.1

interdiff looks good, r+ as a first-pass, i'd like to better compare the code with the old implementation, on next version of it. hope there will be time to do that before i leave.

Comment 61

[Shawn Wilsher :sdwilsh]

(In reply to comment #59)
> actually if the methods are only used in the file they are defined we could use
> an empty named namespace to limit them to this file, and looks like is the
> case. Do we want to be so explicit for the small utils that are defined there?
You cannot do anonymous namespaces between files though.  As we slowly move everything into mozilla::places, we can get rid of these declarations, but until then, it makes using these classes a bit easier.

> why is initing those 2 functions so different, could you make them use a common
> pattern/helper?
The intent is to move that into nsPlacesSQLFunctions.[h|cpp], but in a follow-up bug so the changes being made here are more obvious.

> can we put this into the namespace?
I'll do even better and place it in an anonymous namespace.

> urls are usually considered utf8 all around and saved as such, i guess if this
> would not need to be converted and binded as UTF8 String.
SQLite is going to convert this to UTF-8 anyway when it writes it out.  With that said, this code is just copy and pasted from the deleted nsNavHistoryAutoComplete.cpp

> >+ * Portions created by the Initial Developer are Copyright (C) 2008
> 2009
This file was actually created in 2008 by me - it's just taken that long to drive this bug to completion ;)

> >+function book_tag_sql_fragment(aName, aColumn, aForTag)
> Even if this is just an helper, what about a javadoc comment?
Mostly didn't use java-doc for this macro-replacement because I wasn't terribly sure what to say about it.  We had pretty much zero comments in the old file, so I had nothing to go off of as well.

> ugh, it's a pity we cannot share the value, please add a note in
> nsPlacesTables.h specifying anyone updating that should also update this file.
> Or alternatively, simply select all the columns you need below, so we can get
> rid of this dependancy.
Updated comment to indicate that it's only the columns that we use in our AutoComplete queries.

> we are not using limit 1 here because the code is already removing duplicates?
> I cant' recall.
I don't recall either - but I copied the queries from the existing files.

> >+    // We want to store the original string with no leading or trailing
> >+    // whitespace for case sensitive searches.
> nit: whitespaces?
Naw, whitespace is one of those strange words where the plural form is the same as the singular form AFIAK.

> to me looks like _setBehavior can also accept (kBehaviorHistory |
> kBehaviorTyped), it's doing a bit OR, so there should be no problem calling it
> once.
It can accept a bitfield, but I find that it's more readable this way.  Using the methods hides the fact that this is being stored as a bitfield, and I kinda like that.

> OT: just FYI, iirc storage docs (https://developer.mozilla.org/En/Storage)
> suggest to use:
> # for (let row = aResultSet.getNextRow();  
> #          row;  
> #          row = aResultSet.getNextRow()) {  
> the while is clearly far more readable, would be better in docs too, not sure
> if there's a reason for that strange for loop.
Yeah, I'm not sure I like the storage doc way (even if I wrote it).  Here it worked out well to just declare it outside with the other variable I needed to use.

> >+      if (this._result.matchCount >= this._maxRichResults) {
> which case is covering >? is it just for sanity?
I think it was sanity, but I shouldn't need to do that.

> something i'm just thinking, do you need to reset() the statements before using
> them again? In cpp we had the scoper, here i don't see a clear reset() call
> after each use of the global statements.
You do not need to reset asynchronous statements.  See https://developer.mozilla.org/En/MozIStorageStatement#executeAsync%28%29

Comment 62

[Shawn Wilsher :sdwilsh]

I'll have a new patch up in a bit.  I updated my tree this morning, and now I'm failing test_000_frecency.js, so I need to figure out why before I post a new patch.

Comment 63

[Ed Lee :Mardak]

(In reply to comment #59)
> >+      if (this._result.matchCount >= this._maxRichResults) {
> which case is covering >? is it just for sanity?
In the old code, I believe it was possible to have more than maxRichResults for matchCount. I think it would happen if the adaptive query finds more than 12 results then stops then the normal history query runs and appends one and sees that it's over 12 then quits.

Comment 64

[Shawn Wilsher :sdwilsh]

(In reply to comment #63)
> In the old code, I believe it was possible to have more than maxRichResults for
> matchCount. I think it would happen if the adaptive query finds more than 12
> results then stops then the normal history query runs and appends one and sees
> that it's over 12 then quits.
That is not a factor with this new code.

Comment 65

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Tests were failing do to a missing line filtering out duplicates in the base query.  This passes tests and should address review comments.  Once mak and Mardak give this r+, it'll be dietrich's turn.

Comment 66

[Ed Lee :Mardak]

Attachment: interdiff 1.1 -> 1.2

Comment 67

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

I forgot to qrefresh...

Comment 68

[Ed Lee :Mardak]

Comment on attachment 387284 [details] [diff] [review]
v1.3

r=Mardak. Nothing really needs to be changed, but the BEHAVIOR stuff could be cleaned up.

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -2146,62 +2085,6 @@ nsNavHistory::LoadPrefs(PRBool aInitiali
>-  // Clear out the search on any pref change to invalidate cached search
>-  mCurrentSearchString = EmptyString();
FYI, this was only needed because we used previous queries.

>+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
>+// This is just a helper for the next constant.
>+function book_tag_sql_fragment(aName, aColumn, aForTag)
>+{
>+  return ["(",
>+    "SELECT ", aColumn, " ",
>+    "FROM moz_bookmarks b ",
>+    "JOIN moz_bookmarks t ",
>+      "ON t.id = b.parent ",
>+      "AND t.parent ", (aForTag ? "" : "!"), "= :parent ",
>+    "WHERE b.type = ", Ci.nsINavBookmarksService.TYPE_BOOKMARK, " ",
>+      "AND b.fk = h.id ",
>+    (aForTag ? "AND LENGTH(t.title) > 0" :
>+               "ORDER BY b.lastModified DESC LIMIT 1"),
>+  ") AS ", aName].join("");
>+}
Not quite what I meant, but that's fine.
function book_tag_sql_fragment(aName, aColumn, aForTag) ["(",
  "SELECT", aColumn,
  "FROM moz_bookmarks b",
  "JOIN moz_bookmarks t",
  "ON t.id = b.parent AND t.parent", (aForTag ? "" : "!") + "= :parent",
  "WHERE b.type =", Ci.nsINavBookmarksService.TYPE_BOOKMARK,
    "AND b.fk = h.id", (aForTag ? "AND LENGTH(t.title) > 0" :
      "ORDER BY b.lastModified DESC LIMIT 1"),
  ") AS", aName
].join(" ");

>+const BOOK_TAG_SQL =
>+const kQuitApplication = "quit-application";
What was the decision on constant names?

>+// Behavior constants.  These indicate what we should be searching.
>+const BEHAVIOR_HISTORY = Ci.placesIAutoComplete.BEHAVIOR_HISTORY;
>+const BEHAVIOR_BOOKMARK = Ci.placesIAutoComplete.BEHAVIOR_BOOKMARK;
>+const BEHAVIOR_TAG = Ci.placesIAutoComplete.BEHAVIOR_TAG;
>+const BEHAVIOR_TITLE = Ci.placesIAutoComplete.BEHAVIOR_TITLE;
>+const BEHAVIOR_URL = Ci.placesIAutoComplete.BEHAVIOR_URL;
>+const BEHAVIOR_TYPED = Ci.placesIAutoComplete.BEHAVIOR_TYPED;
>+const BEHAVIOR_JAVASCRIPT = Ci.placesIAutoComplete.BEHAVIOR_JAVASCRIPT;
We don't actually need these to be defined.. (See below)

>+  _getSearch: function PAC_getSearch(aTokens)
>+    // Establish which query we should use
>+    for (let i = aTokens.length - 1; i >= 0; i--) {
>+      switch (aTokens[i]) {
>+        case this._restrictHistoryToken:
>+          this._setBehavior(BEHAVIOR_HISTORY);
The comment doesn't make sense in this context as the query picking is in a separate function now. Probably something more related to check for special tokens that change the behavior.

>+  /**
>+   * Determines if the specified AutoComplete behavior is set.
>+   *
>+   * @param aType
>+   *        One of the kBehavior* constants.
>+   * @return true if the behavior is set, false otherwise.
>+   */
>+  _getBehavior: function PAC_getBehavior(aType)
aType is actually a BEHAVIOR_ constant now. Same for setBehavior. But we could just accept the string type instead of the constant.. See below.

Actually.. see attached for the BEHAVIOR stuff.

Comment 69

[Ed Lee :Mardak]

Attachment: behavior changes on top of v1.3

Not a working patch.. need to define this._tokens = [] somewhere.

But basically, setBehavior(BEHAVIOR_HISTORY) -> setBehavior("history")

Comment 70

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 387284 [details] [diff] [review]
v1.3

>diff --git a/toolkit/components/places/src/Makefile.in b/toolkit/components/places/src/Makefile.in
>--- a/toolkit/components/places/src/Makefile.in
>+++ b/toolkit/components/places/src/Makefile.in

> 
> EXTRA_PP_COMPONENTS = nsLivemarkService.js \
>                       nsTaggingService.js \
>-                      nsPlacesDBFlush.js \
>                       $(NULL)

please while here
EXTRA_PP_COMPONENTS = \
  nsLivemarkService.js \
  ...

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

+#define PLACES_AUTOCOMPLETE_FEEDBACK_UPDATED_TOPIC "places-autocomplete-feedback-updated"

nit: this should be #ifdef MOZ_XUL

>+  rv = MatchAutoCompleteFunction::create(mDBConn);

please recall to file the bug to create a common syntax to define our SQL functions

>+mozIStorageStatement*
>+nsNavHistory::GetDBFeedbackIncrease()
>+{
>+  if (mDBFeedbackIncrease)
>+    return mDBFeedbackIncrease;
>+
>+  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+    // Leverage the PRIMARY KEY (place_id, input) to insert/update entries.
>+    "INSERT OR REPLACE INTO moz_inputhistory "
>+      // use_count will asymptotically approach the max of 10.

nit: ucfirst

>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js


>+  //////////////////////////////////////////////////////////////////////////////
>+  //// Shared Constants for Smart Getters
>+
>+  // Define common pieces of various queries
>+  // TODO bug 412736
>+  // in case of a frecency tie, break it with h.typed and h.visit_count
>+  // which is better than nothing.  This is slow, so not doing it yet...
>+  // Note: h.frecency is only selected because we need it for ordering.

is not clear which part of the comment the TODO is referred to

>+  const SQL_BASE =
>+    "SELECT h.url, h.title, f.url, " + BOOK_TAG_SQL + ", h.visit_count, " +
>+           "h.typed, h.id, :query_type " +
>+    "FROM ( " +
>+      "SELECT " + MOZ_PLACES_COLUMNS + " FROM moz_places_temp " +
>+      "UNION ALL " +
>+      "SELECT " + MOZ_PLACES_COLUMNS + " FROM moz_places " +

>+      "WHERE +id NOT IN (SELECT id FROM moz_places_temp) " +

iirc the old code was not doing this because the ac code was already dropping duplicates, so it did feel faster to avoid filtering

>+      "ORDER BY frecency DESC " +

is this subquery really taking everything out of moz_places, unioning with anything out of moz_places_temp, ordering all of it by frecency and then filtering/joining the result? sounds like damn slow on large dbs, sqlite will have to wait for this unioned table to be ready, in the old code this was mitigated by the LIMIT (at least for the first chunks), since here we execute async and don't need the limit, i guess if the query should be better rewritten as 2 unioned querie. It should be faster. Even if this is async we should not use unoptimized queries.


>+  startSearch: function PAC_startSearch(aSearchString, aSearchParam,
>+                                        aPreviousResult, aListener)
>+  {
>+    // Note: We don't use aPreviousResult to make sure ordering of results are
>+    //       consistent.  See bug 412730 for more details.
>+
>+    // We want to store the original string with no leading or trailing
>+    // whitespace for case sensitive searches.
>+    this._originalSearchString = aSearchString.trim();
>+
>+    this._currentSearchString =
>+      this._fixupSearchText(this._originalSearchString.toLowerCase());
>+
>+    this._listener = aListener;
>+    let result = Cc["@mozilla.org/autocomplete/simple-result;1"].
>+                 createInstance(Ci.nsIAutoCompleteSimpleResult);
>+    result.setSearchString(aSearchString);
>+    result.setDefaultIndex(-1);

you never set default index to 0, is that done automatically later? Previous code was doing that in case of RESULT_SUCCESS or _ONGOING

>+  _loadPrefs: function PAC_loadPrefs(aRegisterObserver)
>+  {
>+    let prefs = Cc["@mozilla.org/preferences-service;1"].
>+                getService(Ci.nsIPrefService).
>+                getBranch("browser.urlbar.");

iirc you use "browser.urlbar." in 2 places, i would just put it in a const, that will make also obvious all related prefs are in that branch at first sight.

>+    function safeGetter(aName, aDefault) {
>+      let type;
>+      switch (typeof(aDefault)) {
>+        case "boolean":
>+          type = "Bool";
>+          break;
>+        case "number":
>+          type = "Int";
>+          break;
>+        case "string":
>+          type = "Char";
>+          break;
>+        default:
>+          throw "Unknown type!";
>+      }

you could also do, in a more compact way:
let type = { "boolean":"Bool", "number":"Int", "string":"Char" }[typeof(aDefault)];
if (!type)
  throw "Unknown Type!";


>+  _processRow: function PAC_processRow(aRow)
>+  {
>+    // Before we do any work, make sure this entry isn't already to our results.

comment is still wrong, you skipped some of my comments above, please check them again :)

can't see any hard showstopper

Comment 71

[Shawn Wilsher :sdwilsh]

(In reply to comment #70)
> +#define PLACES_AUTOCOMPLETE_FEEDBACK_UPDATED_TOPIC
> "places-autocomplete-feedback-updated"
> nit: this should be #ifdef MOZ_XUL
Does it really matter?  I mean, it's just a define...

> >+      // use_count will asymptotically approach the max of 10.
> nit: ucfirst
use_count references a column, so making the case different from what is in the query would be confusing.

> iirc the old code was not doing this because the ac code was already dropping
> duplicates, so it did feel faster to avoid filtering
I was getting unit test failures without that and I got different results for things like typed and visit_count without this.  This only started happening after bug 483980 landed.  We also have that line in our current query (I checked it to see if that was why unit tests were failing).

> is this subquery really taking everything out of moz_places, unioning with
> anything out of moz_places_temp, ordering all of it by frecency and then
> filtering/joining the result? sounds like damn slow on large dbs, sqlite will
> have to wait for this unioned table to be ready, in the old code this was
> mitigated by the LIMIT (at least for the first chunks), since here we execute
> async and don't need the limit, i guess if the query should be better rewritten
> as 2 unioned querie. It should be faster. Even if this is async we should not
> use unoptimized queries.
I didn't change the query (other than dropping the internal limits used for chunking)!  I'd prefer to file a follow-up for this and not make this patch any more complicated than it already is.

> comment is still wrong, you skipped some of my comments above, please check
> them again :)
What did I miss?  Looking over all your comments I either addressed them, or replied in comment why I didn't.

(In reply to comment #68)
> FYI, this was only needed because we used previous queries.
I still need it for running the second query if we didn't get enough results.

> >+const BOOK_TAG_SQL =
> >+const kQuitApplication = "quit-application";
> What was the decision on constant names?
I forgot to change BOOK_TAG_SQL, but I was going to go with k* for the ones defined in this file (with the exception of the shortcuts for the idl constants which are all caps).

(In reply to comment #69)
> Created an attachment (id=387307) [details]
> behavior changes on top of v1.3
> But basically, setBehavior(BEHAVIOR_HISTORY) -> setBehavior("history")
While this results in less code, I find that it also obfuscates what is actually going on in places, so I'm not going to take it verbatim.  I will steal some ideas from it though.

Comment 72

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

This should address the review comments.

Comment 73

[Shawn Wilsher :sdwilsh]

Attachment: v1.3->v1.4 interdiff

Comment 74

[Ed Lee :Mardak]

I've tried the tryserver builds and we definitely should bring back the previous query at some point. Finding results then typing another letter and having to wait a second for the same result to show up again is kinda annoying.

But the speed that it goes through all my history (twice) is impressive.

We'll need a followup UI bug for the flashiness.. or is that bug 393902?

Comment 75

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #74)
> I've tried the tryserver builds and we definitely should bring back the
> previous query at some point. Finding results then typing another letter and
> having to wait a second for the same result to show up again is kinda annoying.

could also be that fixing the base query to be more efficient (actually it is not at all, due to my above comment, we are extracting all moz_places from the two tables and sorting them by frecency, and sqlite is waiting for that before starting returning results), will fix the waiting.

Comment 76

[Shawn Wilsher :sdwilsh]

(In reply to comment #74)
> I've tried the tryserver builds and we definitely should bring back the
> previous query at some point. Finding results then typing another letter and
> having to wait a second for the same result to show up again is kinda annoying.
> 
> But the speed that it goes through all my history (twice) is impressive.
I'll be working on bug 479528 today on the plane ride so we an get some actual numbers on this stuff.  Going to try to do this in talos, so we can actually have it running on tinderbox too.

> We'll need a followup UI bug for the flashiness.. or is that bug 393902?
It's bug 393902 AFAIK.

Getting bug 459269 resolved will be nice too - then we can land this on our project branch and parallelize that last bit of work.

(In reply to comment #75)
> could also be that fixing the base query to be more efficient (actually it is
> not at all, due to my above comment, we are extracting all moz_places from the
> two tables and sorting them by frecency, and sqlite is waiting for that before
> starting returning results), will fix the waiting.
But as long as we are "as fast" as the current location bar, I think it's OK to land it and fix that stuff as a follow-up.  Hopefully we'll have numbers by the end of the week.

Comment 77

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #76)
> But as long as we are "as fast" as the current location bar, I think it's OK to
> land it and fix that stuff as a follow-up.  Hopefully we'll have numbers by the
> end of the week.

Of course, there's no need to stop this, follow-up is fine, just pointing current performances are not the best we can get out of that query. Please start filing follow-ups to track things.

Comment 78

[Ed Lee :Mardak]

(In reply to comment #75)
> (In reply to comment #74)
> > having to wait a second for the same result to show up again is annoying.
> ..., will fix the waiting.
That will probably help in the early cases, but I'm describing the tail end for "history search" -- not frequently visited sites.

Like if the autocomplete ends up switching from word boundary and does a second pass for anywhere match, that will most likely incur some waiting. So if you type another letter, it'll wait then reshow the results.

Easy way to run in to this is typing a new url and it searches through the whole db twice and reports that it found nothing.

Comment 79

[Dietrich Ayala (:dietrich)]

Comment on attachment 387367 [details] [diff] [review]
v1.4

r=me, comments here: http://reviews.visophyte.org/r/387367/

Comment 80

[Shawn Wilsher :sdwilsh]

dietrich's review comments:
on file: toolkit/components/places/public/Makefile.in line 64
>   placesIAutoComplete.idl \

hrm. i prefer mozIPlacesAutoComplete, which seems consistent with s/nsI/mozI/.
i don't see a benefit to moving to {foo}I. might be worth a dev.platform
thread...


on file: toolkit/components/places/public/placesIAutoComplete.idl line 43
>  * This interface provides some constants used by the places AutoComplete

s/places/Places/


on file: toolkit/components/places/public/placesIAutoComplete.idl line 58
>    * Match first on word boundaries, and if we do not get enough results, we

s/we will/then/


on file: toolkit/components/places/src/Makefile.in line 90
>           nsPlacesSQLFunctions.cpp \

continuing your trend, we could get crazy and just name it
placesSQLFunctions.cpp.


on file: toolkit/components/places/src/nsPlacesAutoComplete.js line 98
> // AutoComplete query type constants.  Describes that various types of queries

s/that/the/


on file: toolkit/components/places/src/nsPlacesAutoComplete.js line 344
>     // If we are not enabled, we need to return now.

looks like this could be moved up?


on file: toolkit/components/places/src/nsPlacesAutoComplete.js line 350
>     // Notify the listener that results are pending (or if we are not enabled,

if not enabled, you already early-returned before this point. hm, or should
the enabled check and early return be after notifying the listener? (re my
previous comment about moving that check earlier)


on file: toolkit/components/places/src/nsPlacesAutoComplete.js line 371
>     let {query, tokens} =

this doesn't look like it should work... {} destructures into the current
scope?


on file: toolkit/components/places/src/nsPlacesAutoComplete.js line 419
>     let (result = this._result) {

is this block necessary? also, could be inside the haveMatches check.


on file: toolkit/components/places/src/nsPlacesAutoComplete.js line 435
>     // If we have destroyed our array used to track pending queries, we should

i'd prefer something explicit, like an _isSearchComplete() which checks
_pendingQueries.


on file: toolkit/components/places/src/nsPlacesSQLFunctions.h line 68
>  *        The URL to test if we have an AutoComplete match for.

"The URL to test for an AutoComplete match"

and the other instances below.


on file: toolkit/components/places/src/nsPlacesSQLFunctions.h line 76
>  *        Indicates if aURL should be considered a typed URL or not.

s/should be considered/is/

also note that it's a boolean.

for both instances.


on file: toolkit/components/places/src/nsPlacesSQLFunctions.h line 181
>    * Fixes a URI's spec such that it is ready to be searched.

please explain in the comment what kinds of things need fixing up.


on file: toolkit/components/places/src/nsPlacesSQLFunctions.cpp line 65
>     return aDBConn->CreateFunction(

check rv here before returning?


on file: toolkit/components/places/src/nsPlacesSQLFunctions.cpp line 200
>     PRUint32 argCount;

is this possible? if not, please remove.

Comment 81

[Shawn Wilsher :sdwilsh]

(In reply to comment #80)
> on file: toolkit/components/places/src/nsPlacesAutoComplete.js line 344
> >     // If we are not enabled, we need to return now.
> looks like this could be moved up?
No, because we need to tell the listener that we are "done", so it cannot be done earlier.

> on file: toolkit/components/places/src/nsPlacesAutoComplete.js line 371
> >     let {query, tokens} =
> this doesn't look like it should work... {} destructures into the current
> scope?
It does work, and it lets you return an object with named properties.  We could then also do something like |let {tokens} = ...| to just get the tokens and not care about the  queries.  Mardak suggested this, and I think it makes the code easier to follow in general.

> on file: toolkit/components/places/src/nsPlacesSQLFunctions.cpp line 200
> >     PRUint32 argCount;
> is this possible? if not, please remove.
It shouldn't be possible, but that's why it's a debug-only assertion.

Comment 82

[Shawn Wilsher :sdwilsh]

Attachment: v1.5

Addresses review comments; ready to land.

Comment 83

[Ed Lee :Mardak]

I used the test script from the "make location bar even faster" bug:
https://bug478097.bugzilla.mozilla.org/attachment.cgi?id=363522

Each line of the results shows the query and min/avg/max times to get the first result and min/avg/max times to decide to stop searching.

trunk 3.6a1pre
Query: m First: 9,15,17 Finish: 9,15,17
Query: mo First: 6,6,7 Finish: 6,6,7
Query: moz First: 7,7,7 Finish: 109,113,116
Query: mozilla First: 6,7,7 Finish: 6,7,7
Query: mozilla bug First: 116,121,129 Finish: 116,121,129
Query: mozilla bugzilla First: 7,8,10 Finish: 7,8,10
Query: mozilla bugzilla f First: 7,8,9 Finish: 121,124,128
Query: mozilla bugzilla fa First: 7,8,9 Finish: 798,842,950
Query: mozilla bugzilla fas First: 7,9,12 Finish: 16701,17706,19396

async 3.6a1pre
Query: m First: 10,12,16 Finish: 12,203,254
Query: mo First: 8,8,9 Finish: 245,246,247
Query: moz First: 8,8,9 Finish: 244,246,247
Query: mozilla First: 8,54,239 Finish: 244,247,249
Query: mozilla bug First: 238,239,241 Finish: 250,251,253
Query: mozilla bugzilla First: 237,240,242 Finish: 248,251,254
Query: mozilla bugzilla f First: 237,239,242 Finish: 252,258,263
Query: mozilla bugzilla fa First: 242,245,247 Finish: 324,334,347
Query: mozilla bugzilla fas First: 243,246,252 Finish: 2377,2397,2417

Trunk location bar is optimized for incremental typing, so that's why it gets results almost immediately when searching with more words. So I ran another test where I just did [].reverse()..

trunk 3.6a1pre reverse
Query: mozilla bugzilla fas First: 37,126,160 Finish: 16540,17255,17446
Query: mozilla bugzilla fa First: 38,38,38 Finish: 252,296,312
Query: mozilla bugzilla f First: 22,23,23 Finish: 22,23,23
Query: mozilla bugzilla First: 18,19,20 Finish: 18,19,20
Query: mozilla bug First: 19,19,21 Finish: 19,19,21
Query: mozilla First: 14,17,25 Finish: 14,17,25
Query: moz First: 14,18,33 Finish: 14,18,33
Query: mo First: 13,18,34 Finish: 13,18,34
Query: m First: 15,19,34 Finish: 15,19,34

async 3.6a1pre reverse
Query: mozilla bugzilla fas First: 242,244,246 Finish: 2360,2390,2407
Query: mozilla bugzilla fa First: 243,244,244 Finish: 324,326,327
Query: mozilla bugzilla f First: 237,239,241 Finish: 252,257,259
Query: mozilla bugzilla First: 236,238,241 Finish: 249,251,254
Query: mozilla bug First: 237,238,241 Finish: 250,251,254
Query: mozilla First: 8,8,9 Finish: 244,246,249
Query: moz First: 8,10,18 Finish: 244,247,249
Query: mo First: 8,9,10 Finish: 244,246,247
Query: m First: 10,11,11 Finish: 246,249,252

Comment 84

[Nikolay Shopik]

(In reply to comment #83)
> I used the test script from the "make location bar even faster" bug:
> https://bug478097.bugzilla.mozilla.org/attachment.cgi?id=363522
> 
> Each line of the results shows the query and min/avg/max times to get the first
> result and min/avg/max times to decide to stop searching.
> 
This is in ms right?

Comment 85

[Ed Lee :Mardak]

(In reply to comment #84)
> This is in ms right?
Yup milliseconds.

Mak suggested getting rid of the subquery in SQL_BASE:

async nosubquery 3.6a1pre
Query: m First: 10,11,12 Finish: 13,13,14
Query: mo First: 8,9,9 Finish: 10,11,11
Query: moz First: 8,8,8 Finish: 10,11,13
Query: mozilla First: 8,8,9 Finish: 10,11,12
Query: mozilla bug First: 8,9,11 Finish: 16,17,18
Query: mozilla bugzilla First: 8,9,10 Finish: 15,16,16
Query: mozilla bugzilla f First: 9,10,10 Finish: 19,21,23
Query: mozilla bugzilla fa First: 15,16,16 Finish: 100,120,179
Query: mozilla bugzilla fas First: 15,15,16 Finish: 2074,2106,2179

async nosubquery 3.6a1pre reverse
Query: mozilla bugzilla fas First: 15,15,15 Finish: 2075,2080,2083
Query: mozilla bugzilla fa First: 15,15,16 Finish: 98,107,120
Query: mozilla bugzilla f First: 8,9,10 Finish: 20,21,23
Query: mozilla bugzilla First: 8,9,10 Finish: 16,16,16
Query: mozilla bug First: 9,9,10 Finish: 16,16,17
Query: mozilla First: 8,8,9 Finish: 11,12,14
Query: moz First: 8,8,9 Finish: 10,10,11
Query: mo First: 8,8,9 Finish: 10,10,10
Query: m First: 10,11,11 Finish: 12,14,17

OMGWTFBBQ! It's like insanely fast.

The full query looks like this in js.. but we can refactor it because the only thing that changes is moz_places_temp vs moz_places:

  const SQL_BASE =
    "SELECT h.url, h.title, f.url, " + BOOK_TAG_SQL + ", h.visit_count, " +
           "h.typed, h.id, :query_type, h.frecency " +
    "FROM moz_places_temp h " +
    "LEFT OUTER JOIN moz_favicons f " +
    "ON f.id = h.favicon_id " +
    "WHERE h.frecency <> 0 " +
    "AND AUTOCOMPLETE_MATCH(:searchString, h.url, IFNULL(bookmark, h.title), " +
                           "tags, h.visit_count, h.typed, parent, " +
                           ":matchBehavior, :searchBehavior) " +
    "{ADDITIONAL_CONDITIONS} " +
    "UNION ALL " +
    "SELECT h.url, h.title, f.url, " + BOOK_TAG_SQL + ", h.visit_count, " +
           "h.typed, h.id, :query_type, h.frecency " +
    "FROM moz_places h " +
    "LEFT OUTER JOIN moz_favicons f " +
    "ON f.id = h.favicon_id " +
    "WHERE h.frecency <> 0 " +
    "AND AUTOCOMPLETE_MATCH(:searchString, h.url, IFNULL(bookmark, h.title), " +
                           "tags, h.visit_count, h.typed, parent, " +
                           ":matchBehavior, :searchBehavior) " +
    "{ADDITIONAL_CONDITIONS} " +
    "ORDER BY h.frecency DESC";

Comment 86

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

queries changes like this will be handled in followup bug 503360

Comment 87

[Shawn Wilsher :sdwilsh]

In other words, with the query changes, we are as fast or faster than the current implementation for first results, and much faster for all results.  I think we might be OK to land this.

Comment 88

[Shawn Wilsher :sdwilsh]

Attachment: v1.6

Fixes an issue we discovered while testing this on irc.  Basically, we were causing stopSearch to be called twice, which was breaking some of our invariants.

Comment 89

[Shawn Wilsher :sdwilsh]

Attachment: v1.5->v1.6 interdiff

Comment 90

[Ed Lee :Mardak]

Comment on attachment 387772 [details] [diff] [review]
v1.5->v1.6 interdiff

>+  _finishSearch: function PAC_finishSearch(aControllerStopped)
>+    if (aControllerStopped)
>       this._pendingQueries.forEach(function(query) query.cancel());
>+    if (!aControllerStopped) {
Might as well make it an else.

Comment 91

[Ed Lee :Mardak]

Comment on attachment 387771 [details] [diff] [review]
v1.6

>+  handleCompletion: function PAC_handleCompletion(aReason)
>+    if (this._isSearchComplete())
>+      return;
>+    this._pendingQueries.shift();
>+    if (this._pendingQueries.length)
>+      return;
>+
>+    // If we do not have enough results, and our match type is
>+    // MATCH_BOUNDARY_ANYWHERE, search again with MATCH_ANYWHERE to get more
>+    // results.
>+    if (this._matchBehavior == MATCH_BOUNDARY_ANYWHERE &&
>+        this._result.matchCount < this._maxRichResults && !this._secondPass) {
Is it possible that we start a MATCH_ANYWHERE search when we've been told to stopSearch? We've just canceled, and we're on the last pending query to be canceled, but we've only got say 3 results..

Comment 92

[Ed Lee :Mardak]

Shawn hates me! >:)

So... I seemed to have gotten rid of my "Error: invalid 'in' operand this._usedPlaceIds" by adding some magic >:)

_startSearch: function() {
  while (this._pendingQueries.length > 0)
    thread.processNextEvent(true);

Instead of deleting this._pendingQueries at finishSearch, I keep it around so that handleCompletion is able to shift things off. If at pendingQueries still has items at startSearch, I spin the event loop to let the lagging handleCompletion finish.

Otherwise, the last handleCompletion gets in /after/ a new pendingQueries is set up for a new search, and we end up shifting out the "wrong" query.


So other than spinning the event loop, another way to fix this is to bind per-query callbacks. So instead of...
  this._pendingQueries.push(query.executeAsync(this));

We make an object to process handleCompletion, so we know which query is actually finishing. Otherwise, Shawn, mozstorage guy (nudge nudge), it might be useful to pass back the mozIStoragePendingStatement to handleCompletion or at least some identifier that matches up with what you get when doing query.executeAsync.

With that, we can know for sure..
handleCompletion: function(reason, pending) {
  if (pendingQueries.remove(pending)) // returns true if it found and removed

Comment 93

[Ed Lee :Mardak]

Attachment: slow handleCompletion patch v1 on top of v1.6

Here's a quick patch that fixes the problems for me when I type too fast.

Basically, create a new callback object for each executeAsync and add them to the pendingQueries. If their handle* is triggered and they aren't still in pendingQueries, it doesn't do anything; otherwise, it passes the message along to PAC_handle*.

(Hah! A new meaning for "stale handle" ;) ;))

[I never did like that this._isSearchComplete()... And that implicit assumption that pendingQueries will come back in the same order seems iffy, but I suppose now with an explicit listener, it knows which one to remove..]

Not a full fix... I didn't touch the MATCH_ANYWHERE yet, but that will require some extra logic to make sure we don't start one when we've been told to stopSearch. Also finishSearch(false) at the end of handleCompletion probably shouldn't trigger if finishSearch(true) was called from stopSearch.

I was playing around with a this._aborting = true from stopSearch. But perhaps we should just move cleanup code elsewhere..

Comment 94

[Dietrich Ayala (:dietrich)]

Comment on attachment 387772 [details] [diff] [review]
v1.5->v1.6 interdiff


>@@ -560,22 +559,24 @@ nsPlacesAutoComplete.prototype = {
>   /**
>    * Properly cleans up when searching is completed.
>    *
>-   * @param aCancel
>-   *        Indicates if we should cancel the pending queries.
>+   * @param aControllerStopped
>+   *        Indicates if we were told to stop searching by the controller.

make note of why this matters.

>    */
>-  _finishSearch: function PAC_finishSearch(aCancel)
>+  _finishSearch: function PAC_finishSearch(aControllerStopped)
>   {
>     // If we haven't finished, we need to cancel the query so we do not get any
>     // [more] results.
>-    if (aCancel)
>+    if (aControllerStopped)
>       this._pendingQueries.forEach(function(query) query.cancel());
> 
>-    // Notify about results.
>-    let result = this._result.matchCount ?
>-      Ci.nsIAutoCompleteResult.RESULT_SUCCESS :
>-      Ci.nsIAutoCompleteResult.RESULT_NOMATCH;
>-    this._result.setSearchResult(result);
>-    this._listener.onSearchResult(this, this._result);
>+    // Notify about results only if the controller did not tell us to stop.
>+    if (!aControllerStopped) {
>+      let result = this._result.matchCount ?
>+        Ci.nsIAutoCompleteResult.RESULT_SUCCESS :
>+        Ci.nsIAutoCompleteResult.RESULT_NOMATCH;
>+      this._result.setSearchResult(result);
>+      this._listener.onSearchResult(this, this._result);
>+    }

what mardak said

r=me

Comment 95

[Shawn Wilsher :sdwilsh]

Attachment: v1.6->v1.7 interdiff

Takes some ideas from attachment 387832 [details] [diff] [review] to fix the reported issues.  Also addresses review comments.

Comment 96

[Shawn Wilsher :sdwilsh]

Attachment: v1.7

Comment 97

[Dietrich Ayala (:dietrich)]

Comment on attachment 387898 [details] [diff] [review]
v1.6->v1.7 interdiff


>@@ -540,61 +549,104 @@ nsPlacesAutoComplete.prototype = {
>    *
>    * @param aTokens
>    *        The search tokens to use for this search.
>    * @param aQueries
>    *        The SQL queries to run for this search.
>    */
>   _startSearch: function PAC__startSearch(aTokens, aQueries)
>   {
>-    // Start executing the query
>-    this._pendingQueries = [];
>-    aQueries.forEach(function(query) {
>-      this._pendingQueries.push(query.executeAsync(this));
>-    }, this);
>+    // Start executing our queries.
>+    this._executeQueries(aQueries);
> 
>     // Set up our persistent state for the duration of the search.
>     this._searchTokens = aTokens;
>     this._usedPlaceIds = {};
>   },

_startSearch has only a single call site. any reason to not fold it into startSearch?
 
> 
>   /**
>    * Properly cleans up when searching is completed.
>    *
>-   * @param aControllerStopped
>-   *        Indicates if we were told to stop searching by the controller.
>+   * @param aNotify
>+   *        Indicates if we should notify about our results or not.
>    */

for clarity: "should notify the autocomplete listener about"

>+  _executeQueries: function PAC_executeQueries(aQueries)
>+  {
>+    // Because we might get a handleCompletion for canceled queries, we want to
>+    // filter out queries we no longer care about (described in later in the
>+    // handleCompletion implementation on callbackWrapper).

s/described in/described/

r=me otherwise

Comment 98

[Ed Lee :Mardak]

Comment on attachment 387898 [details] [diff] [review]
v1.6->v1.7 interdiff

>   stopSearch: function PAC_stopSearch()
>   {
>+    // We need to cancel the query so we do not get any [more] results.
>+    this._pendingQueries.forEach(function(query) query.cancel());
>+
>+    this._finishSearch(false);
>   },
Could we make this in to..
{
  this._stopQueries();
  this._finishSearch(false);
}

And reuse _stopQueries when we get enough results in handleResults.

>   /**
>+   * Used to track our executing queries so we can cancel them if we do not need
>+   * them to execute any more.
>+   */
>+  _pendingQueries: [],
We can remove this like the other stuff we delete... see below

>+  _finishSearch: function PAC_finishSearch(aNotify)
>-    delete this._pendingQueries;
>+    this._pendingQueries = [];
And keep the delete..

>+  _executeQueries: function PAC_executeQueries(aQueries)
>+  {
>+    // Because we might get a handleCompletion for canceled queries, we want to
>+    // filter out queries we no longer care about (described in later in the
>+    // handleCompletion implementation on callbackWrapper).
>+    let PAC = this;
>+    aQueries.forEach(function(aQuery) {
this._pendingQueries = aQueries.map(function(aQuery) {
...
>+      PAC._pendingQueries.push(handleWrapper);
return handleWrapper;

Btw, why the two separate callbackWrapper and handleWrapper? Could just implement both in one and generateQI([pending, callback]).

Also, technically you can remove the callback QI for placesAutoComplete.

>   _isSearchComplete: function PAC_isSearchComplete()
>-    return !this._pendingQueries;
>+    return this._pendingQueries.length == 0;
Then you can revert this... I would still prefer this._pendingQueries == null. (I thought you were for explicit! ;))

Comment 99

[Shawn Wilsher :sdwilsh]

Attachment: v1.8

Addresses review comments

Comment 100

[Shawn Wilsher :sdwilsh]

Attachment: v1.7->v1.8 interdiff

Comment 101

[Shawn Wilsher :sdwilsh]

Attachment: v1.8->v1.9 interdiff

Mardak and I found one instance of _pendingStatements not be checking before being accessed which can cause an exception.

Comment 102

[Shawn Wilsher :sdwilsh]

Attachment: v1.9

Comment 103

[Shawn Wilsher :sdwilsh]

Current builds contain the patch to bug 503360, which I'll push with this when I land it.  Feedback wanted!

Comment 104

[Shawn Wilsher :sdwilsh]

Attachment: v1.10

Found one other "race condition".  If we end up getting the right amount of results (this._maxResults) in handleResult, we would let handleCompletion call _finishSearch for us.  However, we called _stopActiveQueries, and then notified that we were done.  This is all fine, but then handleCompletion gets called, and it calls _finishSearch, which will notify again.  stopSearch ends up getting called, so we try to cancel again, which will fail.  Yay.  Fix is simple at least.  I also simplified and unified the code path for notifying results so we don't run into this again.

Comment 105

[Shawn Wilsher :sdwilsh]

Attachment: v1.9->v1.10 interdiff

Comment 106

[Robert Sayre]

Comment on attachment 388098 [details] [diff] [review]
v1.10

drive-by nits. this patch rocks overall, obv.

>+
>+  /* static */
>+  bool
>+  MatchAutoCompleteFunction::findOnBoundary(const nsDependentSubstring &aToken,
>+                                            const nsAString &aSourceString)
>+  {
>+    // We cannot match anything if there is nothing to search.
>+    if (aSourceString.IsEmpty())
>+      return false;
>+
>+    // Define a const instance of this class so it is created once.
>+    const nsCaseInsensitiveStringComparator caseInsensitiveCompare;
>+
>+    nsAString::const_char_iterator tokenStart(aToken.BeginReading()),
>+                                   tokenEnd(aToken.EndReading()),
>+                                   sourceStart(aSourceString.BeginReading()),
>+                                   sourceEnd(aSourceString.EndReading());
>+
>+    // The start of aSourceString is considered a word boundary, so start there.
>+    do {
>+      // We are on a word boundary, so start by copying the iterators.
>+      nsAString::const_char_iterator testTokenItr(tokenStart),
>+                                     testSourceItr(sourceStart);
>+
>+      // Keep trying to match the token one by one until it doesn't match.
>+      while (!caseInsensitiveCompare(*testTokenItr, *testSourceItr)) {
>+        // We matched something, so move down one.
>+        testTokenItr++;
>+        testSourceItr++;
>+
>+        // Matched the full token, so we are done!
>+        if (testTokenItr == tokenEnd)
>+          return true;
>+
>+        // However, if we ran into the end of the source while matching the
>+        // token, we will not find it.
>+        if (testSourceItr == sourceEnd)
>+          return false;
>+      }
>+
>+      // Unconditionally move past the current position in the source, but if we
>+      // are not currently on a word boundary, eat up as many non-word boundary
>+      // characters as possible since they are not useful to check anymore.
>+      if (!isWordBoundary(ToLowerCase(*sourceStart++)))
>+        while (sourceStart != sourceEnd && !isWordBoundary(*sourceStart))
>+          sourceStart++;
>+    } while (sourceStart != sourceEnd);

This is kinda hairy. I would at least move the second nested while loop with the big comment into its own function.
call it advanceSource() or something to your liking.

The first nested loop smells bad too, but I can't think of a clearer way to write it.


>+
>+  NS_IMETHODIMP
>+  MatchAutoCompleteFunction::OnFunctionCall(mozIStorageValueArray *aArguments,
>+                                            nsIVariant **_result)
>+  {
>+    // Obtain our data.
>+    nsDependentString searchString;
>+    (void)aArguments->GetString(kArgSearchString, searchString);
>+    nsDependentCString url;
>+    (void)aArguments->GetUTF8String(kArgIndexURL, url);
>+    nsDependentString title;
>+    (void)aArguments->GetString(kArgIndexTitle, title);
>+    nsDependentString tags;
>+    (void)aArguments->GetString(kArgIndexTags, tags);
>+    PRInt32 visitCount = aArguments->AsInt32(kArgIndexVisitCount);
>+    bool typed = aArguments->AsInt32(kArgIndexTyped) ? true : false;
>+    bool bookmark = aArguments->AsInt32(kArgIndexBookmark) ? true : false;
>+    PRInt32 matchBehavior = aArguments->AsInt32(kArgIndexMatchBehavior);
>+    PRInt32 searchBehavior = aArguments->AsInt32(kArgIndexSearchBehavior);
>+    #define GET_BEHAVIOR(aBitName) \
>+      (searchBehavior & mozIPlacesAutoComplete::BEHAVIOR_##aBitName)

Wow, this is dense. :)

Make GET_BEHAVIOR a static inline function. Break up the declarations with spacing, and by not doing them until you need them.
For example, it looks like you only need 'url' and 'searchString' to do the block below:


>+    // We only want to filter javascript: URLs if we are not supposed to search
>+    // for them, and the search does not start with "javascript:".
>+    if (!GET_BEHAVIOR(JAVASCRIPT) &&
>+        !StringBeginsWith(searchString, NS_LITERAL_STRING("javascript:")) &&
>+        StringBeginsWith(url, NS_LITERAL_CSTRING("javascript:"))) {
>+      NS_IF_ADDREF(*_result = new IntegerVariant(0));
>+      NS_ENSURE_TRUE(*_result, NS_ERROR_OUT_OF_MEMORY);
>+      return NS_OK;
>+    }
>+

...

>+  this.__defineGetter__("_db", function() {
>+    delete this._db;
>+    return this._db = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                      getService(Ci.nsPIPlacesDatabase).
>+                      DBConnection;
>+  });
>+
>+  this.__defineGetter__("_bh", function() {
>+    delete this._bh;
>+    return this._bh = Cc["@mozilla.org/browser/global-history;2"].
>+                      getService(Ci.nsIBrowserHistory);
>+  });
>+
>+  this.__defineGetter__("_textURIService", function() {
>+    delete this._textURIService;
>+    return this._textURIService = Cc["@mozilla.org/intl/texttosuburi;1"].
>+                                  getService(Ci.nsITextToSubURI);
>+  });
>+
>+  this.__defineGetter__("_bs", function() {
>+    delete this._bs;
>+    return this._bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+                      getService(Ci.nsINavBookmarksService);
>+  });
>+

We discussed this repetetive boilerplate on irc. Let's at least get a bug filed on it. If we move this stuff to a common location, maybe we'll someday have a good way to bind it at compile time.


>+
>+  //////////////////////////////////////////////////////////////////////////////
>+  //// nsIAutoCompleteSimpleResultListener
>+
>+  onValueRemoved: function PAC_onValueRemoved(aResult, aURISpec, aRemoveFromDB)
>+  {
>+    if (!aRemoveFromDB)
>+      return;
>+
>+    let uri = this._ioService.newURI(aURISpec, null, null);
>+    this._bh.removePage(uri);
>+  },

onValueRemoved: function PAC_onValueRemoved(aResult, aURISpec, aRemoveFromDB)
{
  if (aRemoveFromDB)
    this._bh.removePage(this._ioService.newURI(aURISpec, null, null));
},


>+
>+  /**
>+   * Executes the given queries asynchronously.
>+   *
>+   * @param aQueries
>+   *        The queries to execute.
>+   */
>+  _executeQueries: function PAC_executeQueries(aQueries)
>+  {
>+    // Because we might get a handleCompletion for canceled queries, we want to
>+    // filter out queries we no longer care about (described later in the
>+    // handleCompletion implementation on callbackWrapper).
>+    let PAC = this;
>+    this._pendingQueries = aQueries.map(function(aQuery) {
>+      // Handle will be our mozIStoragePendingStatement called later.  We need
>+      // to reference it before, however, so we declare it here.
>+      let handle;
>+
>+      // This wraps the mozIStoragePendingStatement we get back when calling
>+      // executeAsync.
>+      let handleWrapper = {
>+        cancel: function() handle.cancel(),
>+        QueryInterface: XPCOMUtils.generateQI([Ci.mozIStoragePendingStatement])
>+      };
>+
>+      // This wraps PlacesAutoComplete's implementation of
>+      // mozIStorageStatementCallback to filter handleCompletion calls.
>+      let callbackWrapper = {
>+        handleResult: function(aResultSet)
>+        {
>+          PAC.handleResult.apply(PAC, arguments);
>+        },
>+        handleError: function(aError)
>+        {
>+          PAC.handleError.apply(PAC, arguments);
>+        },
>+        handleCompletion: function(aReason)
>+        {
>+          // Only dispatch handleCompletion if we are not done searching and
>+          // _pendingQueries is still tracking this callback.
>+          if (!PAC._isSearchComplete() &&
>+              PAC._pendingQueries.indexOf(handleWrapper) != -1)
>+            PAC.handleCompletion.apply(PAC, arguments);
>+        },
>+        QueryInterface: XPCOMUtils.generateQI([Ci.mozIStorageStatementCallback])
>+      };


you shouldn't generate these QI impls more than once.


>+  /**
>+   * Processes a mozIStorageRow to generate the proper data for the AutoComplete
>+   * result.  This will add an entry to the current result if it matches the
>+   * criteria.
>+   *
>+   * @param aRow
>+   *        The row to process.
>+   * @return true if the row is accepted, and false if not.
>+   */
>+  _processRow: function PAC_processRow(aRow)
>+  {

This is kinda long, maybe break it up if you agree.

>+    }
>+    else
>+      var favicon = this._faviconService.defaultFavicon.spec;

khaaaaaaaaaaaaaaaaan!!!


>+  /**
>+   * Enables the desired AutoComplete behavior.
>+   *
>+   * @param aType
>+   *        The behavior type to set.
>+   */
>+  _setBehavior: function PAC_setBehavior(aType)
>+  {
>+    let behavior = Ci.mozIPlacesAutoComplete["BEHAVIOR_" + aType.toUpperCase()];
>+    this._behavior |= behavior;
>+  },
>+

_setBehavior: function PAC_setBehavior(aType)
{
   this._behavior |= Ci.mozIPlacesAutoComplete["BEHAVIOR_" + aType.toUpperCase()];
},

Comment 107

[Ed Lee :Mardak]

(In reply to comment #106)
> >+  this.__defineGetter__("_db", function() {
> >+    delete this._db;
> >+    return this._db = Cc["@mozilla.org/browser/nav-history-service;1"].
> >+                      getService(Ci.nsPIPlacesDatabase).
> >+                      DBConnection;
> We discussed this repetetive boilerplate on irc. Let's at least get a bug filed
In Weave (and a bunch of other code), we have lazy utils for this pattern and a lazySvc for services. We put these lazy services on a shared object, so we don't need to redo all this extra stuff per js module.

> onValueRemoved: function PAC_onValueRemoved(aResult, aURISpec, aRemoveFromDB)
>   if (aRemoveFromDB)
>     this._bh.removePage(this._ioService.newURI(aURISpec, null, null));
newURI is only used twice in this file.. but might be useful to have a this._URI(uri spec) ?

> >+        QueryInterface: XPCOMUtils.generateQI([Ci.mozIStorageStatementCallback])
> you shouldn't generate these QI impls more than once.
What?! We don't have dynamic pure function detection and auto-memoization? ;)

> >+    else
> >+      var favicon = this._faviconService.defaultFavicon.spec;
> khaaaaaaaaaaaaaaaaan!!!
Yeah.. I commented on that before and suggested let fav = default; if (..) fav =

Alternatively, just declare favicon outside without using "var".

Comment 108

[Shawn Wilsher :sdwilsh]

(In reply to comment #106)
> Make GET_BEHAVIOR a static inline function.
Doing so makes the code pretty long when we have to call it.  I think it makes things harder to read actually (trust me, I don't like using macros like I'm doing here either, but it makes the code way more readable).

> you shouldn't generate these QI impls more than once.
Pulled these "classes" out and just create new ones in the method now.  The function is small again!

> This is kinda long, maybe break it up if you agree.
Haven't really thought of a way to do so - I've felt's it has been long all this time.

> _setBehavior: function PAC_setBehavior(aType)
> {
>    this._behavior |= Ci.mozIPlacesAutoComplete["BEHAVIOR_" +
> aType.toUpperCase()];
> },
Using the temporary means we don't have line wrapping issues.

Comment 109

[Shawn Wilsher :sdwilsh]

Attachment: v1.10->v1.11 interdiff

per comments

Comment 110

[Shawn Wilsher :sdwilsh]

Attachment: v1.11

Comment 111

[Ed Lee :Mardak]

Comment on attachment 388161 [details] [diff] [review]
v1.10->v1.11 interdiff

>+++ b/toolkit/components/places/src/SQLFunctions.h
>   /**
>+   * Obtains an iterator to the next word boundary.
>+   *
>+   * @param aStart
>+   *        An iterator pointing to the start of the string.
>+   * @param aEnd
>+   *        An iterator pointing to the end of the string.
>+   * @return an iterator pointing to the next word boundary.
>+   */
>+  static const_wchar_iterator advanceSource(const_wchar_iterator aStart,
>+                                            const_wchar_iterator aEnd);
Perhaps call it getWordBoundary? moveTo..? update?

>+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
>+/**
>+ * Wraps a callback and ensures that handleCompletion is not dispatched if the
>+ * query is no longer tracked.
>+ *
>+ * @param aCallback
>+ *        A reference to a nsPlacesAutoComplete.
>+ * @param aHandle
>+ *        A reference to an object implementing mozIStoragePendingStatement for
>+ *        the statement that will notify this wrapper.
>+ */
>+function AutoCompleteStatementCallbackWrapper(aCallback,
>+                                              aHandle)
Why call them callback and handle? Here the "callback" implements mozIStorageStatementCallback, so the description isn't "nsPlacesAutoComplete" specific. But "handle" doesn't make sense? "pending"?

>+////////////////////////////////////////////////////////////////////////////////
>+//// AutoCompletePendingStatementWrapper class
>+
>+/**
>+ * Wraps an implementation of mozIStoragePendingStatement so it can be stored in
>+ * an array and functions like indexOf will work on this object.
>+ *
>+ * @param aHandle
>+ *        The object this class wraps.
>+ */
>+function AutoCompletePendingStatementWrapper(aHandle)
>+{
>+  this._handle = aHandle;
>+}
>+
>+AutoCompletePendingStatementWrapper.prototype = {
>+  //////////////////////////////////////////////////////////////////////////////
>+  //// mozIStoragePendingStatement
>+
>+  cancel: function()
>+  {
>+    this._handle.cancel();
>+  },
>+
>+  //////////////////////////////////////////////////////////////////////////////
>+  //// nsISupports
>+
>+  QueryInterface: XPCOMUtils.generateQI([
>+    Ci.mozIStoragePendingStatement,
>+  ])
>+};
This class does nothing extra and is just overhead.. You can just pass the original pending statement to AutoCompleteStatementCallbackWrapper instead of wrapping the pending statement and it will work the same.

The array of pendingQueries will contain what executeAsync returns, and the callback wrapper can still do the filtering because it knows the original pending query.

>@@ -944,12 +1000,12 @@ nsPlacesAutoComplete.prototype = {
>     this._usedPlaceIds[aPlaceId] = true;
> 
>     // Obtain the favicon for this URI.
>+    let favicon;
>     if (aFaviconSpec) {
>       let uri = this._ioService.newURI(aFaviconSpec, null, null);
>       var favicon = this._faviconService.getFaviconLinkForIcon(uri).spec;
var ?
>     }
>-    else
>-      var favicon = this._faviconService.defaultFavicon.spec;
>+    favicon = favicon || this._faviconService.defaultFavicon.spec;
Perhaps too fancy..

favicon = aFaviconSpec && getLinkForIcon(this._uri(spec)).spec || default.spec

Comment 112

[Ed Lee :Mardak]

(In reply to comment #111)
> favicon = aFaviconSpec && getLinkForIcon(this._uri(spec)).spec || default.spec
favicon = (spec && getLinkForIcon(URI(spec)) || default).spec ;) ;) ;)

Comment 113

[Shawn Wilsher :sdwilsh]

(In reply to comment #111)
> Why call them callback and handle? Here the "callback" implements
> mozIStorageStatementCallback, so the description isn't "nsPlacesAutoComplete"
> specific. But "handle" doesn't make sense? "pending"?
The callback does have to be nsPlacesAutoComplete specific because we rely on methods on the callback that are not part of mozIStorageSatementCallback.  Handle is accurate because it's a handle on a pending statement.  Pending sounds like a poor variable name.

> This class does nothing extra and is just overhead.. You can just pass the
> original pending statement to AutoCompleteStatementCallbackWrapper instead of
> wrapping the pending statement and it will work the same.
I thought so too, but all the unit tests hung and did not run to completion.  It looked like indexOf was always returning -1.

> >+    let favicon;
> >     if (aFaviconSpec) {
> >       let uri = this._ioService.newURI(aFaviconSpec, null, null);
> >       var favicon = this._faviconService.getFaviconLinkForIcon(uri).spec;
> var ?
forgot to change that.

Comment 114

[Ed Lee :Mardak]

Attachment: no pending statement wrapper

(In reply to comment #113)
> > This class does nothing extra and is just overhead.. You can just pass the
> > original pending statement to AutoCompleteStatementCallbackWrapper instead of
> > wrapping the pending statement and it will work the same.
> I thought so too, but all the unit tests hung and did not run to completion. 
> It looked like indexOf was always returning -1.
How did you do it? I've attached a patch that should get rid of the extra statement wrapper, but it wasn't a trivial change. See below.. or just the patch :p

>+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js

>     this._pendingQueries = aQueries.map(function(aQuery) {
>       let handle;
>+      let handleWrapper = new AutoCompletePendingStatementWrapper(handle);
>+      let callbackWrapper =
>+        new AutoCompleteStatementCallbackWrapper(PAC, handleWrapper);
>       handle = aQuery.executeAsync(callbackWrapper);
>       return handleWrapper;
How does this work right now? |handle| should be undefined, so passing it to the statement wrapper won't do what you expect because when you finally assign the actual object, it wont update the wrapper's reference.

For the patch, I had to have the callbackwrapper trigger the execute async so that I get the right handle because the callbackwrapper needs a handle, but to get the handle, you need a callback.

Try applying the patch and see if it passes tests? (I haven't updated my firefox build.. and I'm on battery away from home :p)

Comment 115

[Shawn Wilsher :sdwilsh]

(In reply to comment #114)
> How does this work right now? |handle| should be undefined, so passing it to
> the statement wrapper won't do what you expect because when you finally assign
> the actual object, it wont update the wrapper's reference.
It doesn't work the way I want it too, that's for sure.  Your approach is the right one here.

Comment 116

[Dietrich Ayala (:dietrich)]

Comment on attachment 388100 [details] [diff] [review]
v1.9->v1.10 interdiff


>       if (this._result.matchCount == this._maxRichResults) {
>         // We have enough results, so stop running our queries.
>         this._stopActiveQueries();
> 
>-        break;
>+        // And finish our search.
>+        this._finishSearch(true);
>+        return;
>       }
> 
>     }
> 
>     // Notify about results if we've gotten them.
>-    if (haveMatches) {
>-      let result = this._result;
>-      result.setSearchResult(Ci.nsIAutoCompleteResult.RESULT_SUCCESS_ONGOING);
>-      result.setDefaultIndex(0);
>-      this._listener.onSearchResult(this, result);
>-    }
>+    if (haveMatches)
>+      this._notifyResults(true);

i'd prefer to not return early, esp in the middle of a loop.

>+  _notifyResults: function PAC_notifyResults(aSearchOngoing)
>+  {
>+    let result = this._result;
>+    let searchResult = result.matchCount ? "RESULT_SUCCESS" : "RESULT_NOMATCH";
>+    if (aSearchOngoing)

resultCode would be clearer, since _result is the search result itself.

>+      searchResult += "_ONGOING";
>+    result.setSearchResult(Ci.nsIAutoCompleteResult[searchResult]);
>+    result.setDefaultIndex(result.matchCount ? 0 : -1);
>+    this._listener.onSearchResult(this, this._result);

could just pass result, since you already have it local.

just nits, r=me.

Comment 117

[Shawn Wilsher :sdwilsh]

Attachment: v1.11->v1.12 interdiff

This should address comments.

Comment 118

[Dietrich Ayala (:dietrich)]

Comment on attachment 388252 [details] [diff] [review]
v1.11->v1.12 interdiff

>+  /**
>+   * Executes the specified query asynchronously.  This object will notify the
>+   * callback passed in to the constructor if we should notify.

that 2nd sentence is waterboarding in the form of a code comment.

r=me with that and the faux-private members of callback fixed.

Comment 119

[Shawn Wilsher :sdwilsh]

Attachment: v1.12->v1.13 interdiff

Addresses review comments.

Comment 120

[Shawn Wilsher :sdwilsh]

Attachment: v1.12

Comment 121

[Shawn Wilsher :sdwilsh]

Comment on attachment 388265 [details] [diff] [review]
v1.12

This was v1.12...v1.13 RSN!

Comment 122

[Shawn Wilsher :sdwilsh]

Attachment: v1.13

Comment 123

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/projects/places//rev/b2fd524aff54

Comment 124

[Benjamin Smedberg]

Pretty sure we want this to be a 1.9.2 blocker, and need to go in the alpha.

Comment 125

[Shawn Wilsher :sdwilsh]

I don't think this needs to go into the alpha, but it certainly needs to be in the first beta.

Comment 126

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/b2fd524aff54

Comment 127

[[e-mail]]

Comment on attachment 388268 [details] [diff] [review]
v1.13

>+  stopSearch: function PAC_stopSearch()
>+  {
>+    // We need to cancel our searches so we do not get any [more] results.
>+    this._stopActiveQueries();
>+
>+    this._finishSearch(false);
>+  },
...
>+  _stopActiveQueries: function PAC_stopActiveQueries()
>+  {
>+    this._pendingQueries.forEach(function(aQuery) aQuery.cancel());
>+    delete this._pendingQueries;
>+  },
Congratulations on being the first autocomplete search that throws if you stop it when it's not searching.

Comment 129

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

Comment 133

[armencho]

I cannot understand though how is this marked resolved? It is not in public 3.5.3 release, does this mean it is resolved as in "if you patch it and build it"? I would have to patch and recompile my own build for async queries to work? Right?

Comment 134

[Steve England [:stevee]]

It is resolved because it has landed on the trunk, and will be included in the next version of firefox. The patch is far too large and risky to land on the stable 3.5 branch, but will be in firefox 3.6.