622569 - Use a real segmenter for autocomplete keyword generation

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Stuart Morgan]

Right now when generating trie keywords for history and bookmark titles the segmentation logic is "break at spaces". This does the wrong thing for punctuation (e.g., foo—bar) and languages that don't use spaces (Japanese, Chinese, etc.).

Instead, we should use the built-in OS segmenter.

Comment 1

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

This seems like something we should do for 2.1 (also possibly bug 622565, if the OS segmenter doesn't encapsulate it).

Comment 2

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

[10:53pm] smorgan: We could hard-code a list of separators in the short term
[10:54pm] sauron|frazzled: mm, that would help in most common cases, wouldn't it
[10:54pm] smorgan: :-()[]
[10:54pm] sauron|frazzled: _
[10:54pm] smorgan: .
[10:59pm] sauron|frazzled: should we file a special bug on that?
[11:05pm] smorgan: Nah, we can just do it as a first pass in the breaker bug

Comment 3

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

Attachment: Stab in the dark to implement "first pass" of using more separators

This annoyed me enough today that I wanted to fix it (or at least the "first pass" as detailed in comment 2), and I happened to have a little bit of time again.  In theory it should be an easy fix to http://mxr.mozilla.org/camino/source/camino/src/browser/AutoCompleteKeywordGenerator.mm#92, except that |componentsSeparatedByCharactersInSet:| is 10.5-only.

So I took a giant stab in the dark and and copied our |stringByFooingCharactersInSet| utility methods and made that code populate and return an array.  And, by Jove, it compiles!  And it appears to work, too, at least with today's annoyance-testcase (https://bugzilla.mozilla.org/show_bug.cgi?id=659724, currently entitled "tinderbox queue runner is not keeping up (dm-webtools02)", and trying to autocomplete on "dm" and "webtools").

Assuming by some miracle everything's right so far with the code, should we add any other characters/choose another character set (say, punctuationCharacterSet+whitespaceCharacterSet, or even symbolCharacterSet)?

Comment 4

[Stuart Morgan]

This makes sense, but we need to make sure the query string is broken using the same method (which means we should make a helper that calls this with the set, so there's no chance of call sites getting out of sync). But that means we have to think about what happens to URLs. Hrm.

Let me ponder this for a bit and think about how to make that work.

Comment 5

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

(In reply to Stuart Morgan from comment #4)
> This makes sense, but we need to make sure the query string is broken using
> the same method (which means we should make a helper that calls this with
> the set, so there's no chance of call sites getting out of sync).

So we'd need to change http://mxr.mozilla.org/camino/source/camino/src/browser/AutoCompleteKeywordGenerator.mm#146 in |searchTermsForString:| for this (so that both it and my patch use the proposed helper method for the components-separated-by-charset)?

> But that
> means we have to think about what happens to URLs. Hrm.

Can we move the URL-detection/parsing hunk in |searchTermsForString:| first, and have it handle URLs, and then if it's not a URL, fall through to this new code?

Or did you mean something (somewhere?) else with the query string stuff?

Comment 6

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

@#@$@#$@#$

Definitely 2.1.1 material :P

Comment 7

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

Stuart, any thoughts here yet?

Comment 8

[Stuart Morgan]

Haven't forgotten, just haven't had time to work through this yet.

Comment 10

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

Maybe next time.