406359 - Unify the logic for url bar searches and drop down items

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ed Lee :Mardak]

There's 2 code paths for figuring out what items should show up for the location bar autocomplete. One for clicking the dropdown and another for typing text.

Both could be combined to fix bugs and simplify development later on that want to modify the ordering of both. (such as bug 395739 and bug 406358)

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Leverage the chunking and LIKE %% query for the "typed" search.

An empty search string will just search "LIKE %%" which isn't too bad -- sqlite will probably just ignore it because it matches everything.

Comment 2

[Ed Lee :Mardak]

A couple more notes..

This will fix bug 406358 because the unified query correctly uses visit_count and visit_date.

The original "TypedSearch" code always creates the sql statement each time while the "FullHistory" path uses a one-time created sql.

There were a couple tiny differences such as variable names (imageSpec vs faviconSpec) and the fact that "FullHistory" uses chunking, so it tracks things in the mCurrentResultURLs.

Using the chunking behavior would improve on responsiveness of clicking the drop down as well.

Comment 3

[Ed Lee :Mardak]

Attachment: v1.1

Found a bug while writing the testcase for bug 406358 ;) Missed removing the IsEmpty which caused empty searches to not set mCurrentChunkEndTime.

Comment 4

[Marco Bonardo [:mak]]

i this fast with very big histories like ispiked one? if i'm not wrong typed search was separated for perf reasons (the query with a disinct limited subquery is faster than the normal autocomplete one).

Comment 5

[Ed Lee :Mardak]

It uses the same chunking method as full history searches, so it should be just as fast as typing in something. I've been using it for a bit and it seems pretty fast, but then I suppose what is "fast enough?"

Adam, if you still have that very big history, could you try out a build with this patch?

https://build.mozilla.org/tryserver-builds/2007-12-04_22:[e-mail]/

Comment 6

[Gregory J. Rosmaita]

please refer to Comment #32 in the "resolved" bug 406355 for detailed information about this problem and suggested resolutions:

a. an input mechanism other than the location bar over which the user would have control over the amount and agressiveness of autocomplete

b. a property sheet akin to that in SeaMonkey that controls the autocomplete 
mechanism

refer also to:

http://groups.google.com/group/mozilla.dev.accessibility/browse_thread/thread/f6a57d49019972fe#b22e07ed8130ceda

Comment 7

[Gregory J. Rosmaita]

(In reply to comment #6)

the "severity" of this issue is listed as "normal" -- as is obvious from my comments on Bug 406355 and the cited post to the dev-accessibility list, this is a SEVERE/CRITICAL problem, as the ONLY means of entering content into the location bar is to copy-and-paste a URI typed elsewhere

Comment 8

[Adam Guthrie]

(In reply to comment #5)
> Adam, if you still have that very big history, could you try out a build with
> this patch?

I do (around 73MB) and I ran one of those builds for a few days without any noticeable perf. issues with the location bar autocomplete or when clicking the dropdown.

Comment 9

[Gregory J. Rosmaita]

(In reply to comment #6)

the "severity" of this issue is listed as "normal" -- as is obvious from my comments on Bug 406355 and the cited post to the dev-accessibility list, this is a SEVERE/CRITICAL problem, as the ONLY means of entering content into the location bar is to copy-and-paste a URI typed elsewhere

Comment 10

[Ed Lee :Mardak]

Gregory: This is bug 406359 which has little to do with a11y. Did you mean to comment in bug 407359?

Comment 11

[(not reading, please use [e-mail] instead)]

Thanks for looking into this, Edward.

Looking over the patch, your patch breaks the feature of the drop down, which is that it only gives you the urls that were typed (which is what fx 2 did.)

That could have been worked around, though.  See how we handle browser.urlbar.matchOnlyTyped.

This fix (or one that would have continued to restrict to typed urls) would have helped the performance bug #407429, but that is going to be addressed by global frecency patch, see bug #394038.

I'm not convinced this will still need fixing after the fix for bug #394038, but let's revisit this after that lands.

But r- on this current patch.

Comment 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 291050 [details] [diff] [review]
v1.1

clearing review request

Comment 13

[Ed Lee :Mardak]

Attachment: v2

Updated since global frecency

> >And should things that show up in the menu only be pages that the user has
> >typed in?
> No, I don't think the user will understand that we are using that metric, and
> also the pupose of the drop down menu is to provide a mouse only interaction
> for accessing frequently visited sites, so tying it to a keyboard interaction
> is a little counter intuitive.

So we don't need to worry about only matching Typed results.

This also fixes a potential bug where we don't show the bookmark title from the drop down.

Comment 14

[Ed Lee :Mardak]

Also, this lets us quit early as per bug 414257.

Comment 15

[Ed Lee :Mardak]

Attachment: v2.1

Now passes the testcase from bug 406358. (We stopped letting SQL do the matches which by default is true for a null match while FindInReadable default is false for null match.)

Comment 16

[Ed Lee :Mardak]

Attachment: v2.2

We can land this after bug 401869 to not need changing to FullHistorySearch.

Comment 17

[Ed Lee :Mardak]

Comment on attachment 301368 [details] [diff] [review]
v2.2

Yay code reuse, perf win, added functionality.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 301368 [details] [diff] [review]
v2.2

yes, perf of the dropdown against a large history is noticeably better, thanks. r=me.

Comment 19

[Mike Schroepfer]

Comment on attachment 301368 [details] [diff] [review]
v2.2

Perf + code cleanliness say it ain't so!

Comment 20

[Ed Lee :Mardak]

Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.133; previous revision: 1.132
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.40; previous revision: 1.39
done

Comment 21

[pal-moz]

after this checkin,

(1) start Minefield
(2) click auto-complete dropdown (right-end of locationbar)
(3) some bookmarked site are shown

intended or regression ?
no site visited, so dropdown should be disabled ?

Comment 22

[Tracy Walker [:tracy]]

(2) click auto-complete dropdown (right-end of locationbar)

that should only show "typed into the location bar previously" entries.  Of course those entries could have been bookmarked.

My understanding of this bug was not to change that. Rather make the performance of that search code match that of the awesomebar.

Comment 23

[pal-moz]

20080207_1326_firefox-3.0b4pre.en-US.win32.zip
20080207_1439_firefox-3.0b4pre.en-US.win32.zip (checkin was landed)

with _1326 build,
(1) start Minefield (about:blank)
(2) click dropdown (nothing in locationbar)
(3) nothing appears (no dropdown list appears)

with _1439 build
(3) list appears

Comment 24

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv