emphasize all matching text in title and url, not just the first match in title and url emphasizing the first in title and url is bug #406255 To implement this efficiently, we might need bug #407944 Alternatively, we could create additional <label>s (or actually <html:span>s, for some RTL issues) for each match (so we could emphasize it), but I think this would slow things down.

Dynamically generate multiple spans and #text instead of a single hardcoded span. The current patch will also fix bug 416714.

Here's a build that has the patch for this bug and bug 415403, so all terms will get matched in the autocomplete. https://build.mozilla.org/tryserver-builds/2008-02-10_10:27-edward.lee@engineering.uiuc.edu-showmatch.adaptive/

This patch is needed for blocking bug 415403, so carrying over the P2 priority. This patch does *not* need bug 407944 to land. I've been using this for over a week and things are pretty responsive. gavin: Would it be better if I did a substr(url/title, 1000) before searching for matches assuming we won't be able to show those matches anyway?

Oops. A little bug that I just noticed while dogfooding. ;)

Added a pref to control how many characters to check when emphasizing query matches. This is useful when you happen to match really long URLs.. e.g., bugzilla queries.

blocking-firefox3? because bug 415403 is blocking-firefox3+ and needs this fix.

let instead of var :) Fixed up some variable scope and comments.

Low blocker, mostly an annoyance and polish, but we've got a patch in hand.

Use a helper function to make tokens to help simplify things further down the line such as bug 420437.

I would feel better if this landed after bug 421315 for perf wins and somewhat bug 421412 to save a little bit on cvs blame.. the latter fixes P2 blocking bug 396548.

This patch gets simplified with bug 421412. This patch is also on top of bug 421315 which is on top of bug 419656.

Comment on attachment 307892 [details] [diff] [review] v1.5 Clearing r? until dependencies are ready.

Bumping priority to P2 as this bug blocks P2 blocking bug 415403.

Unbitrot changes from bug 421412 and r? to put in the queue even though this comes after bug 419656 and bug 421315.

Same as v1.6 except as a CVS diff now that bug 421315 landed.

This is now P1 since it's required for bug 407861 which is a P1.

Unbitrot context from changes to bug 393678 and save a few characters (and get a little bit of correctness/generality) when checking for empty search.

Er. Interdiff not working. - if (aSearches[0] == "") + if (aSearches == "")

Comment on attachment 310718 [details] [diff] [review] v1.8 >Index: toolkit/content/widgets/autocomplete.xml >+ <field name="_boundaryCutoff">null</field> >+ >+ <property name="boundaryCutoff" readonly="true"> >+ <getter> >+ <![CDATA[ >+ if (!this._boundaryCutoff) { >+ try { >+ this._boundaryCutoff = >+ Components.classes["@mozilla.org/preferences-service;1"]. >+ getService(Components.interfaces.nsIPrefBranch). >+ getIntPref("browser.urlbar.richQueryEmphasisCutoff"); >+ } catch (ex) { >+ this._boundaryCutoff = 200; >+ } >+ } >+ return this._boundaryCutoff; >+ ]]> >+ </getter> >+ </property> Shouldn't have this toolkit file depend on a browser/ pref. Eeasiest would probably be to just make it a toolkit pref (toolkit.autocomplete.richBoundaryCutoff? there's no precedent here) and put it in all.js. That way you can lose the try/catch and just use a field without a property (fields are lazily initialized, and only initialized once). >+ <method name="_getBoundaryIndices"> > <parameter name="aText"/> >+ <parameter name="aSearches"/> >+ // Short circuit for empty search >+ if (aSearches == "") >+ return [0, aText.length]; This confused me, because aSearches is an array, and I didn't realize that ([""] == "") is true (because the array is coerced to a string instead of the opposite). Wouldn't hurt to add a comment (just |[""] == ""| would do). Maybe also name the parameter "aSearchTokens" to match getSearchTokens? The rest looks fine, though I'm still concerned about the perf impact of adding so many extra child nodes. It seems like this loses some of the "reuse items" optimizations that avoid doing most of the work if the needle/hay haven't changed (e.g. when we're invalidating the initial items after receiving a second chunk). Can we cache the text (aText) and search term as JS properties on the description element, and avoid doing most of the work in _setUpDescription if they haven't changed? I'll r+ with these addressed.

(In reply to comment #19) > >+ <field name="_boundaryCutoff">null</field> > >+ <property name="boundaryCutoff" readonly="true"> I tried just using a field like "<field>dump("field\n"); 200</field>" and it keeps dumping "field". So I'll stay with the field/property lazy init combo. > >+ } catch (ex) { > >+ this._boundaryCutoff = 200; > Shouldn't have this toolkit file depend on a browser/ pref. Eeasiest would > probably be to just make it a toolkit pref > (toolkit.autocomplete.richBoundaryCutoff? there's no precedent here) and put it > in all.js. That way you can lose the try/catch Switched to "toolkit.autocomplete.richBoundaryCutoff" in modules/libpref/src/init/all.js. Got rid of the try/catch. > >+ // Short circuit for empty search > >+ if (aSearches == "") > >+ return [0, aText.length]; > This confused me, because aSearches is an array Commented "Short circuit for empty search ([""] == "")". >Maybe also name the parameter "aSearchTokens" to match getSearchTokens? Done. > Can we cache the text (aText) and search term as JS properties > on the description element, and avoid doing most of the work in > _setUpDescription if they haven't changed? With the fix for bug 421315, we'll never be setting up description for an item if the url and search don't change. So if we do end up calling setUpDescription, it's because there's something new to highlight.

stephend: "Type 't' in the location bar and see that both 't's of hTTp are emphasized."

Comment on attachment 310825 [details] [diff] [review] v1.9 (In reply to comment #20) > > Can we cache the text (aText) and search term as JS properties > > on the description element, and avoid doing most of the work in > > _setUpDescription if they haven't changed? > With the fix for bug 421315, we'll never be setting up description for an item > if the url and search don't change. So if we do end up calling > setUpDescription, it's because there's something new to highlight. Ah, right. OK!

Comment on attachment 310825 [details] [diff] [review] v1.9 a1.9b5? for P1 bug that blocks P1 bug 407861. A litmus test is described in comment #21.

Comment on attachment 310825 [details] [diff] [review] v1.9 a=beltzner Ed, can you either add tests or flip the in-litmus? flag and come up with a quick human test for this?

(In reply to comment #23) > (From update of attachment 310825 [details] [diff] [review]) > a1.9b5? for P1 bug that blocks P1 bug 407861. > > A litmus test is described in comment #21. in-litmus+ https://litmus.mozilla.org/show_test.cgi?id=5209

Checking in modules/libpref/src/init/all.js; /cvsroot/mozilla/modules/libpref/src/init/all.js,v <-- all.js new revision: 3.742; previous revision: 3.741 done Checking in toolkit/content/widgets/autocomplete.xml; /cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v <-- autocomplete.xml new revision: 1.125; previous revision: 1.124 done

Verified FIXED using: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008032204 Minefield/3.0b5pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008032204 Minefield/3.0b5pre -and- Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008032204 Minefield/3.0b5pre (ZOMG, awesomebar really is teh bomb :-)