Post-facto review of UnifiedComplete

RESOLVED FIXED in mozilla34

Status

()

Toolkit
Places
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: mano)

Tracking

Trunk
mozilla34
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Blocks: 995092
Flags: firefox-backlog+

Updated

3 years ago
Whiteboard: p=8 [qa-]

Updated

3 years ago
Assignee: nobody → mano
Status: NEW → ASSIGNED
Whiteboard: p=8 [qa-] → p=8 s=33.1 [qa-]

Updated

3 years ago
Iteration: --- → 33.2
Points: --- → 8
QA Whiteboard: [qa-]
Whiteboard: p=8 s=33.1 [qa-]

Updated

3 years ago
Iteration: 33.2 → 33.3
(Reporter)

Updated

3 years ago
Blocks: 1040335
(Reporter)

Updated

3 years ago
No longer blocks: 995092

Comment 1

3 years ago
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

3 years ago
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 :-)
(Reporter)

Comment 3

3 years ago
(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.
(Reporter)

Comment 4

3 years ago
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.
Depends on: 1041421
Depends on: 1041428
Depends on: 1041429
Depends on: 1041433
Depends on: 1041443
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.