Update code that previously constructed strings from string iterators only to...

Update code that previously constructed strings from string iterators only to use StringToInt. These usages now pass the iterators directly to the new StringToInt overloads.

BUG=None
TEST=All

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

[erikwright (departed), 2010-10-21 15:10:40]

Hi Mark,

I feel guilty foisting this one on you too, but it does probably make sense.

Cheers,

Erik

[Mark Mentovai, 2010-10-21 16:09:29]

Fix the patch up so that it compiles on all of the try platforms, and then I’ll
take a look.

[erikwright (departed), 2010-10-21 20:11:40]

On 2010/10/21 16:09:29, Mark Mentovai wrote:
> Fix the patch up so that it compiles on all of the try platforms, and then
I’ll
> take a look.

All good now. The previous Win fails are unrelated. A green Win build is a
nice-to-have.

[Mark Mentovai, 2010-10-21 20:34:26]

LGTM

http://codereview.chromium.org/3968001/diff/49001/44003
File chrome/browser/autocomplete_history_manager.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44003#newcode65
chrome/browser/autocomplete_history_manager.cc:65: number_string.begin() + 9,
The old code would succeed (but with a questionable result) if number_string’s
size was >= 6 but < 9. This will give a questionable (and maybe worse) result.
Why isn’t there a check on number_string’s length?

http://codereview.chromium.org/3968001/diff/49001/44006
File chrome/browser/safe_browsing/filter_false_positive_perftest.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44006#newcode264
chrome/browser/safe_browsing/filter_false_positive_perftest.cc:264:
base::StringToInt(url.begin() + pos + 1, url.end(), &weight);
Can you just verify that your functions behave correctly if url.begin() + pos +
1 == url.end()? I don’t recall (although I think you were careful.)

http://codereview.chromium.org/3968001/diff/49001/44008
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44008#newcode67
chrome/common/extensions/extension.cc:67: if (base::HexStringToInt(id->begin() +
i, id->begin() + i + 1, &val))
This function is so awkward. Why does it even use HexStringToInt?

http://codereview.chromium.org/3968001/diff/49001/44009
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44009#newcode617
chrome/renderer/webplugin_delegate_pepper.cc:617:
base::HexStringToInt(hex_md5.begin() + 24, hex_md5.end(), &int_val);
Length check?

[erikwright (departed), 2010-10-22 13:50:22]

Thanks!

Running a final try now before committing.

http://codereview.chromium.org/3968001/diff/49001/44003
File chrome/browser/autocomplete_history_manager.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44003#newcode65
chrome/browser/autocomplete_history_manager.cc:65: number_string.begin() + 9,
On 2010/10/21 20:34:26, Mark Mentovai wrote:
> The old code would succeed (but with a questionable result) if number_string’s
> size was >= 6 but < 9. This will give a questionable (and maybe worse) result.
> Why isn’t there a check on number_string’s length?

There actually is a check up above (line 34). But that's a bit removed. I've
modified the code to bring it closer to the iterator manipulation.

http://codereview.chromium.org/3968001/diff/49001/44006
File chrome/browser/safe_browsing/filter_false_positive_perftest.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44006#newcode264
chrome/browser/safe_browsing/filter_false_positive_perftest.cc:264:
base::StringToInt(url.begin() + pos + 1, url.end(), &weight);
On 2010/10/21 20:34:26, Mark Mentovai wrote:
> Can you just verify that your functions behave correctly if url.begin() + pos
+
> 1 == url.end()? I don’t recall (although I think you were careful.)

Yep. There's a check for empty string (where begin == end) in the unittest.

http://codereview.chromium.org/3968001/diff/49001/44008
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44008#newcode67
chrome/common/extensions/extension.cc:67: if (base::HexStringToInt(id->begin() +
i, id->begin() + i + 1, &val))
On 2010/10/21 20:34:26, Mark Mentovai wrote:
> This function is so awkward. Why does it even use HexStringToInt?

Agree :(

http://codereview.chromium.org/3968001/diff/49001/44009
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44009#newcode617
chrome/renderer/webplugin_delegate_pepper.cc:617:
base::HexStringToInt(hex_md5.begin() + 24, hex_md5.end(), &int_val);
On 2010/10/21 20:34:26, Mark Mentovai wrote:
> Length check?

Length of an MD5 digest is guaranteed to be 32 chars, but it doesn't hurt to add
a DCHECK for documentation purposes, if nothing else.

http://codereview.chromium.org/3968001/diff/62001/63007
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/3968001/diff/62001/63007#newcode257
chrome/common/extensions/extension.cc:257: location(INVALID),
Unrelated change in Trunk.

http://codereview.chromium.org/3968001/diff/62001/63009
File chrome/test/page_cycler/page_cycler_test.cc (right):

http://codereview.chromium.org/3968001/diff/62001/63009#newcode151
chrome/test/page_cycler/page_cycler_test.cc:151: VLOG(1) << "Buffer cache should
be primed with " << loaded << " files.";
Unrelated change in Trunk.

[Erik does not do reviews, 2010-10-22 15:38:44]

http://codereview.chromium.org/3968001/diff/49001/44008
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44008#newcode67
chrome/common/extensions/extension.cc:67: if (base::HexStringToInt(id->begin() +
i, id->begin() + i + 1, &val))
On 2010/10/21 20:34:26, Mark Mentovai wrote:
> This function is so awkward. Why does it even use HexStringToInt?

Do you have an alternative suggestion?  Read the comment above for why it needs
to do what it's doing.

[erikwright (departed), 2010-10-22 15:48:44]

http://codereview.chromium.org/3968001/diff/49001/44008
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/3968001/diff/49001/44008#newcode67
chrome/common/extensions/extension.cc:67: if (base::HexStringToInt(id->begin() +
i, id->begin() + i + 1, &val))
On 2010/10/22 15:38:45, Erik Kay wrote:
> On 2010/10/21 20:34:26, Mark Mentovai wrote:
> > This function is so awkward. Why does it even use HexStringToInt?
> 
> Do you have an alternative suggestion?  Read the comment above for why it
needs
> to do what it's doing.

Two options:

1) Expose some of the digit interpretation stuff from string_number_conversions
so that you can convert char-by-char without having to pretend it's a string.

2) Something like:

for char in string:
  if '0' <= char <= '9':
    char += 'a' - '0'
  else if 'a' <= char <= 'f':
    char += 'g' - 'a'
  else if 'A' <= char <= 'F':
    char += 'g' - 'A'
  else:
    # freak out

[Mark Mentovai, 2010-10-22 15:55:50]

Erik Kay wrote:
> Do you have an alternative suggestion?  Read the comment above for why
> it needs to do what it's doing.

Yup, I saw the comment. There’s no reason to round-trip from
char-to-int-to-char for the conversion. You can walk through the
entire string one char at a time, remapping chars as needed.

[Erik does not do reviews, 2010-10-22 15:58:09]

On 2010/10/22 15:48:44, erikwright wrote:
> http://codereview.chromium.org/3968001/diff/49001/44008
> File chrome/common/extensions/extension.cc (right):
> 
> http://codereview.chromium.org/3968001/diff/49001/44008#newcode67
> chrome/common/extensions/extension.cc:67: if (base::HexStringToInt(id->begin()
+
> i, id->begin() + i + 1, &val))
> On 2010/10/22 15:38:45, Erik Kay wrote:
> > On 2010/10/21 20:34:26, Mark Mentovai wrote:
> > > This function is so awkward. Why does it even use HexStringToInt?
> > 
> > Do you have an alternative suggestion?  Read the comment above for why it
> needs
> > to do what it's doing.
> 
> Two options:
> 
> 1) Expose some of the digit interpretation stuff from
string_number_conversions
> so that you can convert char-by-char without having to pretend it's a string.
> 
> 2) Something like:
> 
> for char in string:
>   if '0' <= char <= '9':
>     char += 'a' - '0'
>   else if 'a' <= char <= 'f':
>     char += 'g' - 'a'
>   else if 'A' <= char <= 'F':
>     char += 'g' - 'A'
>   else:
>     # freak out

What's the benefit?  It looks like you've just reimplemented hex string to int
inline, which seems bad (DNRY).  Is the objection that this is just converting a
character while HexStringToInt is designed to operate on multi-character
strings?  In that case, it seems like we should expose CharToDigit rather than
reimplementing it inline.

[Mark Mentovai, 2010-10-22 16:38:24]

Using Hex*String*ToInt on a hexadecimal character at a time is too
heavyweight for what you need. What the code in question is doing
isn’t really converting anything to integers at all. It’s a simple
alphabet translation.

[Erik does not do reviews, 2010-10-22 16:58:05]

On 2010/10/22 16:38:24, Mark Mentovai wrote:
> Using Hex*String*ToInt on a hexadecimal character at a time is too
> heavyweight for what you need. What the code in question is doing
> isn’t really converting anything to integers at all. It’s a simple
> alphabet translation.

*shrug* Since our new alphabet is contiguous, I guess I'd argue that converting
hex to int is equivalent (Erik's suggested code block is almost identical to
BaseCharToDigit), so why create a new function?  It's a tiny amount of code, so
I don't have a huge objection.  I'm really not trying to make a big deal out of
this.  :-) I was just curious where you guys were coming from.