Design the interaction between the UI for top search provider and the places database

VERIFIED FIXED in Firefox 31

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: phlsa, Assigned: mak)

Tracking

(Blocks 1 bug)

28 Branch
Firefox 31
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [search] p=8 s=it-31c-30a-29b.3 [qa-])

Attachments

(1 attachment, 4 obsolete attachments)

Whiteboard: p=3
Whiteboard: p=3 → [feature] p=3
Whiteboard: [feature] p=3 → [feature] p=3 [qa+]
Whiteboard: [feature] p=3 [qa+] → [search] p=3 [qa+]
Status: NEW → ASSIGNED
Whiteboard: [search] p=3 [qa+] → [search] p=8 s=it-30c-29a-28b.3 [qa+]
Assignee: nobody → mak77
Whiteboard: [search] p=8 s=it-30c-29a-28b.3 [qa+] → [search] p=8 s=it-30c-29a-28b.3 [qa-]
Whiteboard: [search] p=8 s=it-30c-29a-28b.3 [qa-] → [search] p=8 [qa-]
Assignee

Comment 1

5 years ago
Summing up technical issues found while investigating this:

1. need to decide whether to go the "actions" path (like switch-to-tab) or just set a "style" property for the match.  Sounds like we don't want to expose the whole uri to the user. Actions should allow a little bit more styling flexibility.

2. As title we will just use name of the engine

3. on full match (users types everything of the domain) we should just suggest nothing, Enter will just navigate to the domain as usual

4. need to merge frecencies, likely with a fixed value. This may be challenging to get right since inline completion has frecency = SUM(pages_frecency)

5. the current autocomplete architecture runs indipendent inline and popup queries, these can return non-matching results, and that would be an issue for this feature since we may have inline suggest an engine while popup doesn't or viceversa.

First of all I think I will try to make an experimental patch that merges back inline and popup searches into a single component. There are a couple tricky parts but should be feasible. The interaction with autocomplete will change a little bit, so I will try to keep this under a pref to test it in Nightly and eventually disable it. At that point, if results are satisfying, issue 5 would be solved and we could move on with the rest that, apart frecency merging, should be "easy".
(In reply to Marco Bonardo [:mak] from comment #1)
> Summing up technical issues found while investigating this:
> 
> 1. need to decide whether to go the "actions" path (like switch-to-tab) or
> just set a "style" property for the match.  Sounds like we don't want to
> expose the whole uri to the user. Actions should allow a little bit more
> styling flexibility.
Just to be sure: This doesn't have any impact on the user, right?
Assignee

Comment 3

5 years ago
(In reply to Philipp Sackl [:phlsa] from comment #2)
> > 1. need to decide whether to go the "actions" path (like switch-to-tab) or
> > just set a "style" property for the match.  Sounds like we don't want to
> > expose the whole uri to the user. Actions should allow a little bit more
> > styling flexibility.
> Just to be sure: This doesn't have any impact on the user, right?

no, should be a mere implementation detail.
Whiteboard: [search] p=8 [qa-] → [search] p=8 s=it-31c-30a-29b.1 [qa-]
Whiteboard: [search] p=8 s=it-31c-30a-29b.1 [qa-] → [search] p=8 s=it-31c-30a-29b.2 [qa-]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Assignee

Updated

5 years ago
Depends on: 991682
Assignee

Updated

5 years ago
Depends on: 754265
Assignee

Updated

5 years ago
Depends on: 810930
Assignee

Updated

5 years ago
Blocks: 810930
No longer depends on: 810930
Assignee

Updated

5 years ago
Blocks: 887864
Assignee

Updated

5 years ago
Blocks: 993372
Assignee

Comment 4

5 years ago
The current plan is to get a quick-review on the new component (that is basically a merge of the 2 old autocomplete components) and land it.
Bug 993372 will take care of converting existing tests and check for sanity through them.
Finally, before enabling the component in Nightly, we will go through a post-review process (that may also happen in parallel to the tests conversion, since may take some time).
At that point we should be able to start hammering edges to make it possible to remove the old autocomplete and make the new one the default.
Assignee

Comment 5

5 years ago
note, we still need bug 754265 to be able to complete the merge, while bug 991682 just landed, making possible to use Sqlite.jsm in the new code.
Assignee

Comment 6

5 years ago
Posted patch wip (obsolete) — Splinter Review
Assignee

Comment 7

5 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Before starting to add complication to the thing, I think this is a good basis.
We can start converting tests to verify the expected behavior and fix eventual issues before moving on with a deeper review process.

Feel free to ask regarding any doubts.

Note: this builds on top of the patch in bug 754265.
Attachment #8403902 - Attachment is obsolete: true
Attachment #8404062 - Flags: review?(ttaubert)
Comment on attachment 8404062 [details] [diff] [review]
patch v1

Review of attachment 8404062 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! Where's the part that switches to the unified autocomplete if pref'ed on? Is that still to come in another bug?

::: browser/base/content/urlbarBindings.xml
@@ +70,5 @@
>  
> +        try {
> +          this._prefs.getBoolPref("unifiedcomplete");
> +          this.setAttribute("autocompletesearch", "unifiedcomplete");
> +        } catch (ex) {}

Fancy, shouldn't we still check |if (this._prefs.getBoolPref("unifiedcomplete")) { ... }| otherwise we'd enable it even when I set it to false, no? Even if that isn't the case I feel like that's a little less "magic".

@@ +605,5 @@
>                  this._mayTrimURLs = this._prefs.getBoolPref(aData);
>                  break;
> +              case "unifiedcomplete":
> +                try {
> +                  let unifiedcomplete = this._prefs.getBoolPref(aData);

We should try/catch-wrap only the fallible part, i.e. getBoolPref().

@@ +610,5 @@
> +                  if (unifiedcomplete) {
> +                    this.setAttribute("autocompletesearch", "unifiedcomplete");
> +                  } else {
> +                    this.setAttribute("autocompletesearch", "urlinline history");
> +                  }

Maybe:

this.setAttribute("autocompletesearch",
  unifiedcomplete ? "unifiedcomplete" : "urlinline history");

(Or use a variable.)

::: toolkit/components/places/UnifiedComplete.js
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: sw=2 ts=2 sts=2 expandtab
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

"use strict";

@@ +63,5 @@
> +// Telemetry probes.
> +const TELEMETRY_1ST_RESULT = "PLACES_AUTOCOMPLETE_1ST_RESULT_TIME_MS";
> +
> +// The default frecency value used to insert priority results.
> +const FRECENCY_PRIORITY_DEFAULT = 1000

Nit: missing semicolon;

And maybe call it FRECENCY_PRIORITY_DEFAULT_MS? I like it when the unit used is obvious.

@@ +255,5 @@
> + */
> +XPCOMUtils.defineLazyGetter(this, "SwitchToTabStorage", () => Object.seal({
> +  _conn: null,
> +  // Temporary cache used while the database connection is not available.
> +  _cache: new Set(),

"cache" doesn't seem like a good name for that. Would "queue" be better? Something that indicates we have a list of URIs that wait to be added.

@@ +257,5 @@
> +  _conn: null,
> +  // Temporary cache used while the database connection is not available.
> +  _cache: new Set(),
> +  initDatabase: function (conn) {
> +    return Task.spawn(function* () {

initDatabase: Task.async(function* (conn) {

@@ +293,5 @@
> +  add: function (uri) {
> +    if (this._conn) {
> +      this._conn.executeCached(sql(
> +        "INSERT OR REPLACE INTO moz_openpages_temp (url, open_count)",
> +          "VALUES ( :page_url, IFNULL( (SELECT open_count + 1",

Where does :page_url come from? Is that missing?

@@ +302,5 @@
> +                 ")"
> +      ), { url: uri.spec });
> +    } else {
> +      this._cache.add(uri);
> +    }

How about doing this at the top of the function:

add: function (uri) {
  if (!this._conn) {
    this._cache.add(uri);
    return;
  }

  this._conn.executeCached(sql( ...
}

I feel like this might be easier to read?

@@ +314,5 @@
> +        "WHERE url = :url"
> +      ), { url: uri.spec });
> +    } else {
> +      this._cache.delete(uri);
> +    }

Same with the early return here?

@@ +318,5 @@
> +    }
> +  },
> +
> +  shutdown: function () {
> +    this._conn = null;

We should probably call |this._cache.clear()| here as well.

@@ +328,5 @@
> + */
> +XPCOMUtils.defineLazyGetter(this, "Prefs", () => {
> +  let prefs = new Preferences(PREF_BRANCH);
> +
> +  let loadPrefs = function () {

function loadPrefs() {

@@ +451,5 @@
> +    this._originalSearchString.slice(pathIndex)
> +  );
> +
> +  this._enableActions = searchParam.split(" ").indexOf("enable-actions") != -1;
> + 

Nit: white space

@@ +454,5 @@
> +  this._enableActions = searchParam.split(" ").indexOf("enable-actions") != -1;
> + 
> +  this._listener = autocompleteListener;
> +  this._autocompleteSearch = autocompleteSearch;
> + 

Nit: white space

@@ +505,5 @@
> +   */
> +  _sleepDeferred: null,
> +  _sleep: function (aTimeMs) {
> +    if (!this._sleepTimer)
> +      this._sleepTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

Use Timer.jsm?

@@ +524,5 @@
> +  filterTokens: function (tokens) {
> +    // Set the proper behavior while filtering tokens.
> +    for (let i = tokens.length - 1; i >= 0; i--) {
> +      switch (tokens[i]) {
> +        case Prefs.restrictHistoryToken:

Could this be a Map that returns the behavior for a given token? We'd only have to special-case the restrictOpenPageToken and that should be faster I think. And less code. And easier to read :)

@@ +608,5 @@
> +                    this._switchToTabQuery,
> +                    this._searchQuery ];
> +
> +    if (this._searchTokens.length == 1) {
> +      yield this._matchPriorityUrl();

yield* this._matchPriorityUrl();

Also, check |this.pending| here? I really wish we had a better of aborting tasks. Something like:

Task.asyncConditional = Task.async(function* (cont, iter) {
  for (let i of iter) {
    if (!cont()) break;
    yield i;
    if (!cont()) break;
  }
});

Task.asyncConditional(() => this.pending, function* (conn) {
  ...
});

@@ +626,5 @@
> +      } else {
> +        // The host query is executed immediately, while any other is delayed
> +        // to avoid overloading the connection.
> +        let [ query, params ] = this._hostQuery;
> +        yield conn.executeCached(query, params, this._onResultRow.bind(this));

Should we check |this.pending| here?

@@ +630,5 @@
> +        yield conn.executeCached(query, params, this._onResultRow.bind(this));
> +      }
> +    }
> +
> +    yield this._sleep(Prefs.delay);

If cancel() would call this._sleepDeferred.reject() the generator would be done here. Search.execute() would also reject in that case thought which we then would probably want to ignore instead of passing to Cu.reportError().

@@ +635,5 @@
> +
> +    for (let [query, params] of queries) {
> +      yield conn.executeCached(query, params, this._onResultRow.bind(this));
> +      if (!this.pending)
> +        break;

if (!this.pending) return ?

@@ +641,5 @@
> +
> +    // 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.pending &&

We shouldn't need to check |this.pending| here if we return above.

@@ +655,5 @@
> +    }
> +
> +    // If we didn't find enough matches and we have some frecency-driven
> +    // matches, add them.
> +    if (this.pending && this._frecencyMatches &&

Same here, |this.pending| was checked after yielding or we don't need to check because we didn't yield.

@@ +695,5 @@
> +        match = this._processUrlRow(row);
> +        break;
> +      case QUERYTYPE_FILTERED:
> +      case QUERYTYPE_KEYWORD:
> +        match = this._processRow(row); 

Nit: trailing white space.

@@ +714,5 @@
> +  _addMatch: function (match) {
> +    let notifyResults = false;
> +
> +    if (this._frecencyMatches && this._frecencyMatches.length > 0) {
> +      for (let i in this._frecencyMatches) {

for (let i = 0; i < this._frecencyMatches.length; i++) {

@@ +745,5 @@
> +
> +    if (this._result.matchCount == Prefs.maxRichResults || !this.pending) {
> +      // We have enough results, so stop running our search.
> +      this.cancel();
> +      throw StopIteration;

That seems really magic to me. _addMatch() is called from so many places, what exactly is the StopIteration exception used for? Can we do this differently?

@@ +1166,5 @@
> +    // Note: We don't use previousResult to make sure ordering of results are
> +    //       consistent.  See bug 412730 for more details.
> +
> +    this._currentSearch = new Search(searchString, searchParam, listener,
> +                                     this, this);

What's the reason for passing |this| twice here? Legacy? We should fix that.

@@ +1177,5 @@
> +    }
> +
> +    this.getDatabaseHandle().then(conn => this._currentSearch.execute(conn))
> +                            .then(() => this.finishSearch(true),
> +                                  Cu.reportError);

This looks like we could end up calling .finishSearch() when shortly after .execute() has been resolved something call startSearch(). How about having a security check like this:

let search = this._currentSearch;
this.getDatabaseHandle().then(conn => this._currentSearch.execute(conn))
                        .then(() => {
                          if (search == this._currentSearch) {
                            this.finishSearch(true);
                          }
                        }, Cu.reportError);

(Needs better formatting :)
Attachment #8404062 - Flags: review?(ttaubert) → feedback+
Assignee

Comment 9

5 years ago
good comments! I will try to fix them asap.
some replies inline (things I didn't reply will just be fixed in next iteration).

(In reply to Tim Taubert [:ttaubert] from comment #8)
> Looking good! Where's the part that switches to the unified autocomplete if
> pref'ed on? Is that still to come in another bug?

it's just the code in urlbarBindings.xml that switches the autocomplete.

> And maybe call it FRECENCY_PRIORITY_DEFAULT_MS? I like it when the unit used
> is obvious.

frecency is just a number, no unit! I just took a reasonable frecency value to mixup results, it's the same we do in newtab page.

> @@ +505,5 @@
> > +   */
> > +  _sleepDeferred: null,
> > +  _sleep: function (aTimeMs) {
> > +    if (!this._sleepTimer)
> > +      this._sleepTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> 
> Use Timer.jsm?

it may look better, but sounds like Timer.jsm is not reusing timer instances. Here I was trying to avoid recreating a new instance every time.  Probably won't make much difference, but still this code runs before a search and we should try to avoid any useless work there, imo.
 
> @@ +524,5 @@
> > +  filterTokens: function (tokens) {
> > +    // Set the proper behavior while filtering tokens.
> > +    for (let i = tokens.length - 1; i >= 0; i--) {
> > +      switch (tokens[i]) {
> > +        case Prefs.restrictHistoryToken:
> 
> Could this be a Map that returns the behavior for a given token? We'd only
> have to special-case the restrictOpenPageToken and that should be faster I
> think. And less code. And easier to read :)

I can try, I didn't touch it to avoid introducing unexpected regressions, this is just the old code copied over.  But sure, it may be better, so I will try.
 
> @@ +608,5 @@
> > +                    this._switchToTabQuery,
> > +                    this._searchQuery ];
> > +
> > +    if (this._searchTokens.length == 1) {
> > +      yield this._matchPriorityUrl();
> 
> yield* this._matchPriorityUrl();

Hm, what's the point? (sorry that operator is pretty new and I'm not used to it)

> @@ +630,5 @@
> > +        yield conn.executeCached(query, params, this._onResultRow.bind(this));
> > +      }
> > +    }
> > +
> > +    yield this._sleep(Prefs.delay);
> 
> If cancel() would call this._sleepDeferred.reject() the generator would be
> done here. Search.execute() would also reject in that case thought which we
> then would probably want to ignore instead of passing to Cu.reportError().

maybe I can somehow filter that error. I will try, no promises for now though, I don't want to complicate things to much now.

> @@ +745,5 @@
> > +
> > +    if (this._result.matchCount == Prefs.maxRichResults || !this.pending) {
> > +      // We have enough results, so stop running our search.
> > +      this.cancel();
> > +      throw StopIteration;
> 
> That seems really magic to me. _addMatch() is called from so many places,
> what exactly is the StopIteration exception used for? Can we do this
> differently?

It's done to tell Sqlite.jsm we don't need any more results (so it can cancel the query internally), we can't do that differently since it's how Sqlite.jsm works. you throw StopIteration and it stops sending back rows to you, plus cancels the underlying query.
Though if you have ideas you may suggest them separately to improve its API.

> @@ +1166,5 @@
> > +    // Note: We don't use previousResult to make sure ordering of results are
> > +    //       consistent.  See bug 412730 for more details.
> > +
> > +    this._currentSearch = new Search(searchString, searchParam, listener,
> > +                                     this, this);
> 
> What's the reason for passing |this| twice here? Legacy? We should fix that.

Cause in this case "this" implements 2 completely different interfaces, if you look at the arguments in Search, it needs a listener and an autocomplete implementation. Surely I may pass "this" just once, but then the code in Search would be less generic and assume autocomplete is also a listener or viceversa, that is true only in this very specific case.
Assignee

Comment 10

5 years ago
Posted patch patch v2 (obsolete) — Splinter Review
fixes your comments, apart some I replied to in comment 9 (though I used a Map in filterTokens and handled _sleep cancel, somehow). I also added a couple comments in points that were unclear.
Attachment #8404062 - Attachment is obsolete: true
Attachment #8405040 - Flags: review?(ttaubert)
Assignee

Updated

5 years ago
Assignee

Updated

5 years ago
No longer blocks: 810930, 887864
(In reply to Marco Bonardo [:mak] from comment #9)
> > And maybe call it FRECENCY_PRIORITY_DEFAULT_MS? I like it when the unit used
> > is obvious.
> 
> frecency is just a number, no unit! I just took a reasonable frecency value
> to mixup results, it's the same we do in newtab page.

Hah, sorry. I mistook it for the time we sleep before executing search queries. This obviously can't be 1000ms.

> it may look better, but sounds like Timer.jsm is not reusing timer
> instances. Here I was trying to avoid recreating a new instance every time. 
> Probably won't make much difference, but still this code runs before a
> search and we should try to avoid any useless work there, imo.

Ok, good point.

> I can try, I didn't touch it to avoid introducing unexpected regressions,
> this is just the old code copied over.  But sure, it may be better, so I
> will try.

Thanks, here I was hoping we have tests for that :)

> > > +    if (this._searchTokens.length == 1) {
> > > +      yield this._matchPriorityUrl();
> > 
> > yield* this._matchPriorityUrl();
> 
> Hm, what's the point? (sorry that operator is pretty new and I'm not used to
> it)

So... with Task.jsm expecting promises to be yielded it was confusing me a little because I was expecting _matchPriorityUrl() to return a promise that is then yielded. Instead _matchPriorityUrl() itself is a generator that is yielded, which Task.jsm seems to support but I thought it would be better to explicitly delegate to the generator returned by _matchPriorityUrl(), that's why I proposed using the delegating yield*.

> > > +    yield this._sleep(Prefs.delay);
> > 
> > If cancel() would call this._sleepDeferred.reject() the generator would be
> > done here. Search.execute() would also reject in that case thought which we
> > then would probably want to ignore instead of passing to Cu.reportError().
> 
> maybe I can somehow filter that error. I will try, no promises for now
> though, I don't want to complicate things to much now.

Ok, fair enough. This can totally be handled as a follow-up. FWIW, catching everything else than a cancel() call should be easy to do.

> > > +    if (this._result.matchCount == Prefs.maxRichResults || !this.pending) {
> > > +      // We have enough results, so stop running our search.
> > > +      this.cancel();
> > > +      throw StopIteration;
> > 
> > That seems really magic to me. _addMatch() is called from so many places,
> > what exactly is the StopIteration exception used for? Can we do this
> > differently?
> 
> It's done to tell Sqlite.jsm we don't need any more results (so it can
> cancel the query internally), we can't do that differently since it's how
> Sqlite.jsm works. you throw StopIteration and it stops sending back rows to
> you, plus cancels the underlying query.
> Though if you have ideas you may suggest them separately to improve its API.

If that's how we do it now I'm fine with leaving it as is and filing a follow-up to come up with something more obvious and easier to understand? I don't have a great idea right now without digging more into the code.

> > What's the reason for passing |this| twice here? Legacy? We should fix that.
> 
> Cause in this case "this" implements 2 completely different interfaces, if
> you look at the arguments in Search, it needs a listener and an autocomplete
> implementation. Surely I may pass "this" just once, but then the code in
> Search would be less generic and assume autocomplete is also a listener or
> viceversa, that is true only in this very specific case.

Hmmm, yeah that's kind of what I assumed. What is the reason for this flexibility? Can we get rid of that in a follow-up? This doesn't seem to be useful if |Search| is only used inside UnifiedComplete.js?
Assignee

Comment 12

5 years ago
(In reply to Tim Taubert [:ttaubert] from comment #11)
> So... with Task.jsm expecting promises to be yielded it was confusing me a
> little because I was expecting _matchPriorityUrl() to return a promise that
> is then yielded. Instead _matchPriorityUrl() itself is a generator that is
> yielded, which Task.jsm seems to support but I thought it would be better to
> explicitly delegate to the generator returned by _matchPriorityUrl(), that's
> why I proposed using the delegating yield*.

oh, I thought yield* was mostly for recursive generators... I'm not sure there's an actual win here, I'd like to find some better docs about it, the ones I found are lacking good examples and best practices...

> > > > +    yield this._sleep(Prefs.delay);
> > > 
> > > If cancel() would call this._sleepDeferred.reject() the generator would be
> > > done here. Search.execute() would also reject in that case thought which we
> > > then would probably want to ignore instead of passing to Cu.reportError().
> > 
> > maybe I can somehow filter that error. I will try, no promises for now
> > though, I don't want to complicate things to much now.
> 
> Ok, fair enough. This can totally be handled as a follow-up. FWIW, catching
> everything else than a cancel() call should be easy to do

In the latest version I actually threw an Error with .becauseCancelled, and I catch it and return in case.

> > Cause in this case "this" implements 2 completely different interfaces, if
> > you look at the arguments in Search, it needs a listener and an autocomplete
> > implementation. Surely I may pass "this" just once, but then the code in
> > Search would be less generic and assume autocomplete is also a listener or
> > viceversa, that is true only in this very specific case.
> 
> Hmmm, yeah that's kind of what I assumed. What is the reason for this
> flexibility? Can we get rid of that in a follow-up? This doesn't seem to be
> useful if |Search| is only used inside UnifiedComplete.js?

Just that I like to write code that is reusable and readable, without going too much into trying to predict future. In this case having 2 well named arguments rather than something like autocompleteAndListener was improving readability.
Comment on attachment 8405040 [details] [diff] [review]
patch v2

Review of attachment 8405040 [details] [diff] [review]:
-----------------------------------------------------------------

Great work!

::: toolkit/components/places/UnifiedComplete.js
@@ +362,5 @@
> +    store.tokenToBehaviorMap.set(store.restrictTagToken, "tag");
> +    store.tokenToBehaviorMap.set(store.restrictOpenPageToken, "openpage");
> +    store.tokenToBehaviorMap.set(store.matchTitleToken, "title");
> +    store.tokenToBehaviorMap.set(store.matchURLToken, "url");
> +    store.tokenToBehaviorMap.set(store.restrictTypedToken, "typed");

You could as well do:

store.tokenToBehaviorMap = new Map([
  [store.restrictHistoryToken, "history"],
  [store.restrictBookmarkToken, "bookmark"],
  ...
]);

@@ +540,5 @@
> +      if (!behavior || (behavior == "openpage" && !this._enableActions)) {
> +        continue;
> +      }
> +      this.setBehavior(behavior);
> +      tokens.splice(i, 1);

Maybe:

if (behavior && (behavior != "openpage" || this._enableActions)) {
  this.setBehavior(behavior);
  tokens.splice(i, 1);
}

@@ +562,5 @@
> +      this._sleepTimer.cancel();
> +    if (this._sleepDeferred) {
> +      let e = new Error();
> +      e.becauseCancelled = true;
> +      this._sleepDeferred.reject(e);

Should we null this._sleepDeferred? Can we leak by not doing so? Not sure.

@@ +624,5 @@
> +    try {
> +      yield this._sleep(Prefs.delay);
> +    } catch (e if e.becauseCancelled) {
> +      return;
> +    }

So I actually think doing this just for this one line here doesn't make sense. We could as well have:

yield this._sleep(Prefs.delay);
if (!this.pending) return;

Without this._sleep() being ever rejected. The reject() version would make sense if we wanted to get rid of all the |this.pending| checks after async operations in this method. We would need to somehow reject the whole task returned by execute()... And we would need to check for |becauseCancelled| below in UnifiedComplete.startSearch() and not report an error in that case.

It maybe wouldn't be too hard if we used a promise that wraps the task run by execute() and that we then could reject in cancel().

I think we should go with checking |this.pending| for now and keep my suggestion for a follow-up?

@@ +702,5 @@
> +    // We keep this array in reverse order, so we can walk it and remove stuff
> +    // from it in one pass.  Notice that for frecency reverse order means from
> +    // lower to higher.
> +    this._frecencyMatches.sort((a, b) =>
> +      a.frecency < b.frecency ? -1 : a.frecency > b.frecency ? 1 : 0

Can't we just do:

this._frecencyMatches.sort((a, b) => a.frecency - b.frecency);
Attachment #8405040 - Flags: review?(ttaubert) → review+
Whiteboard: [search] p=8 s=it-31c-30a-29b.2 [qa-] → [search] p=8 s=it-31c-30a-29b.3 [qa-]
Assignee

Comment 14

5 years ago
(In reply to Tim Taubert [:ttaubert] from comment #13)
> @@ +562,5 @@
> > +      this._sleepTimer.cancel();
> > +    if (this._sleepDeferred) {
> > +      let e = new Error();
> > +      e.becauseCancelled = true;
> > +      this._sleepDeferred.reject(e);
> 
> Should we null this._sleepDeferred? Can we leak by not doing so? Not sure.

I don't think we can leak, I'm doing that regardless for code correctness though.


> Without this._sleep() being ever rejected. The reject() version would make
> sense if we wanted to get rid of all the |this.pending| checks after async
> operations in this method. We would need to somehow reject the whole task
> returned by execute()... And we would need to check for |becauseCancelled|
> below in UnifiedComplete.startSearch() and not report an error in that case.
> 
> It maybe wouldn't be too hard if we used a promise that wraps the task run
> by execute() and that we then could reject in cancel().

May be a good idea, indeed I don't like much the .pending checks, we can improve that later I suppose, so as you suggested I just reverted to a simple pending check for now. I will file a follow-up to investigate a better way to bail-out.
Assignee

Comment 15

5 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Addressed comments
Attachment #8405040 - Attachment is obsolete: true
Assignee

Comment 16

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/e2aff2cd9a9a
Target Milestone: --- → Firefox 31
sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=37747987&tree=Fx-Team
Assignee

Comment 18

5 years ago
that's... surprising... but not too much, looks like I have a typo in tabbrowser.xml...
Assignee

Comment 19

5 years ago
Posted patch patch v4Splinter Review
fixed the typo, I ran the test locally and it worked
Attachment #8406049 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/80e4ae150771
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.