Add a unittest case of ReplaceStringPlaceholders()

Support more than 9 arguments for ReplaceStringPlaceholders()

- support more than 9 arguments ($1, $2, $3, ..., $9 and $10, $11, $12, ...) for ReplaceStringPlaceholders()
- add the unittest named ReplaceStringPlaceholdersMoreThan9Replacements for testing more than 9 arguments for ReplaceStringPlaceholders()

BUG=16963
TEST=string_util_unittest:StringUtilTest.ReplaceStringPlaceholdersMoreThan9Replacements

Patch by haraken@google.com.

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

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

[haraken1, 2011-04-18 17:09:42]

Thank you very much for the review and very sorry for this late reply. 

I first committed this review request from haraken.dev@gmail.com about one week
ago, but now I changed the e-mail address to haraken@google.com. (Actually, I
used haraken.dev@gmail.com because I could not "git-cl upload" from
haraken@google.com... but now I could find the way. Sorry for confusing!)

> http://codereview.chromium.org/6821001/diff/1/base/string_util.cc#newcode862
> base/string_util.cc:862: ++i;
> What happens if we're at the end of a string like "Foo$1"?  > Won't we read
past the end of the string?  Can you add a test for this case?

I added the unittest case for the above. Could you take a look?

Thanks

[tony, 2011-04-19 16:47:49]

LGTM!  I'll try to send it through the commit queue.

[tony, 2011-04-19 18:57:27]

On 2011/04/19 16:47:49, tony wrote:
> LGTM!  I'll try to send it through the commit queue.

Oh, I bet we need approval from someone in owners.

[Evan Martin, 2011-04-19 19:06:46]

LGTM, but the review message is confusing.

If this is two changes, can you commit them as separate changes?
If this is one change, can you make the review description describe it all in
one, without repeated BUG= lines?

http://codereview.chromium.org/6877008/diff/1/base/string_util_unittest.cc
File base/string_util_unittest.cc (left):

http://codereview.chromium.org/6877008/diff/1/base/string_util_unittest.cc#ol...
base/string_util_unittest.cc:887: // are fewer inputs than outputs.
Is it important to retain the test coverage that this old comment was claiming? 
Does this new test cover that as well?

[tony, 2011-04-19 19:21:48]

http://codereview.chromium.org/6877008/diff/1/base/string_util_unittest.cc
File base/string_util_unittest.cc (left):

http://codereview.chromium.org/6877008/diff/1/base/string_util_unittest.cc#ol...
base/string_util_unittest.cc:887: // are fewer inputs than outputs.
On 2011/04/19 19:06:46, Evan Martin wrote:
> Is it important to retain the test coverage that this old comment was
claiming? 
> Does this new test cover that as well?

Evan is right, we should just add a new test, not modify the existing test.

[haraken1, 2011-04-20 07:21:30]

tony wrote:
> Evan is right, we should just add a new test, not modify the existing test.

I restored TEST(StringUtilTest, ReplaceStringPlaceholdersTooFew) as before. 

Evan wrote:
> If this is two changes, can you commit them as separate changes?
> If this is one change, can you make the review description describe it all in
> one, without repeated BUG= lines?

This is one change (although I changed my e-mail address during this review).
Now the description part contains only one "BUG=" line. Is it OK?


On 2011/04/19 19:21:48, tony wrote:
> http://codereview.chromium.org/6877008/diff/1/base/string_util_unittest.cc
> File base/string_util_unittest.cc (left):
> 
>
http://codereview.chromium.org/6877008/diff/1/base/string_util_unittest.cc#ol...
> base/string_util_unittest.cc:887: // are fewer inputs than outputs.
> On 2011/04/19 19:06:46, Evan Martin wrote:
> > Is it important to retain the test coverage that this old comment was
> claiming? 
> > Does this new test cover that as well?
> 
> Evan is right, we should just add a new test, not modify the existing test.

[tony, 2011-04-20 17:08:49]

LGTM.  Trying the commit queue...

[tony, 2011-04-20 23:54:56]

Committed manually in r82405.  Thanks!

[tony, 2011-04-21 02:21:06]

Reverted in r82423 because lots of tests started to fail on windows:
http://build.chromium.org/p/chromium/builders/Vista%20Tests%20%281%29/builds/...

[haraken1, 2011-04-21 18:58:30]

>
http://build.chromium.org/p/chromium/builders/Vista%2520Tests%2520%25281%2529...

I guess that the above problem is not the problem caused by our commit but the
problem caused by other commits associated with content/browser/* or
chrome/browser/*, but now I could not confirm it since I do not yet have a
permission for using trybot and do not have a windows Vista environment now. 

What action should I take next?

[tony, 2011-04-21 19:06:07]

On 2011/04/21 18:58:30, haraken wrote:
> I guess that the above problem is not the problem caused by our commit but the
> problem caused by other commits associated with content/browser/* or
> chrome/browser/*, but now I could not confirm it since I do not yet have a
> permission for using trybot and do not have a windows Vista environment now. 
> 
> What action should I take next?

After reverting, all the test passed again (there were no other changes).  That
suggests that it was this patch that caused the problem.  Here's an example
showing the tests passing after the revert:
http://build.chromium.org/p/chromium/builders/XP%20Tests%20%281%29/builds/2695

Do you have a windows machine you can build and test on?  Otherwise, if you
upload a new patch, I can send it to the try bots for you, although that'll be
pretty slow.

[haraken1, 2011-04-21 20:16:03]

> Do you have a windows machine you can build and test on?  Otherwise, if you
> upload a new patch, I can send it to the try bots for you, although that'll be
> pretty slow.

I am sorry but I do not have a windows machine now (it will take time to get
it). Would you send it to the trybots? Thank you. 

I rebased our change with the latest chromium repository, tested using
base_unittests and uploaded it (although SysInfoTest in the base_unittests
failed). In fact, what I did here is just the rebase and I changed nothing to
the source code that you reviewed the other day. 

> After reverting, all the test passed again (there were no other changes). 
That
> suggests that it was this patch that caused the problem.  

I mean, that patch includes 3 other changes (other than our change) associated
with content/browser/* and chrome/browser/*, and I doubt that those changes
caused the problem.

[tony, 2011-04-21 21:58:43]

Sent to the try bots as master#0aea5 .

[tony, 2011-04-21 21:59:12]

On 2011/04/21 21:58:43, tony wrote:
> Sent to the try bots as master#0aea5 .

Err, master#0a3a5

[haraken1, 2011-06-07 13:33:51]

I am very sorry for this late reply... I added the check "i !=
format_string.end()" to prevent overrun of string parsing. I confirmed that
StringUtilTest.ReplaceStringPlaceholdersMoreThan9Replacements works correctly
both on Linux and Windows with this modification.

[tony, 2011-06-07 17:21:25]

Sending to the try bots.  If it passes, I'll commit it for you.

[tony, 2011-06-07 20:23:34]

r88190