All host names with nonascii characters (cyrillic) + escapable characters (,)...

All host names with nonascii characters (cyrillic) + escapable characters (,)...
were shown corrupted in suggestion box in Omnibox2,
because we escaped punicode string after it was created.
I added some more escaping before IDNA conversion.

Unittest updated, and main method added so we can init ICU data properly.

BUG=5490
TEST=Follow bug description, and make sure that test is as expected (orignal
text+%28).

[Nebojša Ćirić, 2009-08-04 18:09:57]

For some reason SVN wouldn't cooperate with adding new files to old CL, so I had
to create new one.

Added unittest, had to update gyp file for it to allow for gunit_no_main...

[brettw, 2009-08-07 16:53:35]

I'm confused. This new code takes a UTF-16 string, converts it to UTF-8 and
escapes via DoSimpleHost. This will give you an ASCII string, right? Won't
IDNToASCII do nothing in this case?

http://codereview.chromium.org/160589/diff/1001/1003
File googleurl/src/url_canon_host.cc (right):

http://codereview.chromium.org/160589/diff/1001/1003#newcode187
Line 187: RawCanonOutput<1024> url_escaped_host;
Use kTempHostBufferLen instead of 1024 here.

http://codereview.chromium.org/160589/diff/1001/1003#newcode189
Line 189: WideToUTF8(src, src_len, &utf8_host_name);
This file shouldn't depend on STL.

[Nebojša Ćirić, 2009-08-07 17:42:53]

On 2009/08/07 16:53:35, brettw wrote:
> I'm confused. This new code takes a UTF-16 string, converts it to UTF-8 and
> escapes via DoSimpleHost. This will give you an ASCII string, right? Won't
> IDNToASCII do nothing in this case?

We need to escape host name before it gets into IDNToASCII. And we do UTF8ToWide
just before the call to IDNToASCII.
So from "т(" -(WideToUTF8)-> "\0xd1\0x82(" -(DoSimpleHost)-> "\0xd1\0x82%28"
-(UTF8ToWide)-> "т%28" -(IDNToASCII)-> "xn--%28-7ed".

If you know of a method that can escape wide string we could cut lots of
conversion here... I couldn't find one.

> http://codereview.chromium.org/160589/diff/1001/1003
> File googleurl/src/url_canon_host.cc (right):
> 
> http://codereview.chromium.org/160589/diff/1001/1003#newcode187
> Line 187: RawCanonOutput<1024> url_escaped_host;
> Use kTempHostBufferLen instead of 1024 here.

Done.

> http://codereview.chromium.org/160589/diff/1001/1003#newcode189
> Line 189: WideToUTF8(src, src_len, &utf8_host_name);
> This file shouldn't depend on STL.

I do need conversion from Wide/UTF8. Do you have any suggestions on what else
could I use to avoid STL (like a method that can URL escape wide strings)?

[Nebojša Ćirić, 2009-08-07 17:53:45]

It seems that Linux and Mac bots don't like my cl:

.../googleurl/src/url_canon_host.cc: In function 'bool
url_canon::<unnamed>::DoIDNHost(const char16*, int, url_canon::CanonOutput*)':
.../googleurl/src/url_canon_host.cc:189: error: no matching function for call to
'WideToUTF8(const char16*&, int&, std::string*)'
.../base/string_util.h:196: note: candidates are: bool WideToUTF8(const
wchar_t*, size_t, std::string*)
.../base/string_util.h:197: note:                 std::string WideToUTF8(const
std::wstring&)
.../googleurl/src/url_canon_host.cc:201: error: cannot convert 'const wchar_t*'
to 'const char16*'

On 2009/08/07 17:42:53, Nebojša Ćirić wrote:
> On 2009/08/07 16:53:35, brettw wrote:
> > I'm confused. This new code takes a UTF-16 string, converts it to UTF-8 and
> > escapes via DoSimpleHost. This will give you an ASCII string, right? Won't
> > IDNToASCII do nothing in this case?
> 
> We need to escape host name before it gets into IDNToASCII. And we do
UTF8ToWide
> just before the call to IDNToASCII.
> So from "т(" -(WideToUTF8)-> "\0xd1\0x82(" -(DoSimpleHost)-> "\0xd1\0x82%28"
> -(UTF8ToWide)-> "т%28" -(IDNToASCII)-> "xn--%28-7ed".
> 
> If you know of a method that can escape wide string we could cut lots of
> conversion here... I couldn't find one.
> 
> > http://codereview.chromium.org/160589/diff/1001/1003
> > File googleurl/src/url_canon_host.cc (right):
> > 
> > http://codereview.chromium.org/160589/diff/1001/1003#newcode187
> > Line 187: RawCanonOutput<1024> url_escaped_host;
> > Use kTempHostBufferLen instead of 1024 here.
> 
> Done.
> 
> > http://codereview.chromium.org/160589/diff/1001/1003#newcode189
> > Line 189: WideToUTF8(src, src_len, &utf8_host_name);
> > This file shouldn't depend on STL.
> 
> I do need conversion from Wide/UTF8. Do you have any suggestions on what else
> could I use to avoid STL (like a method that can URL escape wide strings)?

[brettw, 2009-08-07 18:34:06]

What do you think about this approach:

DoSimpleHost takes an additional template value for the output type. Then it can
preserve the UTF-16-ness of the input while simultaneously escaping 7-bit
values.

You will also need to pass the "unsigned input value" type as DoHost has, and
use that instead of "unsigned char" in that function.

So the function decl will look like this:

template<INCHAR, UINCHAR, OUTCHAR>
DoSimpleHost(INCHAR* host, int host_len, CanonOutputT<OUTCHAR>* output, bool*
has_non_ascii)

[Nebojša Ćirić, 2009-08-07 20:38:33]

It looks ok, except for:

DoSimpleHost expects 8bit characters (no matter the container), so our original
char16* input "т(" (422, 0x28) won't work.
Internal logic of DoSimpleHost relies on 8bit values (source, DecodeEscaped,
AppendEscapedChar, source >= 0x80)... I would have to rewrite good chunk of that
code to enable true char16 escaping.

What do you think about keeping it as is, and maybe revisiting this at some
point?

On 2009/08/07 18:34:06, brettw wrote:
> What do you think about this approach:
> 
> DoSimpleHost takes an additional template value for the output type. Then it
can
> preserve the UTF-16-ness of the input while simultaneously escaping 7-bit
> values.
> 
> You will also need to pass the "unsigned input value" type as DoHost has, and
> use that instead of "unsigned char" in that function.
> 
> So the function decl will look like this:
> 
> template<INCHAR, UINCHAR, OUTCHAR>
> DoSimpleHost(INCHAR* host, int host_len, CanonOutputT<OUTCHAR>* output, bool*
> has_non_ascii)

[Nebojša Ćirić, 2009-08-07 20:57:31]

Last patch (4) took care of failing linux/mac bots.

[Nebojša Ćirić, 2009-08-07 23:06:22]

Could you take a look again. I did what you suggested.

http://codereview.chromium.org/160589/diff/2012/3020
File googleurl/src/url_canon_host.cc (right):

http://codereview.chromium.org/160589/diff/2012/3020#newcode146
Line 146: unsigned int source = host[i];
There is no unsigned char16, and unsigned int works in all cases so I used it.

http://codereview.chromium.org/160589/diff/2012/3021
File googleurl/src/url_canon_internal.h (right):

http://codereview.chromium.org/160589/diff/2012/3021#newcode148
Line 148: CanonOutputT<OUTCHAR>* output) {
Escaping makes sense only on lower 8bits, so code works for all cases.

[Nebojša Ćirić, 2009-08-10 21:32:20]

Build bots are happy with this CL.

On 2009/08/07 23:06:22, Nebojša Ćirić wrote:
> Could you take a look again. I did what you suggested.
> 
> http://codereview.chromium.org/160589/diff/2012/3020
> File googleurl/src/url_canon_host.cc (right):
> 
> http://codereview.chromium.org/160589/diff/2012/3020#newcode146
> Line 146: unsigned int source = host[i];
> There is no unsigned char16, and unsigned int works in all cases so I used it.
> 
> http://codereview.chromium.org/160589/diff/2012/3021
> File googleurl/src/url_canon_internal.h (right):
> 
> http://codereview.chromium.org/160589/diff/2012/3021#newcode148
> Line 148: CanonOutputT<OUTCHAR>* output) {
> Escaping makes sense only on lower 8bits, so code works for all cases.

[brettw, 2009-08-11 00:51:31]

http://codereview.chromium.org/160589/diff/2012/3021
File googleurl/src/url_canon_internal.h (right):

http://codereview.chromium.org/160589/diff/2012/3021#newcode148
Line 148: CanonOutputT<OUTCHAR>* output) {
On 2009/08/07 23:06:22, Nebojša Ćirić wrote:
> Escaping makes sense only on lower 8bits, so code works for all cases.

Can you add a comment to this effect above the function?

http://codereview.chromium.org/160589/diff/2012/3022
File googleurl/src/url_canon_unittest.cc (right):

http://codereview.chromium.org/160589/diff/2012/3022#newcode1929
Line 1929: // Some tests may use base::Singleton<>, thus we need to instantiate
But I thought that we weren't going to use base here? Using base means that
other users of this library won't be able to compile this file.

I'd also suggest putting this main in a new file. It seems weird to have it here
when there are several unit test files that all get compiled. With the new file,
you can add the ICU initializing code (probably only a small part of
icu_util::Initialize() and I'm guessing it won't require the AtExit stuff).

[Nebojša Ćirić, 2009-08-11 16:58:02]

I am slightly confused about base requirement, since googleurl.gyp includes
reference to base library, and most of the googleurl source files already point
to "base/basictypes.h", "base/string16.h", "base/logging.h"...

> http://codereview.chromium.org/160589/diff/2012/3022
> File googleurl/src/url_canon_unittest.cc (right):
> 
> http://codereview.chromium.org/160589/diff/2012/3022#newcode1929
> Line 1929: // Some tests may use base::Singleton<>, thus we need to
instantiate
> But I thought that we weren't going to use base here? Using base means that
> other users of this library won't be able to compile this file.
> 
> I'd also suggest putting this main in a new file. It seems weird to have it
here
> when there are several unit test files that all get compiled. With the new
file,
> you can add the ICU initializing code (probably only a small part of
> icu_util::Initialize() and I'm guessing it won't require the AtExit stuff).

[Nebojša Ćirić, 2009-08-11 19:15:17]

All done, and passing on bots.

On 2009/08/11 00:51:31, brettw wrote:
> http://codereview.chromium.org/160589/diff/2012/3021
> File googleurl/src/url_canon_internal.h (right):
> 
> http://codereview.chromium.org/160589/diff/2012/3021#newcode148
> Line 148: CanonOutputT<OUTCHAR>* output) {
> On 2009/08/07 23:06:22, Nebojša Ćirić wrote:
> > Escaping makes sense only on lower 8bits, so code works for all cases.
> 
> Can you add a comment to this effect above the function?
> 
> http://codereview.chromium.org/160589/diff/2012/3022
> File googleurl/src/url_canon_unittest.cc (right):
> 
> http://codereview.chromium.org/160589/diff/2012/3022#newcode1929
> Line 1929: // Some tests may use base::Singleton<>, thus we need to
instantiate
> But I thought that we weren't going to use base here? Using base means that
> other users of this library won't be able to compile this file.
> 
> I'd also suggest putting this main in a new file. It seems weird to have it
here
> when there are several unit test files that all get compiled. With the new
file,
> you can add the ICU initializing code (probably only a small part of
> icu_util::Initialize() and I'm guessing it won't require the AtExit stuff).

[brettw, 2009-08-12 20:41:37]

LGTM, thanks!

[Nebojša Ćirić, 2009-08-12 21:29:19]

Nice :). Submitting as soon as tree opens.
Great CL for learning stuff about gyp, punicode, sln building and stuff.

Thanks for the patience.

Cira

On Wed, Aug 12, 2009 at 1:41 PM, <brettw@chromium.org> wrote:

> LGTM, thanks!
>
>
> http://codereview.chromium.org/160589
>

[brettw, 2009-08-18 23:48:58]

Okay, I checked this in as googleurl revision 113.

This is only the googleurl part. You'll need to submit a patch to update the
DEPS file to pull this revision, and also add the build/temp_gyp... file
simultaneously.

[Nebojša Ćirić, 2009-08-20 00:37:36]

Sent new CL 174093 to you, with DEPS and build files pointing to 113.
Cira

On Tue, Aug 18, 2009 at 4:48 PM, <brettw@chromium.org> wrote:

> Okay, I checked this in as googleurl revision 113.
>
> This is only the googleurl part. You'll need to submit a patch to update
> the DEPS file to pull this revision, and also add the build/temp_gyp...
> file simultaneously.
>
>
> http://codereview.chromium.org/160589
>