Remove wchar_t versions of StringPrintf from Android

Remove wchar_t versions of StringPrintf from Android

BUG=137864
TEST=StringPrintfTest


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

[jar (doing other things), 2012-07-24 18:15:34]

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h
File base/stringprintf.h (left):

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h#oldcode36
base/stringprintf.h:36: BASE_EXPORT void StringAppendF(std::string* dst, const
char* format, ...)
This is interesting.... it was previously only advertised for non-android, and
now you're going to advertise and presumably use it.

Where are each of these used in this CL that required making them visible?

...and if you want them visible... I'd rather use separate ifdefs around the
sibling items (like line 40-41) that you want to hid.

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h
File base/stringprintf.h (right):

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h#newcode39
base/stringprintf.h:39: #if !defined(OS_ANDROID)
IMO, this grouping (by whether we're Android or not) to reduce the number of #if
statements, makes readability worse.

I don't like #if stuff (so I can sympathize with the motivation), but with
header files, folks expect to find similar functionality (e.g., StringPrintf()
for various flavors of strings) to be near either other.  I wouldn't expect to
have to look in a special non-android section to find a function :-/.

As a result, I'd rather leave the functions mostly where they naturally landed,
and put in the separate sets of #if statements around them.

[Iain Merrick, 2012-07-25 10:57:23]

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h
File base/stringprintf.h (left):

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h#oldcode36
base/stringprintf.h:36: BASE_EXPORT void StringAppendF(std::string* dst, const
char* format, ...)
On 2012/07/24 18:15:34, jar wrote:
> This is interesting.... it was previously only advertised for non-android, and
> now you're going to advertise and presumably use it.
> 
> Where are each of these used in this CL that required making them visible?
> 
> ...and if you want them visible... I'd rather use separate ifdefs around the
> sibling items (like line 40-41) that you want to hid.

Hmm, probably more interesting than I intended! I'll clean up with separate
ifdefs.

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h
File base/stringprintf.h (right):

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h#newcode39
base/stringprintf.h:39: #if !defined(OS_ANDROID)
On 2012/07/24 18:15:34, jar wrote:
> IMO, this grouping (by whether we're Android or not) to reduce the number of
#if
> statements, makes readability worse.
> 
> I don't like #if stuff (so I can sympathize with the motivation), but with
> header files, folks expect to find similar functionality (e.g., StringPrintf()
> for various flavors of strings) to be near either other.  I wouldn't expect to
> have to look in a special non-android section to find a function :-/.
> 
> As a result, I'd rather leave the functions mostly where they naturally
landed,
> and put in the separate sets of #if statements around them.

Sure, no problem! I considered both and made the opposite call, but I'm happy to
defer to your decision. :)

I also considered adding a special macro -- HAVE_WPRINTF or some such. Let me
know if you think that's a good idea.

[jar (doing other things), 2012-07-25 18:54:04]

with nit (to add comment such as suggested below)...

LGTM

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h
File base/stringprintf.h (right):

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h#newcode39
base/stringprintf.h:39: #if !defined(OS_ANDROID)
Mixed feelings:

Macros are frowned upon.  If you wanted to add one, you'd have to be REAL
careful to avoid conflicting with others, and plausibly with conventions(?).

Your suggestion would make this header more readable.

IMO, risks of problems via name pollution outweigh the gain.

I'd suggest adding one comment, before your first use of the ifdef here, saying
that: 

"OS_ANDROID does not have wchar_t, so several overloads are omitted"


On 2012/07/25 10:57:23, Iain Merrick wrote:
> On 2012/07/24 18:15:34, jar wrote:
> > IMO, this grouping (by whether we're Android or not) to reduce the number of
> #if
> > statements, makes readability worse.
> > 
> > I don't like #if stuff (so I can sympathize with the motivation), but with
> > header files, folks expect to find similar functionality (e.g.,
StringPrintf()
> > for various flavors of strings) to be near either other.  I wouldn't expect
to
> > have to look in a special non-android section to find a function :-/.
> > 
> > As a result, I'd rather leave the functions mostly where they naturally
> landed,
> > and put in the separate sets of #if statements around them.
> 
> Sure, no problem! I considered both and made the opposite call, but I'm happy
to
> defer to your decision. :)
> 
> I also considered adding a special macro -- HAVE_WPRINTF or some such. Let me
> know if you think that's a good idea.

[Iain Merrick, 2012-07-26 11:15:56]

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h
File base/stringprintf.h (right):

http://codereview.chromium.org/10800078/diff/6001/base/stringprintf.h#newcode39
base/stringprintf.h:39: #if !defined(OS_ANDROID)
On 2012/07/25 18:54:04, jar wrote:
> Mixed feelings:
> 
> Macros are frowned upon.  If you wanted to add one, you'd have to be REAL
> careful to avoid conflicting with others, and plausibly with conventions(?).
> 
> Your suggestion would make this header more readable.
> 
> IMO, risks of problems via name pollution outweigh the gain.
> 
> I'd suggest adding one comment, before your first use of the ifdef here,
saying
> that: 
> 
> "OS_ANDROID does not have wchar_t, so several overloads are omitted"

Done.

[commit-bot: I haz the power, 2012-07-26 11:18:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/husky@chromium.org/10800078/22001

[commit-bot: I haz the power, 2012-07-26 11:18:41]

Presubmit check for 10800078-22001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.1s to calculate.

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

[Iain Merrick, 2012-07-26 11:31:04]

+sky for chrome/OWNERS review

[sky, 2012-07-28 02:11:24]

http://codereview.chromium.org/10800078/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.cc (right):

http://codereview.chromium.org/10800078/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.cc:161: experiment_strings[i] =
UTF8ToUTF16(base::StringPrintf(
Why the ifdef here? Can't we use this version every where?

[Iain Merrick, 2012-07-30 08:33:32]

http://codereview.chromium.org/10800078/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.cc (right):

http://codereview.chromium.org/10800078/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.cc:161: experiment_strings[i] =
UTF8ToUTF16(base::StringPrintf(
On 2012/07/28 02:11:24, sky wrote:
> Why the ifdef here? Can't we use this version every where?

I figured there might be a good reason for using wstrings in the original code,
so I was reluctant to change it for all platforms. But I'll go ahead and make
that change. Hopefully we have unit test coverage!

Done.

[sky, 2012-08-01 03:56:35]

LGTM

[commit-bot: I haz the power, 2012-08-01 09:07:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/husky@chromium.org/10800078/26001

[commit-bot: I haz the power, 2012-08-01 10:30:03]

Change committed as 149397