422491 - Optimize AwesomeBar if it finished searching and found fewer than maxResults

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Ed Lee :Mardak]

We can avoid bug 412730 but still do some optimization by only using *finished* results with no possibility of there being more results. This means we haven't hit maxResults and the search finished.

We only need to look through the urls from the previous search while making sure not to kill keyword results and stuff.

Comment 1

[Ed Lee :Mardak]

This patch depends on bug 422490.

Comment 2

[Ed Lee :Mardak]

Attachment: v1

Definitely won't be able to apply this without bug 422490.

Comment 3

[Ed Lee :Mardak]

FYI, this even works nicely with special searches from bug 395161. Adaptive results still do their thing and keyword results keep updating.

Comment 4

[Ed Lee :Mardak]

Attachment: v1.1

Switched to a nsStringArray to keep things sane ;)

Too bad.. no more triple pointer, dereference, postincrement, dereference, assign address magic! :p


>+  // The void * argument is a pointer to a nsString * array. We post-increment
>+  // the contents of the pointer so that it points to the next nsString *
>+  // element for the next enumeration. For the current nsString *, assign it
>+  // the address of the current hashtable entry's string.
>+  *(*(const nsAString ***)aArg)++ = &aKey;

Comment 5

[Ed Lee :Mardak]

Oh, forgot the main reason for saneness. Apparently compiling on windows didn't support C99 ability for variable length stack arrays.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

This will not block the final release of Firefox 3.

Comment 7

[Ed Lee :Mardak]

Bug 422490 was a step to get this patch in, which is another step towards bug 422177 which is a P2 blocker.

I'll unbitrot this patch from moving it to the top of my stack above the special sauce bug 395161 and keyword magic bug 392143.

Comment 8

[Ed Lee :Mardak]

Attachment: v1.2

Moved to the top of my stack and pulled in some changes from bug 395161 because we need to use that query fragment in multiple places.

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 311021 [details] [diff] [review]
v1.2

r=me

Comment 10

[Ed Lee :Mardak]

Comment on attachment 311021 [details] [diff] [review]
v1.2

Making things go faster and needed for bug 422177 which makes things go really fast.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 311021 [details] [diff] [review]
v1.2

a=mconnor on behalf of 1.9 drivers

Comment 12

[Ed Lee :Mardak]

To see this working, you need to type in some search terms that result in ~5 results. Have the search throbber finish spinning, then type a letter that matches in fewer than 5 of those results. It should show the results right away instead of showing nothing and an activity throbber.

Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.151; previous revision: 1.150
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.50; previous revision: 1.49
done

Comment 13

[Ed Lee :Mardak]

Attachment: leak bustage fix

Apparently bad stuff happens with a global const nsCString reference. Converted it to a #define.

Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.52; previous revision: 1.51
done

We were leaking 8 bytes..
Error: Leak Test Failed: Number of leaks 8 is greater than LeakFailureThreshold 0
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1206254100.1206254570.32461.gz

Comment 14

[Ed Lee :Mardak]

FYI, the leak was reported here..

|<----------------Class--------------->
                        nsStringBuffer

|<-----Bytes------>
 Per-Inst   Leaked
        8        8

|<----------------Objects---------------->
    Total      Rem      Mean       StdDev
    32460        1 ( 4330.68 +/-  2367.57)

|<--------------References-------------->|
     Total      Rem      Mean       StdDev
    59418        1 ( 4792.61 +/-  2814.15)

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is pretty obfuscated code here, especially the #define above the former const ref.  It's also I think doing scary thing with temporaries for the strings, and could break depending on how the final #define is being used.

This really just wants to be a nsPrintfCString for the entire thing in some Init function.. maybe with multiple nsPrintfCStrings for the fragments that are now being done via string pasting in the #define.

Comment 16

[Tracy Walker [:tracy]]

added this expectation to test case https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=8955