Port the spell checker to posix.

Port the spell checker to posix.

It all builds but does not link yet.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=7109

[Evan Stade, 2008-12-13 00:12:02]

http://codereview.chromium.org/14408/diff/1/4
File chrome/browser/spellchecker.cc (left):

http://codereview.chromium.org/14408/diff/1/4#oldcode27
Line 27: #include "chrome/common/win_util.h"
All these deleted headers were apparently unused.

[Peter Kasting, 2008-12-13 01:14:39]

LGTM, Except I feel uneasy about string16, but I don't know much

http://codereview.chromium.org/14408/diff/1/2
File base/basictypes.h (right):

http://codereview.chromium.org/14408/diff/1/2#newcode105
Line 105: // cases, you have to use the unsafe ARRAYSIZE_UNSAFE() macro below. 
This is
Was this file just merged badly before?  Maybe it makes sense to do this file's
changes separately?

http://codereview.chromium.org/14408/diff/1/7
File chrome/browser/spellcheck_worditerator.h (right):

http://codereview.chromium.org/14408/diff/1/7#newcode12
Line 12: #include "base/string16.h"
I don't know the cases where we're using string16 already.  We're converting
down to UTF-8 for hunspell, does it make sense to just go to utf-8 here and
force callers to do this?  I feel like string and wstring tend to be preferable
to string16 :/

http://codereview.chromium.org/14408/diff/1/6
File chrome/browser/spellchecker.h (left):

http://codereview.chromium.org/14408/diff/1/6#oldcode110
Line 110: bool IsValidContraction(const Language& word);
(Oops, sorry about this one)

http://codereview.chromium.org/14408/diff/1/9
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/14408/diff/1/9#newcode1386
Line 1386: struct ParamTraits<scoped_refptr<net::HttpResponseHeaders> > {
Similarly, maybe this should be in another change?  I dunno

[Evan Stade, 2008-12-13 20:22:06]

Who wrote this initially? (svn blame says initial commit) Can we get their
opinion on changing from UTF16 to UTF8 for SpellcheckWordIterator words?

http://codereview.chromium.org/14408/diff/1/2
File base/basictypes.h (right):

http://codereview.chromium.org/14408/diff/1/2#newcode105
Line 105: // cases, you have to use the unsafe ARRAYSIZE_UNSAFE() macro below. 
This is
On 2008/12/13 01:14:39, pkasting wrote:
> Was this file just merged badly before?  Maybe it makes sense to do this
file's
> changes separately?

Yea it would make sense, although that would be a dinky changelist. (only
changing comments?)

http://codereview.chromium.org/14408/diff/1/7
File chrome/browser/spellcheck_worditerator.h (right):

http://codereview.chromium.org/14408/diff/1/7#newcode12
Line 12: #include "base/string16.h"
On 2008/12/13 01:14:39, pkasting wrote:
> I don't know the cases where we're using string16 already.  We're converting
> down to UTF-8 for hunspell, does it make sense to just go to utf-8 here and
> force callers to do this?  I feel like string and wstring tend to be
preferable
> to string16 :/

I didn't want to change to string because I didn't know why wstring was chosen
in the first place. But it would be easy to change to string/utf8 since
spellchecker is the only caller (right?)

As far as wstring being preferable to string16, I don't know why this would be
the case---it seems like using wstring on multiple platforms will only lead to
confusion and bugs.

http://codereview.chromium.org/14408/diff/1/9
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/14408/diff/1/9#newcode1386
Line 1386: struct ParamTraits<scoped_refptr<net::HttpResponseHeaders> > {
On 2008/12/13 01:14:39, pkasting wrote:
> Similarly, maybe this should be in another change?  I dunno

same here, *shrug*

[Peter Kasting, 2008-12-13 21:30:55]

On 2008/12/13 20:22:06, Evan Stade wrote:
> Who wrote this initially? (svn blame says initial commit)

Use http://svn/v to get older history (only available internally).

sidchat is the spellcheck author.

> Yea it would make sense, although that would be a dinky changelist. (only
> changing comments?)

Well, you changed one actual usage to ARRAYSIZE_UNSAFE.

I would go ahead and commit the changes here separately, r=me

> http://codereview.chromium.org/14408/diff/1/7#newcode12
> Line 12: #include "base/string16.h"
> On 2008/12/13 01:14:39, pkasting wrote:
> > I don't know the cases where we're using string16 already.  We're converting
> > down to UTF-8 for hunspell, does it make sense to just go to utf-8 here and
> > force callers to do this?  I feel like string and wstring tend to be
> preferable
> > to string16 :/
> 
> I didn't want to change to string because I didn't know why wstring was chosen
> in the first place. But it would be easy to change to string/utf8 since
> spellchecker is the only caller (right?)
> 
> As far as wstring being preferable to string16, I don't know why this would be
> the case---it seems like using wstring on multiple platforms will only lead to
> confusion and bugs.

We use wstring pretty extensively in frontend code.  wstring doesn't imply bugs
unless code assumes wchar_t is 16 bits (which this code doesn't).  We don't use
string16 almost anywhere so I would avoid it unless you're sure it's precisely
what you want.

It may be true that the original source of a lot of this is UTF-8 anyway, in
which case we can save conversions by making all this code UTF-8.  But if it's
not, and it's coming from wstrings somewhere (which I believe it is), leave
things as wstring.

[Evan Stade, 2008-12-15 07:20:38]

> 
> Use http://svn/v to get older history (only available internally).

k sweet.

> Well, you changed one actual usage to ARRAYSIZE_UNSAFE.
> 
> I would go ahead and commit the changes here separately, r=me

Hang with me here if I'm being dense, but I actually make a lot more than one
actual usage change (see spellcheck_unittest). And the change to
ARRAYSIZE_UNSAFE is due to the porting effort (apparently you can use
arraysize() on types defined within functions on windows, but not posix). So why
should this change be separate?

> It may be true that the original source of a lot of this is UTF-8 anyway, in
> which case we can save conversions by making all this code UTF-8.  But if it's
> not, and it's coming from wstrings somewhere (which I believe it is), leave
> things as wstring.

I actually am not sure how we would change it to UTF-8 or leave it as wstring,
because the whole class is largely a wrapper for unorm_normalize, which seems to
be available only for UChar string (i.e. UTF-16).

http://codereview.chromium.org/14408/diff/1/7
File chrome/browser/spellcheck_worditerator.h (right):

http://codereview.chromium.org/14408/diff/1/7#newcode97
Line 97: //   * word [in] (const char16*)
by the way, can we change this parameter name from |word| to |words| or
|contents| or |passage| or something?

[Evan Stade, 2008-12-15 07:21:22]

> by the way, can we change this parameter name from |word| to |words| or
> |contents| or |passage| or something?

Maybe |text|.

[Peter Kasting, 2008-12-15 17:52:12]

On 2008/12/15 07:20:38, Evan Stade wrote:
> > Well, you changed one actual usage to ARRAYSIZE_UNSAFE.
> 
> Hang with me here if I'm being dense, but I actually make a lot more than one
> actual usage change (see spellcheck_unittest).

I meant "one change in basictypes.h".  That's the change I meant to commit
separately.  You're right that changes to the spellcheck code are needed too,
but since that file doesn't compile right now anyway (right?), it won't hurt
anything to not change it instantly.

The only reasons I'm suggesting doing this separately are because updating
basictypes.h to document (and use) ARRAYSIZE_UNSAFE properly is a necessary but
separate change compared to "porting the spellchecker"; and I was trying to help
get that in place immediately since presumably it will affect other code too.

> I actually am not sure how we would change it to UTF-8 or leave it as wstring,
> because the whole class is largely a wrapper for unorm_normalize, which seems
to
> be available only for UChar string (i.e. UTF-16).

I don't know how we've dealt with this elsewhere in the code.  I thought that on
Mac and Linux we were still using wstrings to hold UCS-2 encoded strings (which
is a waste of memory, but whatever) in much of the frontend code, and switching
to UTF-8 otherwise.  I don't know this spellchecker code _or_ our string usage
patterns at all, so I'm going to throw up my hands and ask you to find a better
reviewer, perhaps awalker or somebody.

[Evan Stade, 2008-12-15 18:06:50]

> The only reasons I'm suggesting doing this separately are because updating
> basictypes.h to document (and use) ARRAYSIZE_UNSAFE properly is a necessary
but
> separate change compared to "porting the spellchecker"; and I was trying to
help
> get that in place immediately since presumably it will affect other code too.

The only changes to basictypes.h is documentation, there are no usage changes.

>   I don't know this spellchecker code _or_ our string usage
> patterns at all, so I'm going to throw up my hands and ask you to find a
better
> reviewer, perhaps awalker or somebody.

OK, will do, thanks.

[Peter Kasting, 2008-12-15 18:08:41]

On 2008/12/15 18:06:50, Evan Stade wrote:
> The only changes to basictypes.h is documentation, there are no usage changes.

Oh.  Yeah.  Somehow I didn't notice the "//" in front of the very last change. 
I stupid :(

[sidchat (Google), 2008-12-15 18:20:56]

Sorry to chip in late. 

The Spell Check code changes look good to me, although I am not sure about the
part about choosing string16 over wstring. Peter made all the valid points - I
believe part of the word iterator was written by Brett (brettw). You may want to
seek his opinion.

-Sid

On 2008/12/15 18:08:41, pkasting wrote:
> On 2008/12/15 18:06:50, Evan Stade wrote:
> > The only changes to basictypes.h is documentation, there are no usage
changes.
> 
> Oh.  Yeah.  Somehow I didn't notice the "//" in front of the very last change.

> I stupid :(

[Evan Stade, 2008-12-15 18:29:21]

Added brettw.

I'm concerned about the misspell_location and misspell_length parameters. If we
actually pass in UTF-32 strings then changing to UTF-16 may mess these up...
right? Don't know how to get around this, since unorm_* operates on UTF-16.

[Peter Kasting, 2008-12-15 18:52:22]

On 2008/12/15 18:29:21, Evan Stade wrote:
> I'm concerned about the misspell_location and misspell_length parameters. If
we
> actually pass in UTF-32 strings then changing to UTF-16 may mess these up...
> right?

But where do we have UTF-32 strings?  I don't think we ever encode anything as
UTF-32.  I believe all the wstrings sent to the spellchecker are either UTF-16
or UCS-2 (not sure which).

(Linux ==> wstring is 32 bits =/=> UTF-32 encoding)

[Evan Stade, 2008-12-15 19:28:11]

>wstring doesn't bugs unless code assumes wchar_t is 16 bits (which this code
>doesn't).  We don't use string16 almost anywhere so I would avoid it
>unless you're sure it's precisely what you want.

This code does assume 16 bits: unorm_* requires 16-bit UTF-16 characters. It
won't compile without changing wstring to string16.

> I think Peter is right. WebKit deals in UTF-16, so the offsets and
> lengths will all be relative to that. Probably we want to keep the
> UTF-16 encoding for spellchecking until right before we give the
> individual words to hunspell (in UTF-8).

yea, that sounds right. It seems wasteful not only of space but also of time to
switch from 16 bits to 32 bits to 16 bits again just to spell check. So I'd
think it would make sense to change the entire spell checker (not just word
iterator) to string16.

[Peter Kasting, 2008-12-15 19:35:04]

On 2008/12/15 19:28:11, Evan Stade wrote:
> This code does assume 16 bits: unorm_* requires 16-bit UTF-16 characters. It
> won't compile without changing wstring to string16.

What do we do in the other places we call ICU code?  Or is this the first one
we're attempting to port?

> yea, that sounds right. It seems wasteful not only of space but also of time
to
> switch from 16 bits to 32 bits to 16 bits again just to spell check.

We may not be switching from 16 to 32 bits, if WebKit uses something like
wstring rather than something like string16.  Also note that since we're not
re-encoding the cost of a transformation is roughly the cost of one memcpy, so
you're only going to win if you reduce the number of copies.  If you change the
whole spellchecker to string16, do you omit copies going in?  Or does string16
propagate virally through the IPC layer, etc.?  (I suspect the latter.)

[Evan Stade, 2008-12-15 19:56:14]

> > This code does assume 16 bits: unorm_* requires 16-bit UTF-16 characters. It
> > won't compile without changing wstring to string16.
> 
> What do we do in the other places we call ICU code?  Or is this the first one
> we're attempting to port?

added awalker, evan to answer this question. 

> We may not be switching from 16 to 32 bits, if WebKit uses something like
> wstring rather than something like string16.  Also note that since we're not
> re-encoding the cost of a transformation is roughly the cost of one memcpy, so
> you're only going to win if you reduce the number of copies.  If you change
the
> whole spellchecker to string16, do you omit copies going in?  Or does string16
> propagate virally through the IPC layer, etc.?  (I suspect the latter.)

Webkit uses something more like string16. WebCore::String is a string of UChars.

We might eventually have to change the IPC call as well (whenever those get
ported), but for now, since we are only compiling them on windows, string16 ==
wstring, so no changes necessary to IPC.

[Evan Martin, 2008-12-16 19:34:35]

I found base/word_iterator.cc, which has a gnarly #if defined(WCHAR_T_IS_UTF16)
and makes me sad.

After going back and forth with Tony a bit about this, I think this change is
fine.  We'll probably need to figure out our plan for wchar for the rest of the
world soon.

http://codereview.chromium.org/14408/diff/1/4
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/14408/diff/1/4#newcode353
Line 353: MSVC_SUPPRESS_WARNING(4355)  // Okay to pass "this" here.
I feel like there might be a macro that is explicitly for this case, but I
forget what it is.  Maybe it lives in compiler_specific.h.