383827 - Make bookmark filtering stable

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Stuart Morgan]

Attachment: fix

Right now the bookmark search API builds and returns a set of matching bookmarks, rather than an array. This has several effects:
- bookmark filtering is not at all order-preserving, which is annoying
- we do extra work uniquing things that are already known to be unique
- we do extra work to support fast lookup, when all the client wants or needs is an array to iterate over

Not doing extra work to degrade the UE seems like a no-brainer.

Comment 1

[Chris Lawson (gone)]

Thanks, Stuart.

Are there any advantages of using a set that we might be missing out on here? What if someone has the same page in bookmarks two or three times? (My expectation, I guess, would be that a search finds all instances of that bookmark, not just one, so this is an improvement in my book.)

How about speed impact?

Comment 2

[Stuart Morgan]

We don't define equality that way on bookmarks; it's impossible under the current implementation to have two bookmarks which are equal, which is why I said they are known to be unique. As for speed, I think I was pretty clear that the set is doing extra work.

Comment 3

[Josh Aas]

Comment on attachment 267789 [details] [diff] [review]
fix

Fine with me, but what does this have to do with the description for this bug?

Comment 4

[Stuart Morgan]

(In reply to comment #3)
> Fine with me, but what does this have to do with the description for this bug?

Stable in the sort sense (i.e., order-preserving).

Comment 5

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This apparently doesn't compile against 10.3* SDK or gcc 3:

/builds/tinderbox/Camino/Darwin_7.9.0_Depend/mozilla/camino/src/bookmarks/BookmarkManager.mm: In function `NSArray* -[BookmarkManager searchBookmarksContainer:forString:inFieldWithTag:](BookmarkManager*, objc_selector*, BookmarkFolder*, NSString*, int)':
/builds/tinderbox/Camino/Darwin_7.9.0_Depend/mozilla/camino/src/bookmarks/BookmarkManager.mm:697: error: cannot convert `NSArray*' to `NSSet*' in initialization
** BUILD FAILED **

(Only boxset gives a useful error message, which is bad in another way)

Comment 7

[Stuart Morgan]

I missed a file when checking in; it should go green next time around.