Closed Bug 407946 Opened 17 years ago Closed 16 years ago

emphasize all matching text in title and url, not just the first match in title and url

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta5

People

(Reporter: moco, Assigned: Mardak)

References

Details

Attachments

(1 file, 9 obsolete files)

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.
Blocks: 416714
Blocks: 405745
No longer depends on: 405745
Attached patch v1 (obsolete) — Splinter Review
Dynamically generate multiple spans and #text instead of a single hardcoded span. The current patch will also fix bug 416714.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #302447 - Flags: review?(gavin.sharp)
Blocks: 415403
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/
Blocks: 411963
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?
Priority: -- → P2
Attached patch v1.1 (obsolete) — Splinter Review
Oops. A little bug that I just noticed while dogfooding. ;)
Attachment #302447 - Attachment is obsolete: true
Attachment #305086 - Flags: review?(gavin.sharp)
Attachment #302447 - Flags: review?(gavin.sharp)
Attached patch v1.2 (obsolete) — Splinter Review
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.
Attachment #305086 - Attachment is obsolete: true
Attachment #305396 - Flags: review?(gavin.sharp)
Attachment #305086 - Flags: review?(gavin.sharp)
blocking-firefox3? because bug 415403 is blocking-firefox3+ and needs this fix.
Flags: blocking-firefox3?
Whiteboard: [has patch][needs review gavin]
Whiteboard: [has patch][needs review gavin] → [has patch][needs review gavin][fixes bug 411963][needed for bug 415403]
Attached patch v1.3 (obsolete) — Splinter Review
let instead of var :) Fixed up some variable scope and comments.
Attachment #305396 - Attachment is obsolete: true
Attachment #305675 - Flags: review?(gavin.sharp)
Attachment #305396 - Flags: review?(gavin.sharp)
Low blocker, mostly an annoyance and polish, but we've got a patch in hand.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3+
Priority: P2 → P3
Attached patch v1.4 (obsolete) — Splinter Review
Use a helper function to make tokens to help simplify things further down the line such as bug 420437.
Attachment #305675 - Attachment is obsolete: true
Attachment #306699 - Flags: review?(gavin.sharp)
Attachment #305675 - Flags: review?(gavin.sharp)
Blocks: 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.
Attached patch v1.5 (obsolete) — Splinter Review
This patch gets simplified with bug 421412. This patch is also on top of bug 421315 which is on top of bug 419656.
Attachment #306699 - Attachment is obsolete: true
Attachment #307892 - Flags: review?(gavin.sharp)
Attachment #306699 - Flags: review?(gavin.sharp)
Comment on attachment 307892 [details] [diff] [review]
v1.5

Clearing r? until dependencies are ready.
Attachment #307892 - Flags: review?(gavin.sharp) → review?
Depends on: 421412
Depends on: 421315
Bumping priority to P2 as this bug blocks P2 blocking bug 415403.
Priority: P3 → P2
Attached patch v1.6 (obsolete) — Splinter Review
Unbitrot changes from bug 421412 and r? to put in the queue even though this comes after bug 419656 and bug 421315.
Attachment #307892 - Attachment is obsolete: true
Attachment #308377 - Flags: review?(gavin.sharp)
Attachment #307892 - Flags: review?
Whiteboard: [has patch][needs review gavin][fixes bug 411963][needed for bug 415403] → [has patch][need review gavin][need bug 421315][fixes bug 411963][needed for bug 415403]
Blocks: 418257
Attached patch v1.7 (obsolete) — Splinter Review
Same as v1.6 except as a CVS diff now that bug 421315 landed.
Attachment #308377 - Attachment is obsolete: true
Attachment #309038 - Flags: review?(gavin.sharp)
Attachment #308377 - Flags: review?(gavin.sharp)
Whiteboard: [has patch][need review gavin][need bug 421315][fixes bug 411963][needed for bug 415403] → [has patch][need review gavin][needed for bug 415403, bug 418257]
Blocks: 407861
This is now P1 since it's required for bug 407861 which is a P1.
Priority: P2 → P1
Target Milestone: --- → Firefox 3 beta5
Attached patch v1.8 (obsolete) — Splinter Review
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.
Attachment #309038 - Attachment is obsolete: true
Attachment #310718 - Flags: review?(gavin.sharp)
Attachment #309038 - Flags: review?(gavin.sharp)
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.
Whiteboard: [has patch][need review gavin][needed for bug 415403, bug 418257] → [has patch][need new patch][needed for bug 415403, bug 418257]
Attached patch v1.9Splinter Review
(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.
Attachment #310718 - Attachment is obsolete: true
Attachment #310825 - Flags: review?(gavin.sharp)
Attachment #310718 - Flags: review?(gavin.sharp)
stephend: "Type 't' in the location bar and see that both 't's of hTTp are emphasized."
Flags: in-litmus?
Whiteboard: [has patch][need new patch][needed for bug 415403, bug 418257] → [has patch][needed for bug 415403, bug 418257, bug 407861]
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!
Attachment #310825 - Flags: review?(gavin.sharp) → review+
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.
Attachment #310825 - Flags: approval1.9b5?
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?
Attachment #310825 - Flags: approval1.9b5? → approval1.9b5+
(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
Flags: in-litmus? → in-litmus+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needed for bug 415403, bug 418257, bug 407861]
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 :-)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: