Adding some more escaping method....

Adding some more escaping method.
This will be used by the translate feature.

BUG=None
TEST=Run the unit-tests.


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

[Chris Evans, 2010-01-20 20:08:52]

LGTM conceptually.
Feel free to address the performance question or not, based on context.

http://codereview.chromium.org/548088/diff/5/1005
File net/base/escape.cc (right):

http://codereview.chromium.org/548088/diff/5/1005#newcode356
net/base/escape.cc:356: string16 text(input);
(I don't have context for how this is likely to be called and whether it's on a
hot path etc. so please forgive these efficiency comments upfront :)

If "nothing to unescape" is a common case, there is an API efficiency issue
because a copy is always taken.

http://codereview.chromium.org/548088/diff/5/1005#newcode361
net/base/escape.cc:361: string16 kAmpersandChar =
WideToUTF16(kEscapeToChars[i].ampersand_code);
Can these string16 objects be precomputed, if speed is desired?
Minor style nit: I'm not sure the value is "constant enough" to warrant a name
starting with a 'k'.

http://codereview.chromium.org/548088/diff/5/1005#newcode362
net/base/escape.cc:362: int index = iter - text.begin();
Can this index be calculated outside the loop?

http://codereview.chromium.org/548088/diff/5/1006
File net/base/escape_unittest.cc (right):

http://codereview.chromium.org/548088/diff/5/1006#newcode199
net/base/escape_unittest.cc:199: TEST(EscapeTest, UnescapeURLComponent) {
There was a logic code change to pass through any characters with value >= 0x80.
Can you add a test for that?

http://codereview.chromium.org/548088/diff/5/1006#newcode357
net/base/escape_unittest.cc:357: TEST(EscapeTest, UnescapeForHTML) {
Can you test a string that ends in & to make sure there isn't some array out of
bounds condition? (Code looks fine, but a test is always useful).

[jcampan, 2010-01-20 23:41:54]

Also contains some changes to address so Unix compile failures.

http://codereview.chromium.org/548088/diff/5/1005
File net/base/escape.cc (right):

http://codereview.chromium.org/548088/diff/5/1005#newcode356
net/base/escape.cc:356: string16 text(input);
On 2010/01/20 20:08:52, Chris Evans wrote:
> (I don't have context for how this is likely to be called and whether it's on
a
> hot path etc. so please forgive these efficiency comments upfront :)
> 
I guess without changing the function signature I am not going to be able to
avoid the return copy.
Do you think doing the find here and return input if there are no & in the
string is worth it?

> If "nothing to unescape" is a common case, there is an API efficiency issue
> because a copy is always taken.

http://codereview.chromium.org/548088/diff/5/1005#newcode361
net/base/escape.cc:361: string16 kAmpersandChar =
WideToUTF16(kEscapeToChars[i].ampersand_code);
On 2010/01/20 20:08:52, Chris Evans wrote:
> Can these string16 objects be precomputed, if speed is desired?
> Minor style nit: I'm not sure the value is "constant enough" to warrant a name
> starting with a 'k'.
Reusing them now.

http://codereview.chromium.org/548088/diff/5/1005#newcode362
net/base/escape.cc:362: int index = iter - text.begin();
On 2010/01/20 20:08:52, Chris Evans wrote:
> Can this index be calculated outside the loop?

Done.

http://codereview.chromium.org/548088/diff/5/1006
File net/base/escape_unittest.cc (right):

http://codereview.chromium.org/548088/diff/5/1006#newcode199
net/base/escape_unittest.cc:199: TEST(EscapeTest, UnescapeURLComponent) {
On 2010/01/20 20:08:52, Chris Evans wrote:
> There was a logic code change to pass through any characters with value >=
0x80.
> Can you add a test for that?

Done.

http://codereview.chromium.org/548088/diff/5/1006#newcode357
net/base/escape_unittest.cc:357: TEST(EscapeTest, UnescapeForHTML) {
On 2010/01/20 20:08:52, Chris Evans wrote:
> Can you test a string that ends in & to make sure there isn't some array out
of
> bounds condition? (Code looks fine, but a test is always useful).

Done.

[cevans, 2010-01-21 01:20:51]

LGTM

I don't have the calling context for this new function so I don't know if
it's on a hot path or not.... for now we can just note this as a future
possible optimization.

On Wed, Jan 20, 2010 at 3:41 PM, <jcampan@chromium.org> wrote:

> Also contains some changes to address so Unix compile failures.
>
>
>
> http://codereview.chromium.org/548088/diff/5/1005
> File net/base/escape.cc (right):
>
> http://codereview.chromium.org/548088/diff/5/1005#newcode356
> net/base/escape.cc:356: string16 text(input);
> On 2010/01/20 20:08:52, Chris Evans wrote:
>
>> (I don't have context for how this is likely to be called and whether
>>
> it's on a
>
>> hot path etc. so please forgive these efficiency comments upfront :)
>>
>
> I guess without changing the function signature I am not going to be
> able to avoid the return copy.
> Do you think doing the find here and return input if there are no & in
> the string is worth it?
>
>
>  If "nothing to unescape" is a common case, there is an API efficiency
>>
> issue
>
>> because a copy is always taken.
>>
>
> http://codereview.chromium.org/548088/diff/5/1005#newcode361
> net/base/escape.cc:361: string16 kAmpersandChar =
> WideToUTF16(kEscapeToChars[i].ampersand_code);
> On 2010/01/20 20:08:52, Chris Evans wrote:
>
>> Can these string16 objects be precomputed, if speed is desired?
>> Minor style nit: I'm not sure the value is "constant enough" to
>>
> warrant a name
>
>> starting with a 'k'.
>>
> Reusing them now.
>
>
> http://codereview.chromium.org/548088/diff/5/1005#newcode362
> net/base/escape.cc:362: int index = iter - text.begin();
> On 2010/01/20 20:08:52, Chris Evans wrote:
>
>> Can this index be calculated outside the loop?
>>
>
> Done.
>
>
> http://codereview.chromium.org/548088/diff/5/1006
> File net/base/escape_unittest.cc (right):
>
> http://codereview.chromium.org/548088/diff/5/1006#newcode199
> net/base/escape_unittest.cc:199: TEST(EscapeTest, UnescapeURLComponent)
> {
> On 2010/01/20 20:08:52, Chris Evans wrote:
>
>> There was a logic code change to pass through any characters with
>>
> value >= 0x80.
>
>> Can you add a test for that?
>>
>
> Done.
>
>
> http://codereview.chromium.org/548088/diff/5/1006#newcode357
> net/base/escape_unittest.cc:357: TEST(EscapeTest, UnescapeForHTML) {
> On 2010/01/20 20:08:52, Chris Evans wrote:
>
>> Can you test a string that ends in & to make sure there isn't some
>>
> array out of
>
>> bounds condition? (Code looks fine, but a test is always useful).
>>
>
> Done.
>
>
> http://codereview.chromium.org/548088
>