995094 - Post-facto review of UnifiedComplete

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

The first implementation, disabled by default, will land with a quick review pass, so we can proceed in parallel with its implementation. Though, before we enable the component in Nightly, we need a more involving review pass.

Note that the component behavior is basically the same as the old 2 autocomplete search components we had, that behavior has already been reviewed in the past and is well tests in Firefox from a lot of years. So this should mostly concentrate on the new behaviors due to the merge and on js code/perf improvements.

Comment 1

[:Paolo Amadini]

The UnifiedComplete review may not be the right bug to address the following question, since the UnifiedComplete code is a port of existing functionality and we may want to keep any changes separate, to catch regressions more easily.

But, while I was looking at the UnifiedComplete code, it took some time for me to figure out the logic behind the following special tokens:

const PREF_RESTRICT_HISTORY =   [ "restrict.history",      "^" ];
const PREF_RESTRICT_BOOKMARKS = [ "restrict.bookmark",     "*" ];
const PREF_RESTRICT_TYPED =     [ "restrict.typed",        "~" ];
const PREF_RESTRICT_TAG =       [ "restrict.tag",          "+" ];
const PREF_RESTRICT_SWITCHTAB = [ "restrict.openpage",     "%" ];
const PREF_MATCH_TITLE =        [ "match.title",           "#" ];
const PREF_MATCH_URL =          [ "match.url",             "@" ];

There are three mappings involved! It seems like a candidate for simplification. I found out that they are related to special address bar search functionality, which is indeed documented at https://support.mozilla.org/en-US/kb/awesome-bar-find-your-bookmarks-history-and-tabs but not very discoverable from a UX perspective.

My first question is whether anyone has ever _customized_ these characters. It seems to me the about:config options are overkill, we could measure but I feel this would be closer to 0% than 2%.

The other observation is that the ability to search in history but exclude bookmarks doesn't seem very useful, and the "typed" restriction is also difficult to understand (and unlikely to yield useful results). Some were implemented following requests made in 2008 (bug 422944), but Firefox has changed a lot and the use case may not apply today, or it might be better served in other ways.

And I wonder how many users are actually using this functionality. Some of the options seem useful indeed, like searching only bookmarks or titles, but if they're used by less than 2% of our users, it seems like something that can be addressed by an add-on, or alternatively made more discoverable in our UX if we care about it, so that usage increases and we make Firefox better for everyone and not only a small number of knowledgeable users.

Comment 2

[:Paolo Amadini]

And, at least from what I see in the patch in bug 424557, it seems that making any of those about:config preferences empty made the behavior the default one, and I don't see any hint about that functionality being preserved here. This means we might as well remove support for the preferences now.

So this is indeed related to the UnifiedComplete code review :-)

Comment 3

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #1)
> My first question is whether anyone has ever _customized_ these characters.
> It seems to me the about:config options are overkill, we could measure but I
> feel this would be closer to 0% than 2%.

yep, this is a bit off topic here, the filters are not discoverable at all. That is bug 541891.

> And I wonder how many users are actually using this functionality. Some of
> the options seem useful indeed, like searching only bookmarks or titles, but
> if they're used by less than 2% of our users, it seems like something that
> can be addressed by an add-on, or alternatively made more discoverable in
> our UX if we care about it, so that usage increases and we make Firefox
> better for everyone and not only a small number of knowledgeable users.

The ability to filter is definitely useful, the way we expose it it's definitely not. But again, it's a different issue, we are just porting the de-fact status.
Also, bug 530209.

(In reply to :Paolo Amadini from comment #2)
> And, at least from what I see in the patch in bug 424557, it seems that
> making any of those about:config preferences empty made the behavior the
> default one, and I don't see any hint about that functionality being
> preserved here. This means we might as well remove support for the
> preferences now.

We have default values for each pref, and from that time we have moved prefs to firefox.js (originally they were hidden prefs). The code you are looking it is the cpp version of autocomplete, we had at least a couple revisions from there.

Comment 4

[Marco Bonardo [:mak]]

just to be clear, the goal here is to unify the autocomplete components so that in future it will be easier to add more stuff into a single point without having to manage crazy concurrency across components.
Anything else is a non-goal.

Comment 5

[Asaf Romano (gone)]

Not much, wasn't it?

Marco found and fixed some issues himself during tests migration. I filed few other issues (see blockers list). I intentionally didn't file or comment on styling nits and other matters of personal preferences. I think we can close this bug at this point and consider the module reviewed.

****

Since I'm not going to file a bug for that, I'm logging this here:

XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                  "resource://gre/modules/osfile.jsm");

should be removed. OS.File isn't used.