Closed
Bug 378079
Opened 17 years ago
Closed 17 years ago
AutoComplete returns invalid rows when more than one AutoCompleteSearch is used
Categories
(Toolkit :: Autocomplete, defect)
Toolkit
Autocomplete
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha8
People
(Reporter: matt, Assigned: matt)
References
Details
Attachments
(2 files)
2.37 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
11.12 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070418 Songbird/0.3pre (00000000000000) The AutoCompleteController returns incorrect cell text in cases where more than one nsIAutoCompleteSearch is used and RESULT_NOMATCH is returned. Scenario: Pretend that... * two nsIAutoCompleteSearch sources have been registered * a search has been completed * the first search returned RESULT_NOMATCH * the second search returned RESULT_SUCCESS with two results The popup tree is being built, and this is the first call to RowIndexToSearch: http://lxr.mozilla.org/seamonkey/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#1373 1373 nsAutoCompleteController::RowIndexToSearch(PRInt32 aRowIndex, PRInt32 *aSearchIndex, PRInt32 *aItemIndex) 1374 { 1375 *aSearchIndex = -1; 1376 *aItemIndex = -1; 1377 1378 PRUint32 count; 1379 mSearches->Count(&count); 1380 PRUint32 index = 0; 1381 for (PRUint32 i = 0; i < count; ++i) { 1382 nsCOMPtr<nsIAutoCompleteResult> result; 1383 mResults->GetElementAt(i, getter_AddRefs(result)); 1384 if (!result) 1385 continue; 1386 1387 PRUint16 searchResult; 1388 result->GetSearchResult(&searchResult); 1389 1390 PRUint32 rowCount = 1; 1391 if (searchResult == nsIAutoCompleteResult::RESULT_SUCCESS) { 1392 result->GetMatchCount(&rowCount); 1393 } 1394 1395 if (index + rowCount-1 >= (PRUint32) aRowIndex) { 1396 *aSearchIndex = i; 1397 *aItemIndex = aRowIndex - index; 1398 return NS_OK; 1399 } 1400 1401 index += rowCount; 1402 } 1403 1404 return NS_OK; 1405 } Since this is the first row, aRowIndex=0 On line 1390 rowCount is initialized to 1, but is never updated by result->GetMatchCount since the first search did not succeed. Therefore in the if statement on 1395... (index + rowCount-1 >= (PRUint32) aRowIndex) == ( 0 + 1 - 1 >= 0 ) == true ...causing an incorrect aSearchIndex and aItemIndex to be returned. The visual result is a popup with two rows, the first row blank, and the second row showing the first result of the second search. A fix is to initialize rowCount = 0, and then check rowCount != 0 in the if statement on 1395 (required as these are unsigned ints). I'll post a patch shortly. Reproducible: Always
Assignee | ||
Comment 1•17 years ago
|
||
Comment 2•17 years ago
|
||
Thanks for looking into this! I've been meaning to fix it ever since I tried to make search suggest use the right autocomplete APIs instead of the hack it currently uses.
Status: UNCONFIRMED → NEW
Component: Search → Autocomplete
Ever confirmed: true
Product: Firefox → Toolkit
QA Contact: search → autocomplete
Assignee | ||
Comment 3•17 years ago
|
||
Uhoh, what are the right autocomplete APIs? I'm integrating the browser searchbar binding into Songbird at the moment.
Comment 4•17 years ago
|
||
Sorry, I was unclear. It's using the right APIs, but it's not using them properly. I meant "use the multiple search feature", instead of the current "use a single nsIAutoCompleteSearch (the one in nsSearchSuggestions.js) that forwards to another (the one for form history) to get additional results". nsSearchSuggestions.js has all the gory details.
Assignee | ||
Comment 5•17 years ago
|
||
Ah, that makes sense. Thanks. Who should I hassle for a review?
Comment 6•17 years ago
|
||
See http://www.mozilla.org/projects/toolkit/review.html. I'd be glad to review it.
Assignee | ||
Updated•17 years ago
|
Attachment #262167 -
Flags: review?(gavin.sharp)
Comment 7•17 years ago
|
||
Comment on attachment 262167 [details] [diff] [review] Fix + Comments Sorry for the delay in reviewing here. I thought there were other problems with this code when it adds rows to failed searches for a status message, or something like that, but I can't seem to pinpoint it right now, and I assume you guys would have probably run into it if you're using the multiple search feature, so maybe I'm just remembering wrong. This code needs a unit test - could you write one? You should be able to base it on an existing test, like http://lxr.mozilla.org/seamonkey/source/toolkit/components/autocomplete/tests/unit/test_330578.js .
Attachment #262167 -
Flags: review?(gavin.sharp) → review+
Comment 8•17 years ago
|
||
If this is ready for check-in, please mark it as [checkin needed] in the status whiteboard and someone will get it in.
Assignee: nobody → matt
Version: unspecified → Trunk
Assignee | ||
Comment 9•17 years ago
|
||
Thanks for the review. Sure, I don't mind writing a test. Testing the AutoCompleteController requires an AutoCompleteInput and AutoCompletePopup. Should I stub these out and test with xpcshell, or use mochitest and an actual autocomplete textbox element?
Comment 10•17 years ago
|
||
Either works, as long as you can test that the fix doesn't regress. Do whatever you find easiest :)
Assignee | ||
Comment 11•17 years ago
|
||
Attachment #269290 -
Flags: review?(gavin.sharp)
Comment 12•17 years ago
|
||
Comment on attachment 269290 [details] [diff] [review] Fix + Unit Test This is great work, thanks a lot for getting this done Matt! I can land this if you'd like.
Attachment #269290 -
Flags: review?(gavin.sharp) → review+
Updated•17 years ago
|
Whiteboard: [checkin needed]
Assignee | ||
Comment 13•17 years ago
|
||
Thanks, that would be great.
Updated•17 years ago
|
Keywords: checkin-needed
Whiteboard: [checkin needed]
Comment 14•17 years ago
|
||
Checking in toolkit/components/autocomplete/tests/unit/test_378079.js; initial revision: 1.1 Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp; new revision: 1.59; previous revision: 1.58
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta1
You need to log in
before you can comment on or make changes to this bug.
Description
•