401660 - when showing autocomplete result, show tags that partially match

Status: VERIFIED FIXED

(Firefox :: Address Bar, enhancement, P2)

Description

[(not reading, please use [e-mail] instead)]

when showing autocomplete result, show tags that partially match

I was telling dan mills that for autocomplete results, if you have an exact match with a tag, we'll show you the tagged items (at the top of your autocomplete results)

here's an idea for an enhancement:

when typing "fire", if there is no "fire" tag, show an item corresponding to any tags that might match this text as the topmost result, like:  "firefly firefox fireman".

if you have no tags, you won't see anything.

but if you use tags, this might prove a useful way to find tagged items (in case you don't remember what your tags are called.)

Comment 1

[(not reading, please use [e-mail] instead)]

one concern I have about this is if you are a heavy tag user, because tags currently come first in autocomplete, short autocomplete queries (example, you type "g") will match lots of tags and you won't see the page you're looking for.

Comment 2

[Dan Mills [:thunder]]

Yeah, that is true.  How about only showing the first N tags that match, sorted by ... something? most bookmarks in each tag? (where N is something small, like 3)

That would get you your top tags with that letter.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

When you select a matching tag and hit enter, what is our plan?  it would be great to generate a results page in the content area with thumbnails, but since that is likely really out of scope, how about we load the list of results in the sidebar.

Comment 4

[Dan Mills [:thunder]]

I was thinking that we would make it as if the user had typed in the full tag name in the url bar (so, we'd show the top bookmark hits for that tag).  Isn't that the easiest solution?

Though showing more content in the sidebar or content area would sure be nice.

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

In the past mconnor has been against allowing users to enter tags directly into the location bar, but I'm all for it, and I'm pretty sure beltzner is as well.  Cc'ing mconnor so he can comment on the idea.

Comment 6

[(not reading, please use [e-mail] instead)]

> When you select a matching tag and hit enter, what is our plan? 

we don't have a plan yet.  Note, what we are doing today is, if you have a case insensitive but exact match, we show the urls that are tagged.

I guess your question is about if we start showing actually tags as results, though.

> In the past mconnor has been against allowing users to enter tags directly into
> the location bar.

As stated above, we already allow this.  can you (or mconnor) clarify on what he has been against?

One problem (if we suppoert partial matches) with our current autocomplete result is that while you see tagged items (and you know they are tagged matched, because of the tag icon on the right), you won't know why you're matching.

if we go to a richer result style (as in faaborg's mockups, usually involving multiple lines), we could do something better.

I'm still worried about partial tag matches flooding the results.  I sort of like not seeing them unless I have an exact match.  But maybe that's because I'm not a serious tagger.

Open to ideas / opinions / comments / etc.

Comment 7

[Louise]

How about just showing partial tags when you type something like "tag:foo" for tag "foobar"?
That way, you wouldn't get too many autocomplete results when you just type partial tag and I would like to be able to choose my autocomplete between normal URL,title match and tag match.  
I think it is reasonable to show result of exact tag name match in normal autocomplete like current trunk does since in that case, you have explicitly chosen to look for URL with that tag.

Comment 10

[[:Aleksej]]

Comment #1 describes a serious problem. Is there a separate bug on it?

I have several pages tagged with “mozilla”.

Now, I have just starred http://www.mozilla.org/about/mozilla-manifesto.html
Its title is “The Mozilla Manifesto”.

I didn’t tag it, since it contains “mozilla” and “manifesto” in it already.

Now if I type anything that matches the title or the URL, but contains the full word “mozilla” in it, all the tagged links are at the top, and the Manifesto one is at the bottom. The only ways to get it at the top are to type only a part of “mozilla”, anything BUT “mozilla”, or “mozilla-”.

Comment 11

[George K.]

In reply to comment #1 and comment #2 ...

Perhaps the solution to both of these remarks is to maintain the current search behavior (find bookmarked pages by tag, not find tags themselves) and include bookmarked items with partial tag matches in the results, but not give them any added weight.  This would place these partial-tag-match items in the awesome bar results, but not necessarily at or even near the top unless the normal sort order (frequency, adaptive learning as in bug 395739, word boundaries as requested in bug 393678) would place them there.

I may be off base, but it seems like this would handle Seth's concern without needing a whole new search-of-tags paradigm.

There is the case to consider where a tag-happy user has a short string typed in, lets say it's "mo", and suddenly sees bookmarked pages tagged "mozilla", "movies", "motown", "motorcycle", "mom", "montana", etc. in the results.  The question to ask would be "does partial tag matching increase usability or noise?"

My guess is that it would indeed help users, assuming sorting methods ensure quality results, since this wouldn't be far removed from current case where a user has hundreds of history or bookmark pages that match "mo".  Worst case, the tag-loving friend could type a third letter.

Comment 12

[Ed Lee :Mardak]

Attachment: v1

This allows partial matches but also changes the behavior of multiple tag searches to be more consistent with how we search for multiple terms in the url/title.

Before "tag1 tag2" would search for bookmarks that matched tag1 OR tag2. With this patch, it'll match only those that match tag1 AND tag2. But this also means we can search for "tag1 mozilla.com" to match urls that are mozilla.com and are tagged as tag1.

So you can now search for "title url tag" to match against in the title, url, or tags.

Comment 13

[Ed Lee :Mardak]

Attachment: screenshot of v1

Oh, and because we allow partial matches of tags, tags aren't forced to be the first results (similar to how we don't force starred pages to the top). The tagged results are mixed in with the rest of the usual autocomplete results.

I've also made the title show all the tags of a page, so the multiple word highlighting will show it. (Bug 415403)

Comment 14

[Ed Lee :Mardak]

Comment on attachment 304044 [details] [diff] [review]
v1

This provides out-of-order functionality for blocking bug 405320 and showing bookmark title for wanted bug 416211.

Comment 15

[Ed Lee :Mardak]

Attachment: v1.1

Updated to pass the test in bug 416211 and added some comments.

r? to help fix blocking bug 405320

Comment 16

[Ed Lee :Mardak]

Attachment: v1.2

Always show the bookmark title if we matched its tag to correctly fix bug 416211 instead of only showing the bookmark title if we matched in the bookmark title.

Comment 17

[Ed Lee :Mardak]

Whoops. I've been dogfooding this patch and some other stuff and noticed things slowing down a ton. I put on some timing numbers and "p" went from 4ms to 800-900ms. Similarly "bug" went from 10ms to 900-1000ms.

I tried changing the query to this.. but it only dropped the time to 200ms.. which is much better than 1000ms, but still quite a bit worse than 10ms.

SELECT `h.url`, `h.title`, `f.url`, `b.id`, `b.parent`, MIN(`b.title`), GROUP_CONCAT(`t.title`, ' ')
FROM (
  SELECT h.id, h.frecency, h.url, h.title, f.url, b.id, b.parent, b.title, t.title
  FROM moz_places h
  LEFT OUTER JOIN moz_bookmarks b ON b.fk = h.id AND b.type = ?3
  LEFT OUTER JOIN moz_bookmarks t ON t.id = b.parent AND t.parent = ?4
  LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id
  WHERE h.frecency <> 0
  ORDER BY h.frecency DESC
  LIMIT ?1 OFFSET ?2)
GROUP BY `h.id`
ORDER BY `h.frecency` DESC

This is on a debug build with chunkSize 1000. Chunk size of 500 is 100ms, 250 is 50ms. My optimization in this transformation is to quickly get "chunk size" number of results then GROUP BY those returned results instead of GROUP BY the whole table. But it seems like GROUP BY on a column of a temporary table is slow.

Comment 18

[Ed Lee :Mardak]

Attachment: v2

Yay. :) Got the perf back down to 10-15ms for "bug".

SELECT h.url, h.title, f.url, `b.id`, `b.parent`, `b.title`, `b.tags` 
FROM moz_places h 
LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id 
LEFT OUTER JOIN (
  SELECT b.fk, b.id, b.parent, MIN(b.title) `b.title`, 
    GROUP_CONCAT(t.title, ' ') `b.tags` 
  FROM moz_bookmarks b 
  LEFT OUTER JOIN moz_bookmarks t ON t.id = b.parent AND t.parent = ?4 
  WHERE b.type = ?3 
  GROUP BY b.fk) ON `b.fk` = h.id 
WHERE h.frecency <> 0 
ORDER BY h.frecency DESC LIMIT ?1 OFFSET ?2

I wonder if there's a better way of "passing in" h.id into the subquery JOIN because right now I have to expose b.fk and then check if b.fk equals h.id for the JOIN.

Comment 19

[Marco Bonardo [:mak]]

i don't have a good db to test perf on, but i'm trying to come with a better query so you should be able to test it on yours. actually if you use a join you have to specify what to join on, so you HAVE to expose the join-to column.
What you're really doing is selecting a new field the group_concat, so we could probably rewrite the query avoiding the join on a memory created table. My only concern is perf since i can't test queries... if tomorrow you could send me a valid db to test on i should come with better results, however will come with a temp query to see if we can gain perf without double selecting fields

Comment 20

[Marco Bonardo [:mak]]

try this, it is using one less memory table, but still i don't have a db with many tags. (I've replaced bindings with actual values to test, so you should revert them)

SELECT h.url, h.title, f.url, b.id, b.parent, MIN(b.title),
( 
  SELECT GROUP_CONCAT(t.title, ' ')
  FROM moz_bookmarks t
  WHERE t.id = b.parent AND t.parent = ?4
  GROUP BY t.fk 
)
FROM moz_places h
LEFT OUTER JOIN moz_bookmarks b on b.fk = h.id
LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id
WHERE (b.id IS NULL OR b.type = ?3) AND h.frecency <> 0
GROUP BY h.id
ORDER BY h.frecency DESC LIMIT ?1 OFFSET ?2

Comment 21

[Ed Lee :Mardak]

The problem with slowdown is that we do a GROUP BY, so it'll need to fetch all the rows. That's why I did a subselect earlier LIMITing first then grouping which saved 1000 -> 200ms. My new query is just 15ms because it makes a memory table for just bookmark tags.

I'll try your query and report back in a bit.

Comment 22

[Marco Bonardo [:mak]]

i'm not sure that the actual query i attached will correctly catch all tags, due to b.parent be not grouped :( So i should revise it

yes i see the point about avoiding grouping since our urls in places are guaranteed to be unique

Comment 23

[Marco Bonardo [:mak]]

correct results:

SELECT h.url, h.title, f.url, b.id, b.parent, MIN(b.title),
(
  SELECT GROUP_CONCAT(t1.title, ' ')
  FROM moz_bookmarks t1
  JOIN moz_bookmarks t2 on t2.parent = t1.id
  WHERE t1.parent = 4 AND t2.fk = h.id
  GROUP BY t2.fk
)
FROM moz_places h
LEFT OUTER JOIN moz_bookmarks b on b.fk = h.id
LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id
WHERE (b.id IS NULL OR b.type = 1) AND h.frecency <> 0
GROUP BY h.id
ORDER BY h.frecency DESC LIMIT 500 OFFSET 0

we cannot reduce the number of tables :( On my big db this is faster than yours, but that could be db dependant. Do you need to select b.parent at all? how do you choose between different parents? i suggest dropping b.parent from select if you don't use it, or put some rule to choose a parent if you use it

Comment 24

[Marco Bonardo [:mak]]

this is not beautiful to read out, but avoid grouping (and does not have b.parent). in my db i have 18000 moz_places, about 10 tags, this query is quite fast. I cannot understand why your query on my db is really slow (about 800ms, while this is 30ms and my previous one was 400ms :( )

SELECT h.url, h.title, f.url,
(SELECT id FROM moz_bookmarks WHERE fk = h.id AND type = 1),
(SELECT title FROM moz_bookmarks WHERE fk = h.id AND title NOT NULL ORDER BY title ASC LIMIT 1),
(
  SELECT GROUP_CONCAT(t1.title, ' ')
  FROM moz_bookmarks t1
  JOIN moz_bookmarks t2 on t2.parent = t1.id
  WHERE t1.parent = 4 AND t2.fk = h.id
  GROUP BY t2.fk
)
FROM moz_places h
LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id
WHERE h.frecency <> 0
ORDER BY h.frecency DESC LIMIT 500 OFFSET 0

Comment 25

[Marco Bonardo [:mak]]

finally, on my db this is the fastest between all queries in this report (sorry for spam)

SELECT h.url, h.title, f.url,
(SELECT id FROM moz_bookmarks WHERE fk = h.id AND type = 1 LIMIT 1),
(SELECT MIN(title) FROM moz_bookmarks WHERE fk = h.id),
(
  SELECT GROUP_CONCAT(t1.title, ' ')
  FROM moz_bookmarks t1
  JOIN moz_bookmarks t2 on t2.parent = t1.id
  WHERE t1.parent = 4 AND t2.fk = h.id
  GROUP BY t2.fk
)
FROM moz_places h
LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id
WHERE h.frecency <> 0
ORDER BY h.frecency DESC LIMIT 1000 OFFSET 0

Comment 26

[Ed Lee :Mardak]

We need the parent id to decide if we show a bookmark icon or a favicon icon.

Comment 27

[Marco Bonardo [:mak]]

But you may have many parents for the same url, doing a grouping you will end up with a random one if you don't use a grouping function.
The same applies to b.id so, since you probably use b.id only to see if an item is bookmarked, since a bookmarked items should always have a parent, you can only select parent and use that for both. That is if b.id is not directly used.

Comment 28

[Ed Lee :Mardak]

Hrm that's interesting. We don't really need b.id because we only use that to check if it's non-null. So we can get away with only grabbing b.parent and b.title. So we can prefer tags over bookmarks.

SELECT h.url, h.title, f.url,
  (SELECT MIN(b.parent)
   FROM moz_bookmarks b
   JOIN moz_bookmarks t ON t.id = b.parent AND t.parent != 4
   WHERE b.type = 1 AND b.fk = h.id),
  (SELECT MIN(b.title)
   FROM moz_bookmarks b
   JOIN moz_bookmarks t ON t.id = b.parent AND t.parent != 4
   WHERE b.type = 1 AND b.fk = h.id),
  (SELECT GROUP_CONCAT(t.title, ' ')
   FROM moz_bookmarks b
   JOIN moz_bookmarks t ON t.id = b.parent AND t.parent = 4
   WHERE b.type = 1 AND b.fk = h.id)
FROM moz_places h
LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id
WHERE h.frecency <> 0
ORDER BY h.frecency DESC LIMIT 1000 OFFSET 0

Two queries that get bookmark parent and bookmark title and one query for tag titles.

That query is slightly faster than what I have in the patch right now, and it's slightly cleaner so I'll update the patch.

Comment 29

[Ed Lee :Mardak]

Attachment: v2.1

Updated the query to be just slightly faster and cleaned up the code. It fixes some testcases and adds more tests cases.

Comment 30

[Ed Lee :Mardak]

Attachment: v2.2

Split off some tests to their own bugs. When this lands, I'll be landing 5 separate testcases:

Bug 401660 - when showing autocomplete result, show tags that partially match
Bug 405320 - support "out of order" multiple word tag search in the url bar and in the places organizer
Bug 416211 - Tagged results don't show bookmark title when matching the tag
Bug 418920 - Allow multiple word search with tags in addition to title and url
Bug 419068 - Allow tagged pages to mingle with non-tagged autocomplete results

Comment 31

[Ed Lee :Mardak]

Attachment: v2.3

Reworked the query to let the tag folder always be ?1 and bookmark type always be ?2. This will make it more useful for other queries that also need to fetch the same columns.

Comment 32

[Ed Lee :Mardak]

Requesting blocking-firefox3 as it fixes bug 405320 which is blocking+. Additionally, partial matches is useful because the user can have a descriptive tag and adding a tag to a page just requires selecting the checkbox; however, finding it by typing out a long word or series of words would be not fun.

Comment 33

[Dietrich Ayala (:dietrich)]

Comment on attachment 305083 [details] [diff] [review]
v2.3


>diff --git a/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp b/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>--- a/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>@@ -104,15 +104,28 @@ nsNavHistory::InitAutoComplete()
> //    a separate function so it can be re-created when the option changes.
> 
> nsresult
> nsNavHistory::CreateAutoCompleteQueries()
> {
>+  // Helper to get a particular column from the bookmark/tags table based on if
>+  // we want to include tags or not
>+#define SQL_STR_FRAGMENT_GET_BOOK_TAG(column, comparison) \
>+  "(SELECT " column " " \
>+  "FROM moz_bookmarks b " \
>+  "JOIN moz_bookmarks t ON t.id = b.parent AND t.parent " comparison " ?1 " \
>+  "WHERE b.type = ?2 AND b.fk = h.id), "

b.type is always bookmark, yeah? if so, could use nsINavBookmarksService::TYPE_BOOKMARK here instead of binding later.

>+
>+  // This gets three columns from the bookmarks and tags table followed by ", "
>+  const nsCString &bookTag = NS_LITERAL_CSTRING(
>+    SQL_STR_FRAGMENT_GET_BOOK_TAG("MIN(b.parent)", "!=")
>+    SQL_STR_FRAGMENT_GET_BOOK_TAG("MIN(b.title)", "!=")

why MIN()?

preferably we'd want the most recently added or modified bookmark for both queries.

>+    SQL_STR_FRAGMENT_GET_BOOK_TAG("GROUP_CONCAT(t.title, ' ')", "="));
>+
>   nsCString sql = NS_LITERAL_CSTRING(
>-    "SELECT h.url, h.title, f.url, b.id, b.parent, b.title "
>+    "SELECT h.url, h.title, f.url, ") + bookTag + NS_LITERAL_CSTRING("NULL "

nit: please make multiple lines for readability

>@@ -171,15 +184,10 @@ nsNavHistory::PerformAutoComplete()
>   mCurrentResult->SetSearchString(mCurrentSearchString);
> 
>   nsresult rv;
>   // Only do some extra searches on the first chunk
>   if (!mCurrentChunkOffset) {
>-    // No need to search for tags if there's nothing to search
>-    if (!mCurrentSearchString.IsEmpty()) {
>-      rv = AutoCompleteTagsSearch();
>-      NS_ENSURE_SUCCESS(rv, rv);
>-    }
>   }

remove empty block?

>-
>-      PRInt64 parentId = 0;
>-      // Only bother to fetch parent id and bookmark title if we have a bookmark
>-      if (itemId) {
>-        rv = aQuery->GetInt64(kAutoCompleteIndex_ParentId, &parentId);
>-        NS_ENSURE_SUCCESS(rv, rv);
>+      
>+      // Only fetch the bookmark title if we have a bookmark
>+      if (parentId) {
>         rv = aQuery->GetString(kAutoCompleteIndex_BookmarkTitle, entryBookmarkTitle);
>         NS_ENSURE_SUCCESS(rv, rv);
>       }
> 
>+      nsAutoString entryTags;
>+      rv = aQuery->GetString(kAutoCompleteIndex_Tags, entryTags);
>+      NS_ENSURE_SUCCESS(rv, rv);

if it's not a bookmark, it won't have any tags, so could put this in that block as well

>+
>+      // Add the tags to the title if necessary
>+      if (showTags) {
>+        // Always show the bookmark if possible when we have tags
>+        useBookmark = !entryBookmarkTitle.IsEmpty();
>+        (useBookmark ? entryBookmarkTitle : entryTitle)
>+          += NS_LITERAL_STRING(" (") + entryTags + NS_LITERAL_STRING(")");
>+      }

this does not seem RTL-friendly. please put this change on bug 418257 instead of here, for independent UX analysis.

Comment 34

[Ed Lee :Mardak]

(In reply to comment #33)
> >+  // Helper to get a particular column from the bookmark/tags table based on if
> >+  // we want to include tags or not
> >+#define SQL_STR_FRAGMENT_GET_BOOK_TAG(column, comparison) \
> >+  "(SELECT " column " " \
> >+  "FROM moz_bookmarks b " \
> >+  "JOIN moz_bookmarks t ON t.id = b.parent AND t.parent " comparison " ?1 " \
> >+  "WHERE b.type = ?2 AND b.fk = h.id), "
> b.type is always bookmark, yeah? if so, could use
> nsINavBookmarksService::TYPE_BOOKMARK here instead of binding later.
I tried that before and had issues with the #define and nsPrintfCString and concatenating with more NS_LITERAL_CSTRINGs.

> >+  const nsCString &bookTag = NS_LITERAL_CSTRING(
> >+    SQL_STR_FRAGMENT_GET_BOOK_TAG("MIN(b.parent)", "!=")
> >+    SQL_STR_FRAGMENT_GET_BOOK_TAG("MIN(b.title)", "!=")
> why MIN()?
> preferably we'd want the most recently added or modified bookmark for both
> queries.
This is hackish, but hopefully there aren't multiple bookmarks often. I figured the user is more likely to bookmark a page and then tag it. MIN on parent is somewhat arbitrary.. just making sure to choose one instead of none. Tagging appears to make a dummy bookmark with likely-to-be-lowercase title, so MIN on the title prefers titles that start with upper-case.

> >   if (!mCurrentChunkOffset) {
> >-    // No need to search for tags if there's nothing to search
> >-    if (!mCurrentSearchString.IsEmpty()) {
> >-      rv = AutoCompleteTagsSearch();
> >-      NS_ENSURE_SUCCESS(rv, rv);
> >-    }
> >   }
> remove empty block?
It'll would be readded for bug 395739, so I preferred keeping the original blame instead of removing and readding.

> >+      nsAutoString entryTags;
> >+      rv = aQuery->GetString(kAutoCompleteIndex_Tags, entryTags);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> if it's not a bookmark, it won't have any tags, so could put this in that block
> as well
I tried that before, but it failed the tagging testcases. That's because I ignore bookmarks made specifically for tags. (Note the != given to the #define.)

> >+      // Add the tags to the title if necessary
> >+      if (showTags) {
> >+        // Always show the bookmark if possible when we have tags
> >+        useBookmark = !entryBookmarkTitle.IsEmpty();
> >+        (useBookmark ? entryBookmarkTitle : entryTitle)
> >+          += NS_LITERAL_STRING(" (") + entryTags + NS_LITERAL_STRING(")");
> >+      }
> this does not seem RTL-friendly. please put this change on bug 418257 instead
> of here, for independent UX analysis.
Sure. But I would assume the RTL would take the string and reverse it for display? We're appending to the end of the string which will still be at the "end" for RTL.

Comment 35

[Ed Lee :Mardak]

Attachment: v2.4

(In reply to comment #33)
> b.type is always bookmark, yeah? if so, could use
> nsINavBookmarksService::TYPE_BOOKMARK here instead of binding later.
Figured out what I was doing wrong earlier. I started NS_LITERAL_CSTRING outside of the macro and tried to end it and restart another one from within the #define.

> >+    SQL_STR_FRAGMENT_GET_BOOK_TAG("MIN(b.parent)", "!=")
> >+    SQL_STR_FRAGMENT_GET_BOOK_TAG("MIN(b.title)", "!=")
> preferably we'd want the most recently added or modified bookmark for both
> queries.
Converted to consistently take the most recently modified bookmark.

> >+    "SELECT h.url, h.title, f.url, ") + bookTag + NS_LITERAL_CSTRING("NULL "
> nit: please make multiple lines for readability
Split on two lines.

> >   if (!mCurrentChunkOffset) {
> >-    // No need to search for tags if there's nothing to search
> >-    if (!mCurrentSearchString.IsEmpty()) {
> >-      rv = AutoCompleteTagsSearch();
> >-      NS_ENSURE_SUCCESS(rv, rv);
> >-    }
> >   }
> remove empty block?
Leaving as is for now.

> >+      nsAutoString entryTags;
> >+      rv = aQuery->GetString(kAutoCompleteIndex_Tags, entryTags);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> if it's not a bookmark, it won't have any tags, so could put this in that block
> as well
Leaving as is for now.

> >+        (useBookmark ? entryBookmarkTitle : entryTitle)
> >+          += NS_LITERAL_STRING(" (") + entryTags + NS_LITERAL_STRING(")");
> this does not seem RTL-friendly. please put this change on bug 418257 instead
> of here, for independent UX analysis.
Commented out and added XXX bug comment.

Comment 36

[Ed Lee :Mardak]

Attachment: v2.3 -> v2.4 interdiff

Comment 37

[Dietrich Ayala (:dietrich)]

Comment on attachment 305576 [details] [diff] [review]
v2.4

looks good, thanks for fixing the comments. r=me.

Comment 38

[Ed Lee :Mardak]

Comment on attachment 305576 [details] [diff] [review]
v2.4

a1.9? This fixes several bugs including a blocker.

Comment 39

[Damon Sicore (:damons)]

Comment on attachment 305576 [details] [diff] [review]
v2.4

a1.9+=damons

Comment 40

[Ed Lee :Mardak]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.261; previous revision: 1.260
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.143; previous revision: 1.142
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.45; previous revision: 1.44
done
Checking in toolkit/components/places/tests/unit/test_000_frecency.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_000_frecency.js,v  <--  test_000_frecency.js
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/components/places/tests/unit/test_408221.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_408221.js,v  <--  test_408221.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/places/tests/unit/test_history_autocomplete_tags.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_history_autocomplete_tags.js,v  <--  test_history_autocomplete_tags.js
new revision: 1.3; previous revision: 1.2
done

Comment 41

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

Comment on attachment 304046 [details]
screenshot of v1

I think a better way to render the partial tag names would be:

# Title (<=| tagname)     *

that way we make it clear that it's also a bookmarked page, and associate the tagnames with what they are.

Comment 42

[Ed Lee :Mardak]

The behavior of tag match is a little different than before. Before it would match any of the tags requested while now it matches all of the tags requested. This acts just like typing in more words for matching against the title or url.

Comment 43

[Ondrej Brablc]

With latest trunk I get a syntax error in test_000_frecency.js at line 257:

SyntaxError: missing ; before statement:
       let searchURL = controller.getValueAt(i);
 ......^

Comment 44

[Ondrej Brablc]

(In reply to comment #43)
> With latest trunk I get a syntax error in test_000_frecency.js at line 257:
> 
> SyntaxError: missing ; before statement:
>        let searchURL = controller.getValueAt(i);
>  ......^
> 

This will not be related to this bug. I seem to have some general problem with let.

Comment 45

[Ed Lee :Mardak]

Are you using something other than "make check" to run the tests? It should look something like..

./objdir/dist/bin/xpcshell -v 180 ...

That'll use javascript version 1.8

Comment 46

[Ondrej Brablc]

The problem has gone, and unfortunately I do not know how. Building from scratch from clean check-out and restarting afterwards (sounds really strange) solved the problem.

Comment 47

[Stephen Donner [:stephend] Not actively reading bugmail]

in-litmus+:

https://litmus.mozilla.org/show_test.cgi?id=5258

Verified FIXED using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041306 Minefield/3.0pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041304 Minefield/3.0pre

What hotness (checked multiple tags too).