Support IDNA 2008

Support IDNA 2008 with UTS46.

UTS 46 provides a migration path from IDNA 2003 to IDNA 2008.

1. Use the up-to-date Unicode data; new characters were
added and some case-folding/mapping have changed since Unicode 3.2 on which
IDNA 2003 is based.
2. Define a case folding/mapping as is the case with IDNA 2003.
3. Use transitional mechanism for 4 deviant characters : German sharp-S,
Greek final-sigma, ZWJ and ZWNJ. That is, the former two are mapped to
'ss' and regular sigma and the latter two are dropped. All the major
browsers do this at the moment so allowing them does not do any good.
We'll review this later as the consensus builds among browser vendors
and registrars. We can also consider handling them separately. For instance,
ZWJ/ZWNJ can be allowed with ContextJ rules, which requires a minor change
in ICU's UTS 46 implementation.
4. Symbol and punctuations continue to be allowed.

We also do the following:

1. Continue to "violate" STD3 rules about non-LDH (Letter, digits and
hyphens) by allowing non-LDH's. That is no change from the current
implementation.
2. Do not allow unassigned code points any more. With an up-to-date
Unicode data, this does not make much difference. And the chance of
new characters not yet reflected in our Unicode data popping up in
a domain name is extremely low.
3. Continue to use CHECK_BIDI. The Bidi rule in IDNA 2008 is more
permissive than in IDNA 2003.

References:
1. http://unicode.org/reports/tr46/ and references therein
   to IDNA 2003 and 2008 RFCs.
2. What IE 10 does : http://goo.gl/3XBhqw
3. Mozilla bug :  https://bugzilla.mozilla.org/show_bug.cgi?id=479520

BUG=61328
TEST=url_unittests (URLCanonTest.Host), net_unittests (NetUtilTest.IDNToU*),
unittests (X509CertificateModelTest.*)

R=brettw@chromium.org, pkasting@chromium.org, rsleevi@chromium.org, thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225878

[abarth-chromium, 2013-09-17 17:51:45]

The description for this CL just describes what the CL does, but it doesn't
explain why we should make this change.  Can you help me understand why
migrating to IDNA 2008 is the right thing to do?

Other browsers appear to be sticking with IDNA 2003, and the URL standard uses
IDNA 2003 as well:

http://annevankesteren.nl/2013/08/idna2003

I assume you understand these issues in more depth than I do, which is why I'd
appreciate a bit more of the "why" behind this change.  Thanks!

[bulach, 2013-09-17 18:26:32]

I haven't been working on the net/ layer for quite a while now, adding rsleevi
as he can certainly provide better feedback.

[jungshik at Google, 2013-09-17 22:52:57]

Accidentally, I've just lost a long reply. I'll try to restore that from my
memory. 

On 2013/09/17 17:51:45, abarth wrote:
> The description for this CL just describes what the CL does, but it doesn't
> explain why we should make this change.  Can you help me understand why
> migrating to IDNA 2008 is the right thing to do?
> 
> Other browsers appear to be sticking with IDNA 2003, and the URL standard uses
> IDNA 2003 as well:
> 
> http://annevankesteren.nl/2013/08/idna2003

Anne's decision appeared to be based on outdated information as to what browsers
do or plan to do. IE made a switch more than a year ago to UTS 46 [1]  and
Mozilla plans to move too. [2]  Mozilla was blocked because of a licensing issue
with idnkit2 from JPNic they use for IDN support. Now that the issue was
resolved, they plan to go ahead. 

His email about the URL spec sticking to IDNA 2003 with updated Unicode data
triggered a lively discussion (as you observed) in which all three browsers
expressed their interests (or what they've already implemented) in going to UTS
46/IDNA 2008. 

> I assume you understand these issues in more depth than I do, which is why I'd
> appreciate a bit more of the "why" behind this change.  Thanks!

First and foremost of all is that we cannot stay with Unicode 3.2 forever. We
have allowed unassigned code points, but just passing through unassigned code
points is not the same as using the up-to-date Unicode data (e.g. case-folding
would be different and normalization behavior would be different). 

Secondly, some domain labels that are "linguistically" valid are banned in IDNA
2003. 
The URL spec (draft) requires that IDNA 2003 rules with the up-to-date Unicode
data be used. It'll still leave out some 'legitimate' domain labels. For
instance, BiDi check in IDNA 2003 is too restrictive and disallows domain labels
ending with a combining mark (both LTR and RTL labels). Mapping away ZWJ/ZWNJ in
*all* cases regardless of the context would disallow some labels distinct from
labels without ZWJ/ZWNJ in some languages. This CL continues to disallow
ZWJ/ZWNJ for now, but I hope to allow them with ContextJ rules (so that it'd not
be a spoofing risk when combined with Chrome's IDN display policy) eventually.

Finally, given that what IE did and Mozilla plans to do, I believe it's time to
move this forward in Chrome, too. 



[1]
http://blogs.msdn.com/b/shawnste/archive/2013/09/09/how-does-ie-handle-the-id...

[2] https://bugzilla.mozilla.org/show_bug.cgi?id=479520

[abarth-chromium, 2013-09-17 22:58:26]

If we're moving in concert with IE and Firefox, then I'm much less worried. 
Thanks for the additional information!

[brettw, 2013-09-18 19:39:00]

https://codereview.chromium.org/23642003/diff/35001/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/23642003/diff/35001/url/url_canon_icu.cc#newc...
url/url_canon_icu.cc:132: static UIDNA* uidna = NULL; // will be leaked.
Here you make a lazy lock to protect initialization of the uidna struct. Can't
you just make a uidna thing a lazy instance itself and save a step?

[jungshik at Google, 2013-09-18 21:50:13]

https://codereview.chromium.org/23642003/diff/35001/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/23642003/diff/35001/url/url_canon_icu.cc#newc...
url/url_canon_icu.cc:132: static UIDNA* uidna = NULL; // will be leaked.
On 2013/09/18 19:39:00, brettw wrote:
> Here you make a lazy lock to protect initialization of the uidna struct. Can't
> you just make a uidna thing a lazy instance itself and save a step?

I considered that, but gave it up because it appears that lazy_instance needs to
be created on a pre-allocated space (private_buf_) with placement new (in
Pointer()). The corresponding C++ class for UIDNA does not have a ctor but just
has a factory method so that it'd not help either. If I define a trait with
|New| that just ignores a supplied buffer, it'd not work as intended, would it?

[brettw, 2013-09-18 22:01:50]

If the object has weird requirements, generally I'd wrap in it a struct or class
that just allocates it in the constructor.

[jungshik at Google, 2013-09-19 16:08:55]

Forgot to note that the CL  was updated. 
Thanks, Brett, for the tip. Does this look better?

[jungshik at Google, 2013-09-19 16:09:40]

https://codereview.chromium.org/23642003/diff/47001/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/23642003/diff/47001/url/url_canon_icu.cc#newc...
url/url_canon_icu.cc:12: #include "base/synchronization/lock.h"
oops. I'll remove this line in both files.

[brettw, 2013-09-19 17:44:44]

https://codereview.chromium.org/23642003/diff/54001/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/23642003/diff/54001/url/url_canon_icu.cc#newc...
url/url_canon_icu.cc:152: DCHECK(uidna != NULL);
Under what cases might we get an error that would result in this being null? I
just want to make sure we don't actually need to handle the error and fall back
on something.

[jungshik at Google, 2013-09-19 19:16:16]

https://codereview.chromium.org/23642003/diff/54001/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/23642003/diff/54001/url/url_canon_icu.cc#newc...
url/url_canon_icu.cc:152: DCHECK(uidna != NULL);
On 2013/09/19 17:44:45, brettw wrote:
> Under what cases might we get an error that would result in this being null? I
> just want to make sure we don't actually need to handle the error and fall
back
> on something.

I don't think it'll fail unless we can't get access the ICU data file for an
unforseeable reason. Even if that happens,  uidna_nameToASCII (called below)
would bail out immediately with 'invalid argument error) and U_SUCCESS(err)
would be false and we'd return false.

[brettw, 2013-09-19 20:05:44]

Sounds good. url/* LGTM

[Ryan Sleevi, 2013-09-19 20:21:01]

While I'm interested to learn more about IDN, I don't know if I'm qualified to
review the net/ changes.

From looking at the SVN history, I see it's been primarily you and Peter working
on this (and initial.commit). Peter, would you feel comfortable enough looking
at the net/ changes?

We should probably find someone in net/ who can be familiar with IDNA in
general, since this affects other things as well. Or we can add a per-file
OWNERS if jshin & peter ARE the inhouse experts.

https://codereview.chromium.org/23642003/diff/54001/chrome/common/net/x509_ce...
File chrome/common/net/x509_certificate_model.cc (right):

https://codereview.chromium.org/23642003/diff/54001/chrome/common/net/x509_ce...
chrome/common/net/x509_certificate_model.cc:15: std::string ProcessIDN(const
std::string& input) {
We should remove this.

We never handle IDN for the purposes of certificate name matching (that is, we
do not punycode the commonname as being done here), so it's arguably wrong.

Considering the push is to move away from common names, we should be able to
drop this code entirely. I'm happy to remove it now if that will make this patch
easier for you.

[jungshik at Google, 2013-09-19 20:37:20]

On 2013/09/19 20:21:01, Ryan Sleevi wrote:
> While I'm interested to learn more about IDN, I don't know if I'm qualified to
> review the net/ changes.
> 
> From looking at the SVN history, I see it's been primarily you and Peter
working
> on this (and initial.commit). Peter, would you feel comfortable enough looking
> at the net/ changes?
> 
> We should probably find someone in net/ who can be familiar with IDNA in
> general, since this affects other things as well. Or we can add a per-file
> OWNERS if jshin & peter ARE the inhouse experts.

Before the initial check into this repository (a long time ago), Brett also
reviewed/touched the file multiple times although he just reviewed the url part
this time :-)

Anyway, I guess Peter, Darin or Brett would be good to review net/* changes.


> 
>
https://codereview.chromium.org/23642003/diff/54001/chrome/common/net/x509_ce...
> File chrome/common/net/x509_certificate_model.cc (right):
> 
>
https://codereview.chromium.org/23642003/diff/54001/chrome/common/net/x509_ce...
> chrome/common/net/x509_certificate_model.cc:15: std::string ProcessIDN(const
> std::string& input) {
> We should remove this.
> 
> We never handle IDN for the purposes of certificate name matching (that is, we
> do not punycode the commonname as being done here), so it's arguably wrong.
> 
> Considering the push is to move away from common names, we should be able to
> drop this code entirely. I'm happy to remove it now if that will make this
patch
> easier for you.

Thank you, Ryan. If ProcessIDN is going away anyway, it'd shorten my patch and
I'd be happy to see it go.

[Peter Kasting, 2013-09-19 20:55:36]

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
net/base/net_util.cc:401: struct uidna_wrapper {
Struct names should be CamelCase.

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
net/base/net_util.cc:404: // This is the option closest to what we had in the
past with IDNA 2003
Never write in a comment about what "used to" happen.  In the long run, no one
knows or cares about what "used to" happen or even knows when you're referring
to.

Instead, write a comment that merely explains why we're using this value now and
what it does for us.  I would also add a comment above the struct saying what
the point of the struct is; the broad picture of "what's going on" can go there,
and specific implementation details can go in the comment here.

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
net/base/net_util.cc:412: int32_t options = UIDNA_CHECK_BIDI;
Nit: Don't make a temp for this, just inline it.

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
net/base/net_util.cc:444: for (int output_length = 64; ; ) {
How about writing the loop this way:

    // Try to convert the IDN to Unicode.  Arbitrarily start with 64 characters
    // of extra space for the conversion.
    int output_length = 64;
    UErrorCode status = U_ZERO_ERROR;
    UIDNAInfo info = UIDNA_INFO_INITIALIZER;
    do {
      out->resize(original_length + output_length);
      // This returns the actual length required.  If this is more than 64
      // characters, |status| will be U_BUFFER_OVERFLOW_ERROR and we'll try the
      // conversion again, but with a buffer that's sufficiently large.
      output_length = uidna_labelToUnicode(
          uidna, comp, static_cast<int32_t>(comp_len), &(*out)[original_length],
          output_length, &info, &status);
    } while ((status == U_BUFFER_OVERFLOW_ERROR) && (info.errors == 0));
    if (U_SUCCESS(status) && (info.errors == 0)) {
      // Converted successfully.  Ensure the converted component can be safely
      // displayed to the user.
      out->resize(original_length + output_length);
      if (IsIDNComponentSafe(out->data() + original_length, output_length,
                             languages))
        return true;
    }
    // Something went wrong.  Revert to the original string.
    out->resize(original_length);

This seems slightly clearer than the for() version, with more comments, and a
small line-wrapping change for style guide compliance.

[jungshik at Google, 2013-09-20 21:33:41]

Peter, can you take another look?

Ryan, I dropped a ProcessIDN change from the CL. Are you gonna remove it and
change its callers ?

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
net/base/net_util.cc:401: struct uidna_wrapper {
On 2013/09/19 20:55:36, Peter Kasting wrote:
> Struct names should be CamelCase.

Done.

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
net/base/net_util.cc:404: // This is the option closest to what we had in the
past with IDNA 2003
On 2013/09/19 20:55:36, Peter Kasting wrote:
> Never write in a comment about what "used to" happen.  In the long run, no one
> knows or cares about what "used to" happen or even knows when you're referring
> to.
> 
> Instead, write a comment that merely explains why we're using this value now
and
> what it does for us.  I would also add a comment above the struct saying what
> the point of the struct is; the broad picture of "what's going on" can go
there,
> and specific implementation details can go in the comment here.

Done. I just kept TODO comment here and explained other things before struct.

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
net/base/net_util.cc:412: int32_t options = UIDNA_CHECK_BIDI;
On 2013/09/19 20:55:36, Peter Kasting wrote:
> Nit: Don't make a temp for this, just inline it.

Done.

https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
net/base/net_util.cc:444: for (int output_length = 64; ; ) {
On 2013/09/19 20:55:36, Peter Kasting wrote:
> How about writing the loop this way:
> 
>     // Try to convert the IDN to Unicode.  Arbitrarily start with 64
characters
>     // of extra space for the conversion.
>     int output_length = 64;
>     UErrorCode status = U_ZERO_ERROR;
>     UIDNAInfo info = UIDNA_INFO_INITIALIZER;
>     do {
>       out->resize(original_length + output_length);
>       // This returns the actual length required.  If this is more than 64
>       // characters, |status| will be U_BUFFER_OVERFLOW_ERROR and we'll try
the
>       // conversion again, but with a buffer that's sufficiently large.
>       output_length = uidna_labelToUnicode(
>           uidna, comp, static_cast<int32_t>(comp_len),
&(*out)[original_length],
>           output_length, &info, &status);
>     } while ((status == U_BUFFER_OVERFLOW_ERROR) && (info.errors == 0));
>     if (U_SUCCESS(status) && (info.errors == 0)) {
>       // Converted successfully.  Ensure the converted component can be safely
>       // displayed to the user.
>       out->resize(original_length + output_length);
>       if (IsIDNComponentSafe(out->data() + original_length, output_length,
>                              languages))
>         return true;
>     }
>     // Something went wrong.  Revert to the original string.
>     out->resize(original_length);
> 
> This seems slightly clearer than the for() version, with more comments, and a
> small line-wrapping change for style guide compliance.

Done with a slight change because UErrorCode has to be reset every time.

[Ryan Sleevi, 2013-09-20 22:10:16]

On 2013/09/20 21:33:41, Jungshik Shin wrote:
> Peter, can you take another look?
> 
> Ryan, I dropped a ProcessIDN change from the CL. Are you gonna remove it and
> change its callers ?

Hey Jungshik - it looks like there's still one case where ProcessIDN cannot be
removed and where its behaviour is correct -
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/third_party...

All other usages are incorrect or unnecessary, but won't allow removal of that,
although I suspect it should be refactored to use net/base/net_util's
IDNToUnicode, so that it matches the other elements of the UI that we use.

> 
> https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc
> File net/base/net_util.cc (right):
> 
>
https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
> net/base/net_util.cc:401: struct uidna_wrapper {
> On 2013/09/19 20:55:36, Peter Kasting wrote:
> > Struct names should be CamelCase.
> 
> Done.
> 
>
https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
> net/base/net_util.cc:404: // This is the option closest to what we had in the
> past with IDNA 2003
> On 2013/09/19 20:55:36, Peter Kasting wrote:
> > Never write in a comment about what "used to" happen.  In the long run, no
one
> > knows or cares about what "used to" happen or even knows when you're
referring
> > to.
> > 
> > Instead, write a comment that merely explains why we're using this value now
> and
> > what it does for us.  I would also add a comment above the struct saying
what
> > the point of the struct is; the broad picture of "what's going on" can go
> there,
> > and specific implementation details can go in the comment here.
> 
> Done. I just kept TODO comment here and explained other things before struct.
> 
>
https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
> net/base/net_util.cc:412: int32_t options = UIDNA_CHECK_BIDI;
> On 2013/09/19 20:55:36, Peter Kasting wrote:
> > Nit: Don't make a temp for this, just inline it.
> 
> Done.
> 
>
https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newc...
> net/base/net_util.cc:444: for (int output_length = 64; ; ) {
> On 2013/09/19 20:55:36, Peter Kasting wrote:
> > How about writing the loop this way:
> > 
> >     // Try to convert the IDN to Unicode.  Arbitrarily start with 64
> characters
> >     // of extra space for the conversion.
> >     int output_length = 64;
> >     UErrorCode status = U_ZERO_ERROR;
> >     UIDNAInfo info = UIDNA_INFO_INITIALIZER;
> >     do {
> >       out->resize(original_length + output_length);
> >       // This returns the actual length required.  If this is more than 64
> >       // characters, |status| will be U_BUFFER_OVERFLOW_ERROR and we'll try
> the
> >       // conversion again, but with a buffer that's sufficiently large.
> >       output_length = uidna_labelToUnicode(
> >           uidna, comp, static_cast<int32_t>(comp_len),
> &(*out)[original_length],
> >           output_length, &info, &status);
> >     } while ((status == U_BUFFER_OVERFLOW_ERROR) && (info.errors == 0));
> >     if (U_SUCCESS(status) && (info.errors == 0)) {
> >       // Converted successfully.  Ensure the converted component can be
safely
> >       // displayed to the user.
> >       out->resize(original_length + output_length);
> >       if (IsIDNComponentSafe(out->data() + original_length, output_length,
> >                              languages))
> >         return true;
> >     }
> >     // Something went wrong.  Revert to the original string.
> >     out->resize(original_length);
> > 
> > This seems slightly clearer than the for() version, with more comments, and
a
> > small line-wrapping change for style guide compliance.
> 
> Done with a slight change because UErrorCode has to be reset every time.

[jungshik at Google, 2013-09-21 00:37:46]

On 2013/09/20 22:10:16, Ryan Sleevi wrote:
> On 2013/09/20 21:33:41, Jungshik Shin wrote:
> > Peter, can you take another look?
> > 
> > Ryan, I dropped a ProcessIDN change from the CL. Are you gonna remove it and
> > change its callers ?
> 
> Hey Jungshik - it looks like there's still one case where ProcessIDN cannot be
> removed and where its behaviour is correct -
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/third_party...

Yeah, I've seen that, but forgot about it later. 
> 
> All other usages are incorrect or unnecessary, but won't allow removal of
that,
> although I suspect it should be refactored to use net/base/net_util's
> IDNToUnicode, so that it matches the other elements of the UI that we use.

Ok. That's a good idea. I refactored x509*'s ProcessIDN to uset net_util. 

You can fix the incorrect usage of ProcessIDN() at your schedule.

[Ryan Sleevi, 2013-09-21 00:46:13]

https://chromiumcodereview.appspot.com/23642003/diff/82001/chrome/common/net/...
File chrome/common/net/x509_certificate_model.cc (right):

https://chromiumcodereview.appspot.com/23642003/diff/82001/chrome/common/net/...
chrome/common/net/x509_certificate_model.cc:21: string16 output16 =
IDNToUnicode(input, std::string());
IWYU: include net/base/net_util.h on line 10/11

naming: this should be net::IDNToUnicode - x509_certificate_model isn't in the
//net namespace.

[jungshik at Google, 2013-09-23 17:54:53]

Thank you for looking at it. Updated.

https://codereview.chromium.org/23642003/diff/82001/chrome/common/net/x509_ce...
File chrome/common/net/x509_certificate_model.cc (right):

https://codereview.chromium.org/23642003/diff/82001/chrome/common/net/x509_ce...
chrome/common/net/x509_certificate_model.cc:21: string16 output16 =
IDNToUnicode(input, std::string());
On 2013/09/21 00:46:14, Ryan Sleevi wrote:
> IWYU: include net/base/net_util.h on line 10/11
> 
> naming: this should be net::IDNToUnicode - x509_certificate_model isn't in the
> //net namespace.

Done. 
Absolutely.  I realized that on the way home Friday. :-)

[Peter Kasting, 2013-09-23 21:12:43]

Code-wise, net/ LGTM, but I am not qualified to really determine whether this is
the right algorithm; I am just reviewing for style and the like.

The comments could still use some improvement; feedback below.

https://codereview.chromium.org/23642003/diff/88001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/23642003/diff/88001/net/base/net_util.cc#newc...
net/base/net_util.cc:402: // UTS46/IDNA 2008 handling object opened with
|uidna_openUTS46|.
Nit: Function names don't go inside pipes; instead say "uidna_openUTS46()".

You need an "a" before "UTS46/IDNA 2008".

https://codereview.chromium.org/23642003/diff/88001/net/base/net_util.cc#newc...
net/base/net_util.cc:405: // the backward compatibility in mind. What it does :
Migrate from what?  Backward compatibility with what?

We either need to not reference any old versions or behavior at all, and only
speak in reference to the current behavior, or we need to say clearly what
behavior we're trying to reference.

Nit: Don't put spaces before colons (2 places)

[jungshik at Google, 2013-09-24 17:47:55]

Updated per Peter's comments. 

Ryan, can you take another look?

[Ryan Sleevi, 2013-09-24 23:21:24]

I'm not familiar enough with the IDNA aspect to know whether it's correct, but
one questino about the translation mechanism below.

https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc#newc...
net/base/net_util.cc:467: output_length, &info, &status);
Why not use WriteInto here, rather than the resize approach?

Also, is it correct that you want to write into the offset of original_length? 
Why are you not writing it into out[0] ?

[Ryan Sleevi, 2013-09-24 23:21:27]

I'm not familiar enough with the IDNA aspect to know whether it's correct, but
one questino about the translation mechanism below.

[Peter Kasting, 2013-09-24 23:28:07]

https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc#newc...
net/base/net_util.cc:467: output_length, &info, &status);
On 2013/09/24 23:21:24, Ryan Sleevi wrote:
> Why not use WriteInto here, rather than the resize approach?

It's not so much "this versus WriteInto()" as "this versus always doing two
calls, the first passing NULL, so we can just resize once".  If the second
method is chosen, of course, WriteInto could be used to make it slightly more
compact.

I think the choice of methods depends on (a) how verbose the code is (writing
out the two calls explicitly might be fractionally longer; the difference is
probably not large), and (b) the desired performance characteristics (maybe when
the initial call is given sufficient length to start with, it's noticeably
faster than the two-call method, because the actual conversion takes much longer
than the string resizing?).

> Also, is it correct that you want to write into the offset of original_length?

> Why are you not writing it into out[0] ?

|out| represents the entire output string to this point.  This function is
supposed to append a properly converted component to it.

[Ryan Sleevi, 2013-09-24 23:53:56]

net/ LGTM.

https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc#newc...
net/base/net_util.cc:467: output_length, &info, &status);
On 2013/09/24 23:28:08, Peter Kasting wrote:
> On 2013/09/24 23:21:24, Ryan Sleevi wrote:
> > Why not use WriteInto here, rather than the resize approach?
> 
> It's not so much "this versus WriteInto()" as "this versus always doing two
> calls, the first passing NULL, so we can just resize once".  If the second
> method is chosen, of course, WriteInto could be used to make it slightly more
> compact.
> 
> I think the choice of methods depends on (a) how verbose the code is (writing
> out the two calls explicitly might be fractionally longer; the difference is
> probably not large), and (b) the desired performance characteristics (maybe
when
> the initial call is given sufficient length to start with, it's noticeably
> faster than the two-call method, because the actual conversion takes much
longer
> than the string resizing?).
> 
I guess the concern was the usual "space for NULL" case - and whenever seeing
such calls, always having to reason about whether or not there will or will not
be an embedded terminator. Blame WinAPI for that.

Realizing that this is the per-component case, I agree, WriteInto isn't needed.

> > Also, is it correct that you want to write into the offset of
original_length?
> 
> > Why are you not writing it into out[0] ?
> 
> |out| represents the entire output string to this point.  This function is
> supposed to append a properly converted component to it.

I should have read the function header. Mea culpa.

[jungshik at Google, 2013-09-25 00:37:39]

Thank you, Ryan, for reivew and Peter for answering his questions. 

I need chrome/* owner review.

[jungshik at Google, 2013-09-26 00:07:36]

Nico, would you approve for chrome/common?

[Nico, 2013-09-26 00:09:56]

lgtm

[commit-bot: I haz the power, 2013-09-26 01:11:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jshin@chromium.org/23642003/97001

[commit-bot: I haz the power, 2013-09-26 05:41:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jungshik at Google, 2013-09-26 20:02:35]

The component build failed due to the lack of an explicit dependency declaration
for dynamic_annotation. 

Brett, are you ok with a url.gyp change to add that ?

[commit-bot: I haz the power, 2013-09-27 07:05:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jshin@chromium.org/23642003/130001

[commit-bot: I haz the power, 2013-09-27 09:05:20]

Retried try job too often on linux_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-27 18:57:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jshin@chromium.org/23642003/130001

[commit-bot: I haz the power, 2013-09-27 22:57:10]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[jungshik at Google, 2013-09-28 00:09:51]

On 2013/09/27 22:57:10, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_aura for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

I'll land manually later tonigh. According to
http://build.chromium.org/p/tryserver.chromium/builders/linux_aura , the same
test has kept failing for the last couple of days for many other CLs (not just
mine).

[jungshik at Google, 2013-09-29 10:52:53]

Committed patchset #19 manually as r225878.