1041443 - UnifiedComplete: What's _addFrecencyMatch sorting

Status: RESOLVED INVALID

(Toolkit :: Places, defect)

Description

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

_frecnecyMatches is directly manipulated only by _addFrecencyMatch, which is only called for prioritized urls... which have a constant frecency (the _process methods aren't called for _frecencyMatches). However, for every new match _addFrecencyMatch attempts to resort the array, by frecency.

Comment 1

[Marco Bonardo [:mak]]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #0)
> _frecnecyMatches is directly manipulated only by _addFrecencyMatch, which is
> only called for prioritized urls... which have a constant frecency (the
> _process methods aren't called for _frecencyMatches). However, for every new
> match _addFrecencyMatch attempts to resort the array, by frecency.

I don't think we should rely on what PriorityURLProvider does, new providers may add variable frecency values and then the code would be doing the right thing, isn't it?

Comment 2

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

It's not PriorityURLProvider, it's the code inside UnifiedComplete that hardcodes it.

Comment 3

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

  _matchPriorityUrl: function* () {
    if (!Prefs.autofillPriority)
      return;
    let priorityMatch = yield PriorityUrlProvider.getMatch(this._searchString);
    if (priorityMatch) {
      this._result.setDefaultIndex(0);
      this._addFrecencyMatch({
...
        frecency: FRECENCY_PRIORITY_DEFAULT
      });
    }
  },

Comment 4

[Marco Bonardo [:mak]]

frecency may still not be a const in future and then we'd have introduced a very hard to notice bug at that point, imo.

Comment 5

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Your call.

I just didn't get the feeling that _frecencyMatches is the door to all external stuff. That would be a remote match ;)

Comment 6

[Marco Bonardo [:mak]]

We removed _addFrecencyMatch, so no reason to fix this.