Standardize StringToInt{,64} interface.

Standardize StringToInt{,64} interface.

These changes address issue #106655.  All variants of StringToInt have been
converted to use the StringPiece class.  One instance of conversion, in
chrome/browser/history/text_database.cc, required copying an underlying
string.  This is because the string type in question could use 8 or 16
bit characters depending on the OS type, and because StringPiece is not
implemented as a template, the code cannot specify whether to create a
StringPiece or StringPiece16.  This should be remedied in a future CL.

R=erikwright@chromium.org
BUG=106655
TEST=


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

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

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

[erikwright (departed), 2011-12-12 18:49:51]

Brett:

I'd like your opinion on this (in particular, string_piece.h).

Firstly, I think an iterator range constructor is required for many cases, so
the question posed by this CL is whether to also declare a (string,
begin-offset, end-offset) constructor.

In many call-sites it is more compact, but I'm concerned that it is a bit
ambiguous because it's not clear if the second number is 'end' or 'length'.

Let me know what you think.

Thanks,

Erik

[brettw, 2011-12-12 18:58:59]

Yeah, I think we should go with the iterator constructor only which more closely
matches STL.

[Ted Vessenes, 2011-12-12 21:52:13]

Just to clarify, we're talking about iterator range constructors for
StringPiece, not iterator invocations for StringToInt?  (When you have
iterators, you just create a StringPiece() and pass it to StringToInt.)

Given that std::string and string16 can already be constructed from iterators
and StringPiece/16 can be constructed from these objects, is there any reason to
support an iterator constructor interface for StringPiece()?  In theory you
could just write this:

StringToInt(StringPiece(std::string(source.begin() + 3, source.begin() + 5)),
&value);

Though this C++ code is beginning to approach the look of Lisp.

Also, instantiating a std::string() through iterator constructors will generate
a new copy of the string.  But even if StringPiece() were to have a direct
iterator constructor interface, it would need to do the same UNLESS we were
comfortable recasting the begin iterator into the source data pointer and
storing (end-begin) as the length.  I can do that, but I'd like to confirm
that's what you want.

[erikwright (departed), 2011-12-12 21:57:42]

That's the intention. See http://codereview.chromium.org/8846001/ .
On Dec 12, 2011 4:52 PM, <tedvessenes@gmail.com> wrote:

> Just to clarify, we're talking about iterator range constructors for
> StringPiece, not iterator invocations for StringToInt?  (When you have
> iterators, you just create a StringPiece() and pass it to StringToInt.)
>
> Given that std::string and string16 can already be constructed from
> iterators
> and StringPiece/16 can be constructed from these objects, is there any
> reason to
> support an iterator constructor interface for StringPiece()?  In theory you
> could just write this:
>
> StringToInt(StringPiece(std::**string(source.begin() + 3, source.begin()
> + 5)),
> &value);
>
> Though this C++ code is beginning to approach the look of Lisp.
>
> Also, instantiating a std::string() through iterator constructors will
> generate
> a new copy of the string.  But even if StringPiece() were to have a direct
> iterator constructor interface, it would need to do the same UNLESS we were
> comfortable recasting the begin iterator into the source data pointer and
> storing (end-begin) as the length.  I can do that, but I'd like to confirm
> that's what you want.
>
>
http://codereview.chromium.**org/8921006/<http://codereview.chromium.org/8921...
>

[Ted Vessenes, 2011-12-13 17:21:24]

Alright, I updated the interfaces accordingly.  Using iterators turned out to be
surprisingly less ugly than I had anticipated, and I agree it's much safer than
the StringPiece(string, start_index, end_index_or_length_it_is_a_mystery)
interface.

[erikwright (departed), 2011-12-13 20:20:45]

Looking good.

1) A few things missed in your tidying up.
2) Would be nice to do the HexStringToInt functions too, but could be a separate
CL if you want (otherwise I'll do it in the new year).
3) I suggested a slight variation on the changes in string_number_conversions.cc
- I don't know if it turns out more compact or not.

http://codereview.chromium.org/8921006/diff/5001/base/string_number_conversio...
File base/string_number_conversions.cc (right):

http://codereview.chromium.org/8921006/diff/5001/base/string_number_conversio...
base/string_number_conversions.cc:296: IteratorRangeToIntTraits;
How about:

namespace {

template <typename STRING, typename VALUE, int BASE>
class BasicStringPieceToNumberTraits
    : public
BaseIteratorRangeToNumberTraits<base::BasicStringPiece<STRING>::const_iterator,
VALUE, BASE> {}

template <typename STRING, typename VALUE>
bool StringToIntImpl(const BasicStringPiece<STRING>& input, VALUE* output) {
  return IteratorRangeToNumber<BasicStringPieceToNumberTraits<STRING,VALUE, 10>
>::Invoke(
      input.begin(), input.end(), output);
}

}  // namespace

bool StringToInt(const StringPiece& input, int* output) {
  return StringToIntImpl(input, output);
}

bool StringToInt(const StringPiece16& input, int* output) {
  return StringToIntImpl(input, output);
}

bool StringToInt64(const StringPiece& input, int64* output) {
  return StringToIntImpl(input, output);
}

bool StringToInt(const StringPiece16& input, int64* output) {
  return StringToIntImpl(input, output);
}

http://codereview.chromium.org/8921006/diff/5001/base/string_number_conversio...
base/string_number_conversions.cc:458: bool HexStringToInt(const std::string&
input, int* output) {
It would be a bonus to also convert these functions.

http://codereview.chromium.org/8921006/diff/5001/chrome/browser/autocomplete_...
File chrome/browser/autocomplete_history_manager.cc (right):

http://codereview.chromium.org/8921006/diff/5001/chrome/browser/autocomplete_...
chrome/browser/autocomplete_history_manager.cc:68: number_string.begin() + 3),
&area))
I think area needs to go on the next line (aligned with the S in StringPiece16).

http://codereview.chromium.org/8921006/diff/5001/chrome/browser/autocomplete_...
chrome/browser/autocomplete_history_manager.cc:77: number_string.begin() + 5),
&group)
Wrapping as above.

http://codereview.chromium.org/8921006/diff/5001/chrome/browser/autocomplete_...
chrome/browser/autocomplete_history_manager.cc:83: number_string.begin() + 9),
&serial)
Wrapping as above.

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

http://codereview.chromium.org/8921006/diff/5001/net/base/net_util.cc#newcode...
net/base/net_util.cc:1990: allowed_ports.begin() + i), &port);
Wrapping.

http://codereview.chromium.org/8921006/diff/5001/net/http/http_response_heade...
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/8921006/diff/5001/net/http/http_response_heade...
net/http/http_response_headers.cc:703: base::StringToInt(StringPiece(&(*code),
(int)(p - code)),
Can't this be StringPiece(code, p), ...?

http://codereview.chromium.org/8921006/diff/5001/net/http/http_response_heade...
net/http/http_response_headers.cc:1256: bool ok =
base::StringToInt64(std::string(first_byte_pos_begin,
StringToInt64(StringPiece(first_byte_pos_begin, first_byte_pos_end), ...

http://codereview.chromium.org/8921006/diff/5001/net/http/http_response_heade...
net/http/http_response_headers.cc:1268: last_byte_pos_end),
Similar.

http://codereview.chromium.org/8921006/diff/5001/net/http/http_response_heade...
net/http/http_response_headers.cc:1293: instance_length_end),
Similar.

[Ted Vessenes, 2011-12-14 03:34:59]

I updated the formatting per request, and also converted HexStringToInt().  I
also noticed a HexStringToBytes() function which could be converted to use
StringPiece() as well, but didn't do so.

I wasn't able to implement the full template syntax you suggested, since
BasicStringPiece<> isn't defined in the master branch yet.  However, I create
two templates (one for StringPiece and the other for StringPiece16), which can
be merged into one when that CL is committed.  I couldn't find anything in the
style guide about how to break up long lines containing only templated function
definitions, but I took a guess.

[erikwright (departed), 2011-12-14 04:24:08]

It's looking really great. A few more style comments here.

As you might have noticed, my templatization CL is bogged down in a Clang bug.
I've been asked to wait for that to be patched, ETA a week or so.

But this CL can be committed without that.

Have you signed the contributor agreement yet?

Once you respond to the most recent set of comments I'll find appropriate
reviewers with OWNERS privileges. Feel free to email me if you are looking for
further opportunities to contribute to Chromium.

Thanks,

Erik

http://codereview.chromium.org/8921006/diff/12001/chrome/browser/component_up...
File chrome/browser/component_updater/component_updater_service.cc (right):

http://codereview.chromium.org/8921006/diff/12001/chrome/browser/component_up...
chrome/browser/component_updater/component_updater_service.cc:80: &val))
need {} around if and else blocks when condition wraps.

http://codereview.chromium.org/8921006/diff/12001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/8921006/diff/12001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:85: &val))
need {} around if and else blocks when condition wraps.

http://codereview.chromium.org/8921006/diff/12001/net/ftp/ftp_util.cc
File net/ftp/ftp_util.cc (right):

http://codereview.chromium.org/8921006/diff/12001/net/ftp/ftp_util.cc#newcode218
net/ftp/ftp_util.cc:218: &time_exploded.hour))
need {} around if and else blocks when condition wraps.

http://codereview.chromium.org/8921006/diff/12001/net/ftp/ftp_util.cc#newcode221
net/ftp/ftp_util.cc:221: if (!base::StringToInt(StringPiece16(rest.begin() + 3,
rest.begin() + 5),
need {} around if and else blocks when condition wraps.

http://codereview.chromium.org/8921006/diff/12001/net/ftp/ftp_util.cc#newcode226
net/ftp/ftp_util.cc:226: if (!base::StringToInt(StringPiece16(rest.begin(),
rest.begin() + 1),
need {} around if and else blocks when condition wraps.

http://codereview.chromium.org/8921006/diff/12001/net/ftp/ftp_util.cc#newcode230
net/ftp/ftp_util.cc:230: if (!base::StringToInt(StringPiece16(rest.begin() + 2,
rest.begin() + 4),
need {} around if and else blocks when condition wraps.

http://codereview.chromium.org/8921006/diff/12001/net/http/http_chunked_decod...
File net/http/http_chunked_decoder.cc (right):

http://codereview.chromium.org/8921006/diff/12001/net/http/http_chunked_decod...
net/http/http_chunked_decoder.cc:191: if
(chunk_size.find_first_not_of("0123456789abcdefABCDEF")
need {} around if and else blocks when condition wraps.

http://codereview.chromium.org/8921006/diff/12001/net/http/http_response_head...
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/8921006/diff/12001/net/http/http_response_head...
net/http/http_response_headers.cc:703: base::StringToInt(StringPiece(&(*code),
(int)(p - code)),
code & p are iterators...

[Ted Vessenes, 2011-12-14 04:44:41]

Hopefully that's the last round of formatting updates.  Yes, I've signed the
contributor agreement.

[erikwright (departed), 2011-12-14 05:29:57]

LGTM with nits.

Brett, PTAL at base/... and chrome/...

Chris, PTAL at net/...

I will verify the CLA signature when I return to the office.

http://codereview.chromium.org/8921006/diff/15002/base/string_number_conversi...
File base/string_number_conversions.cc (right):

http://codereview.chromium.org/8921006/diff/15002/base/string_number_conversi...
base/string_number_conversions.cc:378: VALUE, BASE>
Sorry, didn't look closely enough at this.

BASE should technically be wrapped below VALUE.

The opening bracket must be at the end of the line containing BASE>. The closing
bracket and semi-colon can appear there or on the next line. I tend to do:

VALUE,
BASE> {};

http://codereview.chromium.org/8921006/diff/15002/base/string_number_conversi...
base/string_number_conversions.cc:390: VALUE, BASE>
Same.

http://codereview.chromium.org/8921006/diff/15002/chrome/browser/autocomplete...
File chrome/browser/autocomplete_history_manager.cc (right):

http://codereview.chromium.org/8921006/diff/15002/chrome/browser/autocomplete...
chrome/browser/autocomplete_history_manager.cc:69: &area))
Sorry, missed this one last time too.

Can you fix up the missing {} on these "if" statements?

I know, they were broken before you got here.

[Ted Vessenes, 2011-12-14 16:25:38]

Thanks for the information on splitting up long template lines.  Made those
consistent.  I also just reviewed all of
chrome/browser/autocomplete_history_manager.cc for missing braces on multi-line
if statements and fixed all of them.  Campfire rule applies here.

[erikwright (departed), 2011-12-14 16:38:29]

On 2011/12/14 16:25:38, tedv wrote:
> Thanks for the information on splitting up long template lines.  Made those
> consistent.  I also just reviewed all of
> chrome/browser/autocomplete_history_manager.cc for missing braces on
multi-line
> if statements and fixed all of them.  Campfire rule applies here.

All my nits are resolved. I have to admit I don't know the campfire rule
reference.

[Ted Vessenes, 2011-12-14 16:42:59]

When visiting a campsite, leave it in better condition than you found it, even
if it involves picking up trash left by other people and so on.

[cbentzel, 2011-12-14 16:54:09]

http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
File base/string_number_conversions.h (right):

http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
base/string_number_conversions.h:63: BASE_EXPORT bool StringToInt(const
StringPiece16& input, int* output);
Is there a reason this isn't called String16ToInt? I know it wasn't before, but
it seems like inconsistent naming both within the overall codebase, as well as
even within this file with IntToString16.

I understand that part of the reason IntToString16 is needed is that you can't
overload based on return value. However, it feels like we should still be
symmetric in naming.

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

http://codereview.chromium.org/8921006/diff/17001/net/base/net_util.cc#newcode75
net/base/net_util.cc:75: using base::StringPiece;
I wouldn't add a using declaration - it's only being used in one call site so
may as well just explicitly specify the namespace there.

http://codereview.chromium.org/8921006/diff/17001/net/base/x509_cert_types.cc
File net/base/x509_cert_types.cc (right):

http://codereview.chromium.org/8921006/diff/17001/net/base/x509_cert_types.cc...
net/base/x509_cert_types.cc:13: using base::StringPiece;
Again, no using declaration.

http://codereview.chromium.org/8921006/diff/17001/net/ftp/ftp_ctrl_response_b...
File net/ftp/ftp_ctrl_response_buffer.cc (right):

http://codereview.chromium.org/8921006/diff/17001/net/ftp/ftp_ctrl_response_b...
net/ftp/ftp_ctrl_response_buffer.cc:97: if
(base::StringToInt(StringPiece(line.begin(), line.begin() + 3),
What would happen if you just passed in 3 here? Would the line.begin() get
implicitly converted to a const char* and call the const char*, len constructor?

I'm always wary about overloading for this reason, but it's a key point of the
StringPiece class.

[erikwright (departed), 2011-12-14 17:01:09]

http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
File base/string_number_conversions.h (right):

http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
base/string_number_conversions.h:63: BASE_EXPORT bool StringToInt(const
StringPiece16& input, int* output);
On 2011/12/14 16:54:09, cbentzel wrote:
> Is there a reason this isn't called String16ToInt? I know it wasn't before,
but
> it seems like inconsistent naming both within the overall codebase, as well as
> even within this file with IntToString16.
> 
> I understand that part of the reason IntToString16 is needed is that you can't
> overload based on return value. However, it feels like we should still be
> symmetric in naming.

I don't have a strong opinion about this. I would prefer for someone with
OWNERship of base to make the decision.

[Ted Vessenes, 2011-12-14 17:04:02]

>
http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
> base/string_number_conversions.h:63: BASE_EXPORT bool StringToInt(const
> StringPiece16& input, int* output);
> Is there a reason this isn't called String16ToInt? I know it wasn't before,
but
> it seems like inconsistent naming both within the overall codebase, as well as
> even within this file with IntToString16.
> 
> I understand that part of the reason IntToString16 is needed is that you can't
> overload based on return value. However, it feels like we should still be
> symmetric in naming.

My understanding is that there's another CL coming down the pipeline which
changes StringPiece and StringPiece16 to be defined as variants of the
BasicStringPiece<STRING> template.  When that change is in place, there would in
theory just be one (templated) StringToInt() function which operates on
BasicStringPiece<STRING>.  That means all callers will use the StringToInt()
signature, so there's not much point in changing all the code now and changing
it back in a week or two.

That said, I'm happy to defer to popular opinion here.

>
http://codereview.chromium.org/8921006/diff/17001/net/base/net_util.cc#newcode75
> net/base/net_util.cc:75: using base::StringPiece;
> I wouldn't add a using declaration - it's only being used in one call site so
> may as well just explicitly specify the namespace there.
> 
>
http://codereview.chromium.org/8921006/diff/17001/net/base/x509_cert_types.cc...
> net/base/x509_cert_types.cc:13: using base::StringPiece;
> Again, no using declaration.

I'll cut those out.

>
http://codereview.chromium.org/8921006/diff/17001/net/ftp/ftp_ctrl_response_b...
> net/ftp/ftp_ctrl_response_buffer.cc:97: if
> (base::StringToInt(StringPiece(line.begin(), line.begin() + 3),
> What would happen if you just passed in 3 here? Would the line.begin() get
> implicitly converted to a const char* and call the const char*, len
constructor?

I'm not sure if line.begin() would get implicitly converted to a character
array, but if it did, it would call a different StringPiece() constructor. 
However, that constructor would create a bit-identical object to the code
written above, so I'm not sure if this qualifies as a dangerous interface from a
maintainability standpoint.

[erikwright (departed), 2011-12-14 17:12:07]

Unfortunately, the CL that is pending would not change the signatures here,
because we would still only define non-templated functions here and simply
implement them in terms of functions in the .cc file.

On Wed, Dec 14, 2011 at 12:04 PM, <tedvessenes@gmail.com> wrote:

>
>
> http://codereview.chromium.**org/8921006/diff/17001/base/**
>
string_number_conversions.h#**newcode63<http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversions.h#newcode63>
>
>> base/string_number_**conversions.h:63: BASE_EXPORT bool StringToInt(const
>> StringPiece16& input, int* output);
>> Is there a reason this isn't called String16ToInt? I know it wasn't
>> before,
>>
> but
>
>> it seems like inconsistent naming both within the overall codebase, as
>> well as
>> even within this file with IntToString16.
>>
>
>  I understand that part of the reason IntToString16 is needed is that you
>> can't
>> overload based on return value. However, it feels like we should still be
>> symmetric in naming.
>>
>
> My understanding is that there's another CL coming down the pipeline which
> changes StringPiece and StringPiece16 to be defined as variants of the
> BasicStringPiece<STRING> template.  When that change is in place, there
> would in
> theory just be one (templated) StringToInt() function which operates on
> BasicStringPiece<STRING>.  That means all callers will use the
> StringToInt()
> signature, so there's not much point in changing all the code now and
> changing
> it back in a week or two.
>
> That said, I'm happy to defer to popular opinion here.
>
>
>
> http://codereview.chromium.**org/8921006/diff/17001/net/**
>
base/net_util.cc#newcode75<http://codereview.chromium.org/8921006/diff/17001/net/base/net_util.cc#newcode75>
>
>> net/base/net_util.cc:75: using base::StringPiece;
>> I wouldn't add a using declaration - it's only being used in one call
>> site so
>> may as well just explicitly specify the namespace there.
>>
>
>
> http://codereview.chromium.**org/8921006/diff/17001/net/**
>
base/x509_cert_types.cc#**newcode13<http://codereview.chromium.org/8921006/diff/17001/net/base/x509_cert_types.cc#newcode13>
>
>> net/base/x509_cert_types.cc:**13: using base::StringPiece;
>> Again, no using declaration.
>>
>
> I'll cut those out.
>
>
>
> http://codereview.chromium.**org/8921006/diff/17001/net/**
>
ftp/ftp_ctrl_response_buffer.**cc#newcode97<http://codereview.chromium.org/8921006/diff/17001/net/ftp/ftp_ctrl_response_buffer.cc#newcode97>
>
>> net/ftp/ftp_ctrl_response_**buffer.cc:97: if
>> (base::StringToInt(**StringPiece(line.begin(), line.begin() + 3),
>> What would happen if you just passed in 3 here? Would the line.begin() get
>> implicitly converted to a const char* and call the const char*, len
>>
> constructor?
>
> I'm not sure if line.begin() would get implicitly converted to a character
> array, but if it did, it would call a different StringPiece() constructor.
> However, that constructor would create a bit-identical object to the code
> written above, so I'm not sure if this qualifies as a dangerous interface
> from a
> maintainability standpoint.
>
>
http://codereview.chromium.**org/8921006/<http://codereview.chromium.org/8921...
>

[brettw, 2011-12-14 17:25:47]

base LGTM, I didn't look at the rest.

http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
File base/string_number_conversions.cc (right):

http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
base/string_number_conversions.cc:373: namespace {
Style nit: can you move this to the top so all the anonymous namespace stuff is
collected together. We normally don't mix namespaces in the middle like this.

http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
File base/string_number_conversions.h (right):

http://codereview.chromium.org/8921006/diff/17001/base/string_number_conversi...
base/string_number_conversions.h:63: BASE_EXPORT bool StringToInt(const
StringPiece16& input, int* output);
I could go either way on this. It will work with any kind of string you have and
it doesn't matter, so I don't feel like we need to hit you over the head with
it. Since we used to use StringToInt, so let's just keep it.

[Ted Vessenes, 2011-12-14 17:53:25]

Fixed the namespace nitpicks and left the StringToInt() interface as written. 
Let me know if you spot anything else.

[cbentzel, 2011-12-14 18:14:54]

LGTM

http://codereview.chromium.org/8921006/diff/22004/net/base/x509_cert_types.cc
File net/base/x509_cert_types.cc (right):

http://codereview.chromium.org/8921006/diff/22004/net/base/x509_cert_types.cc...
net/base/x509_cert_types.cc:13: using base::StringPiece;
Remove this using declaration.

http://codereview.chromium.org/8921006/diff/22004/net/proxy/proxy_bypass_rule...
File net/proxy/proxy_bypass_rules.cc (right):

http://codereview.chromium.org/8921006/diff/22004/net/proxy/proxy_bypass_rule...
net/proxy/proxy_bypass_rules.cc:15: using base::StringPiece;
Remove this using declaration.

[Ted Vessenes, 2011-12-14 19:05:24]

Removed those "using" declarations and, anticipating similar feedback from other
OWNERS, removed a few others from other files.

[erikwright (departed), 2011-12-14 19:09:58]

Hi Ted,

I'll take OWNER responsibility for the chrome/ stuff.

I had in-flight comments for the 'using' changes you anticipated, and one other
nit.

Once you have that in, I'll hit the commit button.

Thanks again!

-Erik

http://codereview.chromium.org/8921006/diff/22004/chrome/browser/history/text...
File chrome/browser/history/text_database.cc (right):

http://codereview.chromium.org/8921006/diff/22004/chrome/browser/history/text...
chrome/browser/history/text_database.cc:15: #include "base/string_piece.h"
I guess we don't need this include at the moment. I'll add it in when I revisit
with my CL.

http://codereview.chromium.org/8921006/diff/22004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8921006/diff/22004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:35: using base::StringPiece;
Remove 'using' for consistency with cbentzel's comments.

http://codereview.chromium.org/8921006/diff/22004/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/8921006/diff/22004/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:43: using
base::StringPiece;
Remove 'using' for consistency with cbentzel's comments.

[Ted Vessenes, 2011-12-14 19:21:44]

Removed that include file per request.

[commit-bot: I haz the power, 2011-12-14 19:22:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/21005

[commit-bot: I haz the power, 2011-12-14 19:22:36]

Presubmit check for 8921006-21005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  net/ftp/ftp_ctrl_response_buffer.cc

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/profiles/profile_info_cache.cc,chrome/browser/component_updater/component_updater_service.cc

Presubmit checks took 4.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[erikwright (departed), 2011-12-14 20:21:07]

mal, PTAL at chrome/browser/component_updater/component_updater_service.cc

mirandac, PTAL at chrome/browser/profiles/profile_info_cache.cc

Thanks,

Erik

[Mark Larson, 2011-12-14 23:47:30]

LGTM
for changes in component updater

[Miranda Callahan, 2011-12-15 08:37:09]

On 2011/12/14 23:47:30, Mark Larson wrote:
> LGTM
> for changes in component updater

profiles/* LGTM.

[commit-bot: I haz the power, 2011-12-15 13:44:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/21005

[commit-bot: I haz the power, 2011-12-15 13:44:39]

Can't apply patch for file chrome/browser/autocomplete_history_manager.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/autocomplete_history_manager.cc
Hunk #1 FAILED at 19.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/autocomplete_history_manager.cc.rej

[Ted Vessenes, 2011-12-15 17:32:25]

Fixed the merge conflict and uploaded a version rebased onto the latest sync. 
This *should* go through now.

[commit-bot: I haz the power, 2011-12-15 20:09:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/27001

[commit-bot: I haz the power, 2011-12-15 20:45:57]

Try job failure for 8921006-27001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Ted Vessenes, 2011-12-15 20:50:15]

Failures were caused by a performance test that was still using the
StringToInt(begin_iter, end_iter, &val) interface.  Fixed and reuploaded.

[commit-bot: I haz the power, 2011-12-15 21:58:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/37001

[commit-bot: I haz the power, 2011-12-16 03:53:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/31003

[erikwright (departed), 2011-12-16 03:56:33]

http://codereview.chromium.org/8921006/diff/31003/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

http://codereview.chromium.org/8921006/diff/31003/base/mac/mac_util.mm#newcod...
base/mac/mac_util.mm:525: if
(!base::StringToInt(base::StringPiece(uname_info.release, dot),
Does this work because a std::string::iterator is really a char*?

It seems that, technically, the right invocation would be
StringPiece(uname_info.release, dot - uname_info.release)

[Ted Vessenes, 2011-12-16 04:00:51]

Yes, that's the reason it works.  I was a bit suspicious when I saw the data
type of dot, but the original code used the three argument iterator syntax, so I
figured it was acceptable.  If anything else breaks, I'll fix this when I fix
the other thing.  I don't know if it's worth the trouble if this CL passes tests
though.

[grt (UTC plus 2), 2011-12-16 04:16:53]

On 2011/12/16 04:00:51, tedv wrote:
> Yes, that's the reason it works.  I was a bit suspicious when I saw the data
> type of dot, but the original code used the three argument iterator syntax, so
I
> figured it was acceptable.  If anything else breaks, I'll fix this when I fix
> the other thing.  I don't know if it's worth the trouble if this CL passes
tests
> though.

The actual type of std::string::iterator is implementation-defined.  Relying on
it being char* (or there being an implicit conversion to one from a char*) seems
fragile to me.

[commit-bot: I haz the power, 2011-12-16 04:44:27]

Try job failure for 8921006-31003 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Ted Vessenes, 2011-12-16 05:44:52]

Fragile is an understatement-- apparently it doesn't build at all.  Now I'm
curious how the old version even compiled.  At any rate, that's more than enough
reason to fix it.  Nothing else cropped up, so that's the only delta in the
latest CL.

[commit-bot: I haz the power, 2011-12-16 11:43:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/36004

[commit-bot: I haz the power, 2011-12-16 12:13:12]

Try job failure for 8921006-36004 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Ted Vessenes, 2011-12-16 16:53:39]

Is this a legitimate compilation failure or just flakiness in the windows test
machines?  I've looked over the logs and don't see anything out of the
ordinary-- certainly nothing associated with this CL.

[erikwright (departed), 2011-12-16 16:56:03]

Near as I can tell, the second failure was spurious, but the first one was
legit. The Linux and Mac failures also both appear to be spurious.

See here:

http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

I'm surprised that the Windows builds appear to stop immediately on
failure. It would certainly be nice to be able to get all the failures at
once.

-e

On Fri, Dec 16, 2011 at 11:53 AM, <tedvessenes@gmail.com> wrote:

> Is this a legitimate compilation failure or just flakiness in the windows
> test
> machines?  I've looked over the logs and don't see anything out of the
> ordinary-- certainly nothing associated with this CL.
>
>
http://codereview.chromium.**org/8921006/<http://codereview.chromium.org/8921...
>

[Ted Vessenes, 2011-12-16 17:11:44]

I just audited all files matching '*test*cc' for inconsistent StringToInt()
interfaces and that appears to be only instance, which I fixed.  I did catch
once instance of the form 'x.substr(y)' instead of 'StringPiece(x, y)' which I
updated as well.

[commit-bot: I haz the power, 2011-12-16 20:51:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/31027

[commit-bot: I haz the power, 2011-12-16 20:51:27]

Presubmit check for 8921006-31027 failed and returned exit status 1.

Running presubmit commit checks ...
  checking owners took a long time: 1350ms

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/sync/test/integration/extensions_helper.cc

Presubmit checks took 5.0s to calculate.

[erikwright (departed), 2011-12-16 21:05:49]

http://codereview.chromium.org/8921006/diff/31027/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/extensions_helper.cc (right):

http://codereview.chromium.org/8921006/diff/31027/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/extensions_helper.cc:118:
!base::StringToInt(base::StringPiece(name, strlen(extension_name_prefix)),
Are you sure this works? Looks like it needs to be StringPiece(name.begin() +
strlen(extension_name_prefix), name.end())

[Ted Vessenes, 2011-12-16 21:19:39]

Thanks for the catch; I mentally parsed the original code as name.substr(0,
strlen(extension_name_prefix));

On 2011/12/16 21:05:49, erikwright wrote:
>
http://codereview.chromium.org/8921006/diff/31027/chrome/browser/sync/test/in...
> File chrome/browser/sync/test/integration/extensions_helper.cc (right):
> 
>
http://codereview.chromium.org/8921006/diff/31027/chrome/browser/sync/test/in...
> chrome/browser/sync/test/integration/extensions_helper.cc:118:
> !base::StringToInt(base::StringPiece(name, strlen(extension_name_prefix)),
> Are you sure this works? Looks like it needs to be StringPiece(name.begin() +
> strlen(extension_name_prefix), name.end())

[commit-bot: I haz the power, 2011-12-16 21:22:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/34008

[commit-bot: I haz the power, 2011-12-16 21:23:03]

Presubmit check for 8921006-34008 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/sync/test/integration/extensions_helper.cc

Presubmit checks took 3.9s to calculate.

[Ted Vessenes, 2011-12-16 21:33:14]

Given that the update to
chrome/browser/sync/test/integration/extensions_helper.cc are largely aesthetic
and would require another review, I'm just reverting this section instead.

[commit-bot: I haz the power, 2011-12-16 21:33:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/41051

[commit-bot: I haz the power, 2011-12-16 23:49:44]

Try job failure for 8921006-41051 (retry) on win_rel for step "net_unittests".
It's a second try, previously, step "net_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Ted Vessenes, 2011-12-16 23:55:35]

It's hard to say without a windows build in front of me, but the hang in
GetContentRange() is most likely caused by a StringPiece() being given a begin
iterator that starts after the end iterator.  I'm looking into this now.

[commit-bot: I haz the power, 2011-12-17 17:18:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/44001

[Ted Vessenes, 2011-12-17 18:12:54]

I added the sanity check of "begin <= end" in StringPiece construction, even
though it technically shouldn't matter (unless you want to feed StringPiece a
reverse iterator, but I can't fathom the motivation for that).  But it didn't
address this infinite loop in the Windows test case.

So right now I'm installing Visual Studio and Chromium on a windows machine to
debug this myself.  If anyone has advice on a faster way to debug this problem
though, I'd love to hear it.

[erikwright (departed), 2011-12-17 18:17:01]

http://codereview.chromium.org/8921006/diff/44001/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/8921006/diff/44001/base/string_piece.h#newcode65
base/string_piece.h:65: : ptr_(&(*begin)), length_((end > begin) ?
(size_type)(end - begin) : 0) { }
In fact, you should use the same condition in the initialization of ptr_:

ptr_((end > begin) ? &(*begin) : NULL)

[Ted Vessenes, 2011-12-17 18:24:21]

This might be philosophical, but the code should probably use >=, so that a zero
length string piece has a valid internal pointer, but one constructed with
invalid iterators has a NULL pointer.

[erikwright (departed), 2011-12-17 18:28:27]

On 2011/12/17 18:24:21, tedv wrote:
> This might be philosophical, but the code should probably use >=, so that a
zero
> length string piece has a valid internal pointer, but one constructed with
> invalid iterators has a NULL pointer.

It's actually not philisophical, unfortunately.

It is illegal to dereference begin() on an empty string (there may be no buffer,
so &(*begin) is the equivalent of &(*NULL)). A checking STL implementation may
crash at this point.

[commit-bot: I haz the power, 2011-12-17 18:54:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/45004

[commit-bot: I haz the power, 2011-12-17 21:35:30]

Change committed as 114929

[Ryan Sleevi, 2011-12-17 22:31:07]

This got reverted for making the ASAN bots red, reporting buffer overflows.

Looks like there may be one or more subtleties missed? Or were the ASAN bots
hosed?

[erikwright (departed), 2011-12-18 00:48:44]

On 2011/12/17 22:31:07, Ryan Sleevi wrote:
> This got reverted for making the ASAN bots red, reporting buffer overflows.
> 
> Looks like there may be one or more subtleties missed? Or were the ASAN bots
> hosed?

It looks like the build containing the revert similarly failed.

[Ted Vessenes, 2011-12-18 01:02:02]

On 2011/12/18 00:48:44, erikwright wrote:
> It looks like the build containing the revert similarly failed.

I'm looking into what's going on now.  Which revert are we talking about by the
way?

[Ryan Sleevi, 2011-12-18 06:33:52]

On 2011/12/18 00:48:44, erikwright wrote:
> On 2011/12/17 22:31:07, Ryan Sleevi wrote:
> > This got reverted for making the ASAN bots red, reporting buffer overflows.
> > 
> > Looks like there may be one or more subtleties missed? Or were the ASAN bots
> > hosed?
> 
> It looks like the build containing the revert similarly failed.

Yes, it turned out that
http://src.chromium.org/viewvc/chrome?view=rev&revision=114926 had landed. It
didn't show up on the main waterfall, and tedv's change was the first to trigger
it.

I let tedv know on IRC that things are good to reland this CL once the ASAN bots
are good. Unfortunately, the ASAN Builder needs a clobber (
http://build.chromium.org/p/chromium.memory/builders/ASAN%20Builder ) following
my revert ( http://src.chromium.org/viewvc/chrome?view=rev&revision=114936 ),
but unfortunately I don't have the ability to do that. Changing the whitespace
file ( http://src.chromium.org/viewvc/chrome?view=rev&revision=114937 ) was not
sufficient - the builder needs an actual clobber.

[commit-bot: I haz the power, 2011-12-18 15:50:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/45004

[commit-bot: I haz the power, 2011-12-18 17:16:30]

Change committed as 114944

[Ted Vessenes, 2011-12-18 23:53:36]

I've uploaded fixes for compiler breakage on Chrome OS.  As apparently cros bots
are not part of the standard try bot system, I want to manually test this before
doing a standard submit.  (Otherwise there's a chance the patch will break for
Chrome OS in some unforeseen way, but pass the standard try bot gauntlet and get
committed to master.)

I don't have official access to the try bots yet, so someone else will have to
handle that (or give me access, but that's probably premature).  At any rate,
let me know how else I can assist here.

[battre, 2011-12-19 09:46:20]

On 2011/12/18 23:53:36, tedv wrote:
> I've uploaded fixes for compiler breakage on Chrome OS.  As apparently cros
bots
> are not part of the standard try bot system, I want to manually test this
before
> doing a standard submit.  (Otherwise there's a chance the patch will break for
> Chrome OS in some unforeseen way, but pass the standard try bot gauntlet and
get
> committed to master.)
> 
> I don't have official access to the try bots yet, so someone else will have to
> handle that (or give me access, but that's probably premature).  At any rate,
> let me know how else I can assist here.

http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/5363
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/5476
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/5755
http://build.chromium.org/p/tryserver.chromium/builders/linux_view/builds/1307
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
http://build.chromium.org/p/tryserver.chromium/builders/linux_sync/builds/1392
http://build.chromium.org/p/tryserver.chromium/builders/mac_sync/builds/1328
http://build.chromium.org/p/tryserver.chromium/builders/win_sync/builds/1379

[commit-bot: I haz the power, 2011-12-19 14:21:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/8921006/40004

[commit-bot: I haz the power, 2011-12-19 16:10:57]

Change committed as 114993