Remove wstring from autocomplete.

Remove wstring from autocomplete.

Recommit of r72380.

BUG=23581
TEST=no visible changes; all tests pass


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

[Avi (use Gerrit), 2011-01-24 22:08:54]

Same as before, change in extensions/extension_omnibox_apitest.cc and
autocomplete/autocomplete_browsertest.cc to use the size_t modifier.

[evanm, 2011-01-24 22:10:25]

LGTM++

http://codereview.chromium.org/6306011/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_omnibox_apitest.cc (left):

http://codereview.chromium.org/6306011/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_omnibox_apitest.cc:35: std::wstring
output(base::StringPrintf(L"{%d} ", result.size()));
whaaa, the old code used to work?  hate computers.

[Avi (use Gerrit), 2011-01-24 22:12:56]

http://codereview.chromium.org/6306011/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_omnibox_apitest.cc (left):

http://codereview.chromium.org/6306011/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_omnibox_apitest.cc:35: std::wstring
output(base::StringPrintf(L"{%d} ", result.size()));
Yeah, dunno why. Too bad for me doing the Right Thing and fixing it, eh? :)

[James Su, 2011-01-26 01:42:05]

http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1436: string16
text_wstr = UTF8ToUTF16(text);
This line causes crash when text is NULL. As UTF8ToUTF16() only accepts
std::string, which doesn't like NULL pointer at all. While UTF8ToWide() accepts
StringPiece, which is ok for NULL.
We need to care about such difference when migrating from wstring to string16.

[Avi (use Gerrit), 2011-01-26 04:56:34]

That inconsistency between UTF8ToUTF16 and UTF8ToWide is really troubling.
Those are really very different semantics for two similarly named functions
to have and I can't be the only one who's been bitten.

std::wstring UTF8ToWide(const base::StringPiece& utf8);
string16 UTF8ToUTF16(const std::string& utf8);

So StringPiece can construct from null? Ew. Given that these are const
references, I wouldn't think that it would be a good idea to rely on the
null thing, and that seems like a latent bug in
the autocomplete_edit_view_gtk code that was exposed by the conversion.

Avi

On Tue, Jan 25, 2011 at 8:42 PM, <suzhe@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
>
>
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1436: string16
> text_wstr = UTF8ToUTF16(text);
> This line causes crash when text is NULL. As UTF8ToUTF16() only accepts
> std::string, which doesn't like NULL pointer at all. While UTF8ToWide()
> accepts StringPiece, which is ok for NULL.
> We need to care about such difference when migrating from wstring to
> string16.
>
>
> http://codereview.chromium.org/6306011/
>

[James Su, 2011-01-26 05:13:41]

On 2011/01/26 04:56:34, Avi wrote:
> That inconsistency between UTF8ToUTF16 and UTF8ToWide is really troubling.
> Those are really very different semantics for two similarly named functions
> to have and I can't be the only one who's been bitten.
> 
> std::wstring UTF8ToWide(const base::StringPiece& utf8);
> string16 UTF8ToUTF16(const std::string& utf8);
> 
> So StringPiece can construct from null? Ew. Given that these are const
> references, I wouldn't think that it would be a good idea to rely on the
> null thing, and that seems like a latent bug in
> the autocomplete_edit_view_gtk code that was exposed by the conversion.
Yes. We need to fix autocomplete_edit_view_gtk. I'm just wondering if there are
other similar hidden bugs yet to be discovered.

> 
> Avi
> 
> On Tue, Jan 25, 2011 at 8:42 PM, <mailto:suzhe@chromium.org> wrote:
> 
> >
> >
> >
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
> > chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1436: string16
> > text_wstr = UTF8ToUTF16(text);
> > This line causes crash when text is NULL. As UTF8ToUTF16() only accepts
> > std::string, which doesn't like NULL pointer at all. While UTF8ToWide()
> > accepts StringPiece, which is ok for NULL.
> > We need to care about such difference when migrating from wstring to
> > string16.
> >
> >
> > http://codereview.chromium.org/6306011/
> >

[Avi (use Gerrit), 2011-01-26 14:53:40]

That's a really good question. Can we DCHECK on a StringPiece construction
from null? Is that something we want to disallow?

Avi

On Wed, Jan 26, 2011 at 12:13 AM, <suzhe@chromium.org> wrote:

> On 2011/01/26 04:56:34, Avi wrote:
>
>> That inconsistency between UTF8ToUTF16 and UTF8ToWide is really troubling.
>> Those are really very different semantics for two similarly named
>> functions
>> to have and I can't be the only one who's been bitten.
>>
>
>  std::wstring UTF8ToWide(const base::StringPiece& utf8);
>> string16 UTF8ToUTF16(const std::string& utf8);
>>
>
>  So StringPiece can construct from null? Ew. Given that these are const
>> references, I wouldn't think that it would be a good idea to rely on the
>> null thing, and that seems like a latent bug in
>> the autocomplete_edit_view_gtk code that was exposed by the conversion.
>>
> Yes. We need to fix autocomplete_edit_view_gtk. I'm just wondering if there
> are
> other similar hidden bugs yet to be discovered.
>
>
>  Avi
>>
>
>  On Tue, Jan 25, 2011 at 8:42 PM, <mailto:suzhe@chromium.org> wrote:
>>
>
>  >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
>
>> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
>
>> > chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1436: string16
>> > text_wstr = UTF8ToUTF16(text);
>> > This line causes crash when text is NULL. As UTF8ToUTF16() only accepts
>> > std::string, which doesn't like NULL pointer at all. While UTF8ToWide()
>> > accepts StringPiece, which is ok for NULL.
>> > We need to care about such difference when migrating from wstring to
>> > string16.
>> >
>> >
>> > http://codereview.chromium.org/6306011/
>> >
>>
>
>
>
> http://codereview.chromium.org/6306011/
>

[Avi (use Gerrit), 2011-01-26 14:56:22]

Looking, StringPiece construction from NULL seems to be an explicit feature.

Avi

On Wed, Jan 26, 2011 at 9:53 AM, Avi Drissman <avi@chromium.org> wrote:

> That's a really good question. Can we DCHECK on a StringPiece construction
> from null? Is that something we want to disallow?
>
> Avi
>
>
> On Wed, Jan 26, 2011 at 12:13 AM, <suzhe@chromium.org> wrote:
>
>> On 2011/01/26 04:56:34, Avi wrote:
>>
>>> That inconsistency between UTF8ToUTF16 and UTF8ToWide is really
>>> troubling.
>>> Those are really very different semantics for two similarly named
>>> functions
>>> to have and I can't be the only one who's been bitten.
>>>
>>
>>  std::wstring UTF8ToWide(const base::StringPiece& utf8);
>>> string16 UTF8ToUTF16(const std::string& utf8);
>>>
>>
>>  So StringPiece can construct from null? Ew. Given that these are const
>>> references, I wouldn't think that it would be a good idea to rely on the
>>> null thing, and that seems like a latent bug in
>>> the autocomplete_edit_view_gtk code that was exposed by the conversion.
>>>
>> Yes. We need to fix autocomplete_edit_view_gtk. I'm just wondering if
>> there are
>> other similar hidden bugs yet to be discovered.
>>
>>
>>  Avi
>>>
>>
>>  On Tue, Jan 25, 2011 at 8:42 PM, <mailto:suzhe@chromium.org> wrote:
>>>
>>
>>  >
>>> >
>>> >
>>>
>>
>>
>>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
>>
>>> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
>>> >
>>> >
>>> >
>>>
>>
>>
>>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
>>
>>> > chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1436:
>>> string16
>>> > text_wstr = UTF8ToUTF16(text);
>>> > This line causes crash when text is NULL. As UTF8ToUTF16() only accepts
>>> > std::string, which doesn't like NULL pointer at all. While UTF8ToWide()
>>> > accepts StringPiece, which is ok for NULL.
>>> > We need to care about such difference when migrating from wstring to
>>> > string16.
>>> >
>>> >
>>> > http://codereview.chromium.org/6306011/
>>> >
>>>
>>
>>
>>
>> http://codereview.chromium.org/6306011/
>>
>
>

[James Su, 2011-01-26 20:01:38]

I suggest to patch UTF8ToUTF16 to accept const StringPiece& instead of const
std::string&.

On 2011/01/26 14:56:22, Avi wrote:
> Looking, StringPiece construction from NULL seems to be an explicit feature.
> 
> Avi
> 
> On Wed, Jan 26, 2011 at 9:53 AM, Avi Drissman <mailto:avi@chromium.org> wrote:
> 
> > That's a really good question. Can we DCHECK on a StringPiece construction
> > from null? Is that something we want to disallow?
> >
> > Avi
> >
> >
> > On Wed, Jan 26, 2011 at 12:13 AM, <mailto:suzhe@chromium.org> wrote:
> >
> >> On 2011/01/26 04:56:34, Avi wrote:
> >>
> >>> That inconsistency between UTF8ToUTF16 and UTF8ToWide is really
> >>> troubling.
> >>> Those are really very different semantics for two similarly named
> >>> functions
> >>> to have and I can't be the only one who's been bitten.
> >>>
> >>
> >>  std::wstring UTF8ToWide(const base::StringPiece& utf8);
> >>> string16 UTF8ToUTF16(const std::string& utf8);
> >>>
> >>
> >>  So StringPiece can construct from null? Ew. Given that these are const
> >>> references, I wouldn't think that it would be a good idea to rely on the
> >>> null thing, and that seems like a latent bug in
> >>> the autocomplete_edit_view_gtk code that was exposed by the conversion.
> >>>
> >> Yes. We need to fix autocomplete_edit_view_gtk. I'm just wondering if
> >> there are
> >> other similar hidden bugs yet to be discovered.
> >>
> >>
> >>  Avi
> >>>
> >>
> >>  On Tue, Jan 25, 2011 at 8:42 PM, <mailto:suzhe@chromium.org> wrote:
> >>>
> >>
> >>  >
> >>> >
> >>> >
> >>>
> >>
> >>
> >>
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
> >>
> >>> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> >>> >
> >>> >
> >>> >
> >>>
> >>
> >>
> >>
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
> >>
> >>> > chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1436:
> >>> string16
> >>> > text_wstr = UTF8ToUTF16(text);
> >>> > This line causes crash when text is NULL. As UTF8ToUTF16() only accepts
> >>> > std::string, which doesn't like NULL pointer at all. While UTF8ToWide()
> >>> > accepts StringPiece, which is ok for NULL.
> >>> > We need to care about such difference when migrating from wstring to
> >>> > string16.
> >>> >
> >>> >
> >>> > http://codereview.chromium.org/6306011/
> >>> >
> >>>
> >>
> >>
> >>
> >> http://codereview.chromium.org/6306011/
> >>
> >
> >

[evanm, 2011-01-26 20:08:05]

Yeah, that saves copying the char data when calling
  UTF8ToUTF16("foo")
(which would otherwise create a std::string)

On Wed, Jan 26, 2011 at 12:01 PM,  <suzhe@chromium.org> wrote:
> I suggest to patch UTF8ToUTF16 to accept const StringPiece& instead of const
> std::string&.
>
> On 2011/01/26 14:56:22, Avi wrote:
>>
>> Looking, StringPiece construction from NULL seems to be an explicit
>> feature.
>
>> Avi
>
>> On Wed, Jan 26, 2011 at 9:53 AM, Avi Drissman <mailto:avi@chromium.org>
>> wrote:
>
>> > That's a really good question. Can we DCHECK on a StringPiece
>> > construction
>> > from null? Is that something we want to disallow?
>> >
>> > Avi
>> >
>> >
>> > On Wed, Jan 26, 2011 at 12:13 AM, <mailto:suzhe@chromium.org> wrote:
>> >
>> >> On 2011/01/26 04:56:34, Avi wrote:
>> >>
>> >>> That inconsistency between UTF8ToUTF16 and UTF8ToWide is really
>> >>> troubling.
>> >>> Those are really very different semantics for two similarly named
>> >>> functions
>> >>> to have and I can't be the only one who's been bitten.
>> >>>
>> >>
>> >>  std::wstring UTF8ToWide(const base::StringPiece& utf8);
>> >>> string16 UTF8ToUTF16(const std::string& utf8);
>> >>>
>> >>
>> >>  So StringPiece can construct from null? Ew. Given that these are const
>> >>> references, I wouldn't think that it would be a good idea to rely on
>> >>> the
>> >>> null thing, and that seems like a latent bug in
>> >>> the autocomplete_edit_view_gtk code that was exposed by the
>> >>> conversion.
>> >>>
>> >> Yes. We need to fix autocomplete_edit_view_gtk. I'm just wondering if
>> >> there are
>> >> other similar hidden bugs yet to be discovered.
>> >>
>> >>
>> >>  Avi
>> >>>
>> >>
>> >>  On Tue, Jan 25, 2011 at 8:42 PM, <mailto:suzhe@chromium.org> wrote:
>> >>>
>> >>
>> >>  >
>> >>> >
>> >>> >
>> >>>
>> >>
>> >>
>> >>
>
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
>>
>> >>
>> >>> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc
>> >>> > (right):
>> >>> >
>> >>> >
>> >>> >
>> >>>
>> >>
>> >>
>> >>
>
>
http://codereview.chromium.org/6306011/diff/56002/chrome/browser/autocomplete...
>>
>> >>
>> >>> > chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1436:
>> >>> string16
>> >>> > text_wstr = UTF8ToUTF16(text);
>> >>> > This line causes crash when text is NULL. As UTF8ToUTF16() only
>> >>> > accepts
>> >>> > std::string, which doesn't like NULL pointer at all. While
>> >>> > UTF8ToWide()
>> >>> > accepts StringPiece, which is ok for NULL.
>> >>> > We need to care about such difference when migrating from wstring to
>> >>> > string16.
>> >>> >
>> >>> >
>> >>> > http://codereview.chromium.org/6306011/
>> >>> >
>> >>>
>> >>
>> >>
>> >>
>> >> http://codereview.chromium.org/6306011/
>> >>
>> >
>> >
>
>
>
> http://codereview.chromium.org/6306011/
>