Consolidate most of net/'s icu_string_conversions.h dependencies.

Consolidate most of net/'s icu_string+conversions dependencies.

TBR=brettw@chromium.org
BUG=362608
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266957

[mmenke, 2014-04-28 19:01:37]

The Windows trybot failures are bogus.  Think the Android ones are, too.

https://codereview.chromium.org/252733002/diff/100001/net/DEPS
File net/DEPS (right):

https://codereview.chromium.org/252733002/diff/100001/net/DEPS#newcode50
net/DEPS:50: ],
Not sure if we need to do this one.  It also uses a slightly different behavior
on error (Add special character).  Can do that in Java, but not sure we want a
function with another behavior unless we need it.

[mef, 2014-04-28 19:19:50]

looks pretty good.

https://codereview.chromium.org/252733002/diff/100001/net/base/net_string_uti...
File net/base/net_string_util_icu.cc (right):

https://codereview.chromium.org/252733002/diff/100001/net/base/net_string_uti...
net/base/net_string_util_icu.cc:27: char* buf = WriteInto(output,
output_length);
could it return NULL?

https://codereview.chromium.org/252733002/diff/100001/net/http/http_auth_hand...
File net/http/http_auth_handler_digest.cc (right):

https://codereview.chromium.org/252733002/diff/100001/net/http/http_auth_hand...
net/http/http_auth_handler_digest.cc:230: if
(!net::ConvertLatin1ToUtf8AndNormalize(value, &realm))
Are we doing Latin1-specific function to avoid using base::kCodepageLatin1?

https://codereview.chromium.org/252733002/diff/100001/net/http/http_content_d...
File net/http/http_content_disposition.cc (left):

https://codereview.chromium.org/252733002/diff/100001/net/http/http_content_d...
net/http/http_content_disposition.cc:71: if (decoded.empty()) {
why is this not needed?

[mmenke, 2014-04-28 19:29:18]

https://codereview.chromium.org/252733002/diff/100001/net/base/net_string_uti...
File net/base/net_string_util_icu.cc (right):

https://codereview.chromium.org/252733002/diff/100001/net/base/net_string_uti...
net/base/net_string_util_icu.cc:27: char* buf = WriteInto(output,
output_length);
On 2014/04/28 19:19:51, mef wrote:
> could it return NULL?

No, see
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

https://codereview.chromium.org/252733002/diff/100001/net/http/http_auth_hand...
File net/http/http_auth_handler_digest.cc (right):

https://codereview.chromium.org/252733002/diff/100001/net/http/http_auth_hand...
net/http/http_auth_handler_digest.cc:230: if
(!net::ConvertLatin1ToUtf8AndNormalize(value, &realm))
On 2014/04/28 19:19:51, mef wrote:
> Are we doing Latin1-specific function to avoid using base::kCodepageLatin1?

Yes - it's declared in base/i18n, which is part of a project that depends on
icu, so can't use it without the icu dependency.  It's actually the same string
in Java as well, so we could make another net-specific version of it, but I
think that's ugly.

https://codereview.chromium.org/252733002/diff/100001/net/http/http_content_d...
File net/http/http_content_disposition.cc (left):

https://codereview.chromium.org/252733002/diff/100001/net/http/http_content_d...
net/http/http_content_disposition.cc:71: if (decoded.empty()) {
On 2014/04/28 19:19:51, mef wrote:
> why is this not needed?

An empty string should just end up being converted to another UTF8 string...but
you're right, shouldn't do that change in this CL, which should be a pure
refactoring.  I've put it in ConvertToUtf8.

[mmenke, 2014-04-28 19:31:37]

https://codereview.chromium.org/252733002/diff/100001/net/http/http_content_d...
File net/http/http_content_disposition.cc (left):

https://codereview.chromium.org/252733002/diff/100001/net/http/http_content_d...
net/http/http_content_disposition.cc:71: if (decoded.empty()) {
On 2014/04/28 19:29:18, mmenke wrote:
> On 2014/04/28 19:19:51, mef wrote:
> > why is this not needed?
> 
> An empty string should just end up being converted to another UTF8
string...but
> you're right, shouldn't do that change in this CL, which should be a pure
> refactoring.  I've put it in ConvertToUtf8.

Oh...I guess there's the invalid charset case, with an empty string.

[mef, 2014-04-28 20:27:41]

lgtm

https://codereview.chromium.org/252733002/diff/140001/net/http/http_content_d...
File net/http/http_content_disposition.cc (right):

https://codereview.chromium.org/252733002/diff/140001/net/http/http_content_d...
net/http/http_content_disposition.cc:69: }
is it better with braces?

[mmenke, 2014-04-28 20:45:03]

Thanks for the feedback!

https://codereview.chromium.org/252733002/diff/140001/net/http/http_content_d...
File net/http/http_content_disposition.cc (right):

https://codereview.chromium.org/252733002/diff/140001/net/http/http_content_d...
net/http/http_content_disposition.cc:69: }
On 2014/04/28 20:27:42, mef wrote:
> is it better with braces?

My understanding (From reading a long thread on chromium-dev from quite a while
back) is that the consensus style is to use braces when either the if-body or
if-antecedent takes more than one line.

[mmenke, 2014-04-28 20:45:21]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:46:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/252733002/140001

[commit-bot: I haz the power, 2014-04-28 21:30:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 21:30:31]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[mmenke, 2014-04-29 14:49:42]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-04-29 14:50:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/252733002/140001

[commit-bot: I haz the power, 2014-04-29 20:58:14]

Change committed as 266957