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

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Currently waiting for UX (bug 959571)

Comment 1

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

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".

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(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?

Comment 3

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

(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.

Comment 4

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

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.

Comment 5

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

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.

Comment 6

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

Attachment: wip

Comment 7

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

Attachment: patch v1

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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 9

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

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.

Comment 10

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

Attachment: patch v2

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.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 12

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

(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 13

[Tim Taubert [:ttaubert] (inactive)]

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);

Comment 14

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

(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.

Comment 15

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

Attachment: patch v3

Addressed comments

Comment 16

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

https://hg.mozilla.org/integration/fx-team/rev/e2aff2cd9a9a

Comment 17

[Carsten Book [:Tomcat]]

sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=37747987&tree=Fx-Team

Comment 18

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

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

Comment 19

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

Attachment: patch v4

fixed the typo, I ran the test locally and it worked

Comment 20

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

https://hg.mozilla.org/integration/fx-team/rev/80e4ae150771

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80e4ae150771