Eliminate wstring from base/utf_offset_string_conversions.h, net/base/escape.h, and net/base/net_...

Eliminate wstring from base/utf_offset_string_conversions.h, net/base/escape.h, and net/base/net_util.h, and reduce the API surfaces in various places slightly where possible.

BUG=23581
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83948

[brettw, 2011-04-25 16:19:43]

LGTM

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc#newcode1566
net/base/net_util.cc:1566: url_parse::Parsed parsed_temp;
I'm assuming you just moved this code and changed string types here. Let me know
if you changed anything non-trivial.

[Peter Kasting, 2011-04-25 17:44:52]

I'll commit this once I figure out the test failures.

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc#newcode1566
net/base/net_util.cc:1566: url_parse::Parsed parsed_temp;
On 2011/04/25 16:19:44, brettw wrote:
> I'm assuming you just moved this code and changed string types here. Let me
know
> if you changed anything non-trivial.

The only non-trivial change was to change kHTTP from "const wchar_t[]" to
"string16" because I don't know of any way to initialize a "const char16[]".

[Avi (use Gerrit), 2011-04-25 17:52:06]

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc#newcode1614
net/base/net_util.cc:1614: string16 kHTTP = ASCIIToUTF16("http://");
eww. const char like kFTP below.

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc#newcode1624
net/base/net_util.cc:1624: (url.host().compare(0, arraysize(kFTP) - 1, kFTP) !=
0);
Can you use string_util's LowerCaseEqualsASCII?

[brettw, 2011-04-25 17:56:28]

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6898026/diff/28/net/base/net_util.cc#newcode1624
net/base/net_util.cc:1624: (url.host().compare(0, arraysize(kFTP) - 1, kFTP) !=
0);
The host name will be canonicalized so this isn't necessary.

[Peter Kasting, 2011-04-26 18:41:55]

Fixing the test failures exposed a number of problems.  This version of the
patch does the following versus the last one:

* Simplifies net_util.h APIs slightly
* Makes net_util unittests shorter, more verbose, and more comprehensive (e.g.
new cases tested like usernames/passwords with escapes and updating the offset
of the ':'/'@' auth separators)
* Fixes bug where offsets of ':'/'@' auth separators were not updated correctly
* Rewrites offset handling in net_util.  The old code updated the offset vector
in-place, which only happens to work because all current transforms shorten the
string; it also assumed things about how the destination string related to the
source string, such as that it would contain all the source components before
the current one.  The new code tries to be clearer and more general by tracking
three offset vectors: an immutable copy of the original offsets; a set of
offsets specific to the current component, relative to the start of the
component; and a destination vector in which to write the final, adjusted
offsets relative to the destination string.

Mike, I've added you as a reviewer since you wrote much of the offset handling
recently; I'd like you and Brett to both take a look at net_util.cc.

Avi, you will be pleased to note that I was able to convert |kHTTP| to a char[].

[Peter Kasting, 2011-04-26 18:42:49]

On 2011/04/26 18:41:55, Peter Kasting wrote:
> I'd like you and Brett to both take a look at net_util.cc.

Oh, and by this, I mean you might want to look at the diff between patch set 2
and patch set 3.  Looking at the overall diff will be less clear.

[mrossetti, 2011-04-26 22:13:33]

LGTM

Stylistics comments only. I only reviewed utf_offset_string_conversions*.* and
net_util*.*.

http://codereview.chromium.org/6898026/diff/10001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6898026/diff/10001/net/base/net_util.cc#newcod...
net/base/net_util.cc:661: // Functions may stack-allocate one of these in order
to clamp the offsets in
I'm not sure I see the point of making this an automatic. It has no state and
does not make a copy of either input — it just clamps. It would seem to be more
obvious to the future user what this class was doing if it was a
function/functor instead.

http://codereview.chromium.org/6898026/diff/10001/net/base/net_util.cc#newcod...
net/base/net_util.cc:704: OffsetLimiter offset_limiter(offsets_for_adjustment,
&out16);
Continuing from the previous comment: This auto could just as easily be placed
just before the final return and have the same affect.

http://codereview.chromium.org/6898026/diff/10001/net/base/net_util.cc#newcod...
net/base/net_util.cc:1592: AdjustOffset::Adjustments adjustments;
The AdjustOffset::Adjustments concept would be a good candidate for making an
automatic, eliminating the need for the concluding for_each.

[Peter Kasting, 2011-04-27 02:07:19]

New snap up.

http://codereview.chromium.org/6898026/diff/10001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6898026/diff/10001/net/base/net_util.cc#newcod...
net/base/net_util.cc:661: // Functions may stack-allocate one of these in order
to clamp the offsets in
On 2011/04/26 22:13:34, mrossetti wrote:
> I'm not sure I see the point of making this an automatic. It has no state and
> does not make a copy of either input — it just clamps. It would seem to be
more
> obvious to the future user what this class was doing if it was a
> function/functor instead.

Good point.  Originally I wanted an object to guard multiple exit points.  But
since none of the existing code needs that, it's better to write the simple way
for now.  Changed.

http://codereview.chromium.org/6898026/diff/10001/net/base/net_util.cc#newcod...
net/base/net_util.cc:1592: AdjustOffset::Adjustments adjustments;
On 2011/04/26 22:13:34, mrossetti wrote:
> The AdjustOffset::Adjustments concept would be a good candidate for making an
> automatic, eliminating the need for the concluding for_each.

Good idea.  Rewrote this object.

[mrossetti, 2011-04-27 02:15:01]

LGTM

Sweet!

[Peter Kasting, 2011-04-27 15:16:51]

Brett, are you OK with the changes since patch set 2?

[brettw, 2011-04-27 17:47:51]

LGTM, sorry, I didn't realize you needed another pass.

http://codereview.chromium.org/6898026/diff/11033/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6898026/diff/11033/net/base/net_util.cc#newcod...
net/base/net_util.cc:724: // Transforms |original_offsets| by subtracting
|section_begin| from all
section_begin -> component_begin

http://codereview.chromium.org/6898026/diff/11033/net/base/net_util_unittest.cc
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/6898026/diff/11033/net/base/net_util_unittest....
net/base/net_util_unittest.cc:1842: kFormatUrlOmitUsernamePassword,
UnescapeRule::NORMAL, omit_auth_cases_2,
Indent 2 more spaces.

[willchan no longer on Chromium, 2011-05-03 20:02:17]

LGTM for net/