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

VERIFIED FIXED in Firefox 3 beta5

Status

()

P1
normal
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: moco, Assigned: Mardak)

Tracking

Trunk
Firefox 3 beta5
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
wanted-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

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.
(Assignee)

Updated

11 years ago
Blocks: 416714
(Assignee)

Updated

11 years ago
Blocks: 405745
No longer depends on: 405745
(Assignee)

Comment 1

11 years ago
Posted 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)
(Assignee)

Updated

11 years ago
Blocks: 415403
(Assignee)

Comment 2

11 years ago
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/
(Assignee)

Updated

11 years ago
Blocks: 411963
(Assignee)

Comment 3

11 years ago
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
(Assignee)

Comment 4

11 years ago
Posted 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)
(Assignee)

Comment 5

11 years ago
Posted 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)
(Assignee)

Comment 6

11 years ago
blocking-firefox3? because bug 415403 is blocking-firefox3+ and needs this fix.
Flags: blocking-firefox3?
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review gavin]
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review gavin] → [has patch][needs review gavin][fixes bug 411963][needed for bug 415403]
(Assignee)

Comment 7

11 years ago
Posted 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
(Assignee)

Comment 9

11 years ago
Posted 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)
(Assignee)

Updated

11 years ago
Blocks: 420437
(Assignee)

Comment 10

11 years ago
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.
(Assignee)

Comment 11

11 years ago
Posted 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)
(Assignee)

Comment 12

11 years ago
Comment on attachment 307892 [details] [diff] [review]
v1.5

Clearing r? until dependencies are ready.
Attachment #307892 - Flags: review?(gavin.sharp) → review?
(Assignee)

Updated

11 years ago
Depends on: 421412
(Assignee)

Updated

11 years ago
Depends on: 421315
(Assignee)

Comment 13

11 years ago
Bumping priority to P2 as this bug blocks P2 blocking bug 415403.
Priority: P3 → P2
(Assignee)

Comment 14

11 years ago
Posted 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?
(Assignee)

Updated

11 years ago
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]
(Assignee)

Updated

11 years ago
Blocks: 418257
(Assignee)

Comment 15

11 years ago
Posted 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)
(Assignee)

Updated

11 years ago
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]
(Assignee)

Updated

11 years ago
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
(Assignee)

Comment 17

11 years ago
Posted 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)
(Assignee)

Comment 18

11 years ago
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]
(Assignee)

Comment 20

11 years ago
Posted 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)
(Assignee)

Comment 21

11 years ago
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+
(Assignee)

Comment 23

11 years ago
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+
(Assignee)

Comment 26

11 years ago
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
Last Resolved: 11 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.