Closed
Bug 406359
Opened 17 years ago
Closed 17 years ago
Unify the logic for url bar searches and drop down items
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta4
People
(Reporter: Mardak, Assigned: Mardak)
References
Details
(Keywords: perf)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
dietrich
:
review+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•17 years ago
|
||
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.
Assignee | ||
Comment 2•17 years ago
|
||
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.
Assignee | ||
Comment 3•17 years ago
|
||
Found a bug while writing the testcase for bug 406358 ;) Missed removing the IsEmpty which caused empty searches to not set mCurrentChunkEndTime.
Attachment #291015 -
Attachment is obsolete: true
Attachment #291050 -
Flags: review?(dietrich)
Attachment #291015 -
Flags: review?(dietrich)
Comment 4•17 years ago
|
||
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).
Assignee | ||
Comment 5•17 years ago
|
||
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:14-edward.lee@engineering.uiuc.edu-aw.bar.dl.mgr/
Comment 6•17 years ago
|
||
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•17 years ago
|
||
(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•17 years ago
|
||
(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•17 years ago
|
||
(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
Assignee | ||
Comment 10•17 years ago
|
||
Gregory: This is bug 406359 which has little to do with a11y. Did you mean to comment in bug 407359?
Comment 11•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 291050 [details] [diff] [review]
v1.1
clearing review request
Attachment #291050 -
Flags: review?(dietrich)
Assignee | ||
Comment 13•17 years ago
|
||
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.
Attachment #291050 -
Attachment is obsolete: true
Attachment #299618 -
Flags: review?(seth)
Assignee | ||
Comment 14•17 years ago
|
||
Also, this lets us quit early as per bug 414257.
Assignee | ||
Updated•17 years ago
|
Attachment #299618 -
Flags: review?(seth)
Assignee | ||
Comment 15•17 years ago
|
||
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.)
Attachment #299618 -
Attachment is obsolete: true
Attachment #301077 -
Flags: review?
Assignee | ||
Comment 16•17 years ago
|
||
We can land this after bug 401869 to not need changing to FullHistorySearch.
Attachment #301077 -
Attachment is obsolete: true
Attachment #301368 -
Flags: review?
Attachment #301077 -
Flags: review?
Assignee | ||
Comment 17•17 years ago
|
||
Comment on attachment 301368 [details] [diff] [review]
v2.2
Yay code reuse, perf win, added functionality.
Attachment #301368 -
Flags: review? → review?(dietrich)
Comment 18•17 years ago
|
||
Comment on attachment 301368 [details] [diff] [review]
v2.2
yes, perf of the dropdown against a large history is noticeably better, thanks. r=me.
Attachment #301368 -
Flags: review?(dietrich) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #301368 -
Flags: approval1.9?
Comment 19•17 years ago
|
||
Comment on attachment 301368 [details] [diff] [review]
v2.2
Perf + code cleanliness say it ain't so!
Attachment #301368 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 20•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
Comment 21•17 years ago
|
||
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•17 years ago
|
||
(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.
Status: RESOLVED → VERIFIED
Comment 23•17 years ago
|
||
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•15 years ago
|
||
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•