Generalizing conditional compilation logic for systems with native utf8 locale.

Generalizing conditional compilation logic for systems with native utf8 locale.
This change was needed for Chromecast, while avoiding adding yet another
preprocessor condition.

BUG=438331
Committed: https://crrev.com/bd52a8a1d59abc3e9fab7856655a8d6dee8fa90b
Cr-Commit-Position: refs/heads/master@{#308109}

[halliwell, 2014-12-10 00:52:14]

halliwell@chromium.org changed reviewers:
+ brettw@chromium.org, gunsch@chromium.org, lcwu@chromium.org,
michaelbai@chromium.org, rvargas@chromium.org

[gunsch, 2014-12-10 01:28:52]

High-level comment: it may be more appropriate to split this into two CLs (one
for the base/ change and one for the net/ change) and mail to the appropriate
OWNERS of each.

https://codereview.chromium.org/790903003/diff/1/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode982
base/base.gypi:982: # Enable more direct string conversions on OS with native
utf8 strings
OS --> platforms

https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode983
base/base.gypi:983: ['OS=="mac" or OS=="ios" or <(chromeos)==1 or
<(chromecast)==1', {
no need to wrap the chromeos/chromecast variables in <(...), just:

chromeos==1 or chromecast==1

[halliwell, 2014-12-10 15:55:53]

On 2014/12/10 01:28:52, gunsch wrote:
> High-level comment: it may be more appropriate to split this into two CLs (one
> for the base/ change and one for the net/ change) and mail to the appropriate
> OWNERS of each.

My understanding was that rvargas is in OWNERS for all affected directories ...
did
I miss something here?

[gunsch, 2014-12-10 16:35:17]

On 2014/12/10 15:55:53, halliwell wrote:
> On 2014/12/10 01:28:52, gunsch wrote:
> > High-level comment: it may be more appropriate to split this into two CLs
(one
> > for the base/ change and one for the net/ change) and mail to the
appropriate
> > OWNERS of each.
> 
> My understanding was that rvargas is in OWNERS for all affected directories
...
> did
> I miss something here?

Nope, that's accurate. Will let him comment on if this seems okay then. Fairly
small CL so probably :)

[rvargas (doing something else), 2014-12-10 21:06:50]

I looks fine but I'll defer to thestig. You are also missing the gn file.

https://codereview.chromium.org/790903003/diff/1/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode983
base/base.gypi:983: ['OS=="mac" or OS=="ios" or <(chromeos)==1 or
<(chromecast)==1', {
why the test for chromeos is not the same one as for mac (in other words,
OS=="chromeos")?

https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
File net/base/filename_util_unittest.cc (right):

https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
net/base/filename_util_unittest.cc:52: #if !defined(OS_ANDROID)
Don't you need something like if posix here?

https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
net/base/filename_util_unittest.cc:78: << GetLocaleWarningString();
The old logic printed a message if the test was to fail (and also failed the
test right away)... this logic ignores calls that pass but _should_ have failed.

[rvargas (doing something else), 2014-12-10 21:07:31]

rvargas@chromium.org changed reviewers:
+ thestig@chromium.org

[rvargas (doing something else), 2014-12-10 21:07:31]

^^

[halliwell, 2014-12-10 21:43:48]

On 2014/12/10 21:06:50, rvargas wrote:
> I looks fine but I'll defer to thestig. You are also missing the gn file.
> 
Ok, will update gn file.

> https://codereview.chromium.org/790903003/diff/1/base/base.gypi
> File base/base.gypi (right):
> 
> https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode983
> base/base.gypi:983: ['OS=="mac" or OS=="ios" or <(chromeos)==1 or
> <(chromecast)==1', {
> why the test for chromeos is not the same one as for mac (in other words,
> OS=="chromeos")?
> 
Not sure, but if you look in the gypi files, this is how the test is done
everywhere.

>
https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
> File net/base/filename_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
> net/base/filename_util_unittest.cc:52: #if !defined(OS_ANDROID)
> Don't you need something like if posix here?
> 
Yep, will add if posix.

>
https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
> net/base/filename_util_unittest.cc:78: << GetLocaleWarningString();
> The old logic printed a message if the test was to fail (and also failed the
> test right away)... this logic ignores calls that pass but _should_ have
failed.

Yes, this is deliberate.  Firstly, failing the test (as the old implementation
did) required it
to be conditionally compiled out for Chromecast - but that preprocessor define
can't be
upstreamed.  Secondly, my understanding is that the test assertion isn't there
to catch cases
where the tests pass but should have failed - it's to help people quickly
understand why the
test failed if they happened to run it under the wrong locale.

Please correct me if I'm wrong on that second point - this obviously needs
rethinking if so :)

[rvargas (doing something else), 2014-12-10 22:21:43]

>
https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
> > net/base/filename_util_unittest.cc:78: << GetLocaleWarningString();
> > The old logic printed a message if the test was to fail (and also failed the
> > test right away)... this logic ignores calls that pass but _should_ have
> failed.
> 
> Yes, this is deliberate.  Firstly, failing the test (as the old implementation
> did) required it
> to be conditionally compiled out for Chromecast - but that preprocessor define
> can't be
> upstreamed.  Secondly, my understanding is that the test assertion isn't there
> to catch cases
> where the tests pass but should have failed - it's to help people quickly
> understand why the
> test failed if they happened to run it under the wrong locale.
> 
> Please correct me if I'm wrong on that second point - this obviously needs
> rethinking if so :)

I guess you are right. What bothers me is that if I start making changes to the
code that this is testing, and I forget to set the environment as needed for the
test, then sort of by definition my local testing is flawed and I may not see it
(if for some reason I change the logic in a way that should make the test fail,
but now it does't fail). Now, in practice, these tests are just comparing a
result against an expected value so the scenario that I described is probably
too far fetched.

[Lei Zhang, 2014-12-10 22:25:15]

base/ lgtm

https://codereview.chromium.org/790903003/diff/1/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode983
base/base.gypi:983: ['OS=="mac" or OS=="ios" or <(chromeos)==1 or
<(chromecast)==1', {
On 2014/12/10 21:06:50, rvargas wrote:
> why the test for chromeos is not the same one as for mac (in other words,
> OS=="chromeos")?

There's no OS==chromeos... yet. I vaguely remember base.gypi being a bit
special, and thus all the variables are early eval.

[rvargas (doing something else), 2014-12-10 22:30:29]

On 2014/12/10 22:25:15, Lei Zhang wrote:
> base/ lgtm
> 
> https://codereview.chromium.org/790903003/diff/1/base/base.gypi
> File base/base.gypi (right):
> 
> https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode983
> base/base.gypi:983: ['OS=="mac" or OS=="ios" or <(chromeos)==1 or
> <(chromecast)==1', {
> On 2014/12/10 21:06:50, rvargas wrote:
> > why the test for chromeos is not the same one as for mac (in other words,
> > OS=="chromeos")?
> 
> There's no OS==chromeos... yet. I vaguely remember base.gypi being a bit
> special, and thus all the variables are early eval.

What?. I guess I'm getting too used to gn :p

[halliwell, 2014-12-11 00:48:28]

https://codereview.chromium.org/790903003/diff/1/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode982
base/base.gypi:982: # Enable more direct string conversions on OS with native
utf8 strings
On 2014/12/10 01:28:52, gunsch wrote:
> OS --> platforms

Done.

https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode983
base/base.gypi:983: ['OS=="mac" or OS=="ios" or <(chromeos)==1 or
<(chromecast)==1', {
On 2014/12/10 01:28:52, gunsch wrote:
> no need to wrap the chromeos/chromecast variables in <(...), just:
> 
> chromeos==1 or chromecast==1

Yep, without <(...) around variables, you get name 'xxx' is not defined errors.

https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
File net/base/filename_util_unittest.cc (right):

https://codereview.chromium.org/790903003/diff/1/net/base/filename_util_unitt...
net/base/filename_util_unittest.cc:52: #if !defined(OS_ANDROID)
On 2014/12/10 21:06:50, rvargas wrote:
> Don't you need something like if posix here? 

Done.

[halliwell, 2014-12-11 21:14:31]

halliwell@chromium.org changed reviewers:
- brettw@chromium.org, gunsch@chromium.org, lcwu@chromium.org,
michaelbai@chromium.org

[rvargas (doing something else), 2014-12-11 22:19:37]

lgtm

https://codereview.chromium.org/790903003/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/790903003/diff/20001/base/BUILD.gn#newcode939
base/BUILD.gn:939: # Enable more direct string conversions on platforms with
native utf8
nit: s/enable/allow

https://codereview.chromium.org/790903003/diff/20001/base/BUILD.gn#newcode941
base/BUILD.gn:941: if (is_mac || is_ios || is_chromeos) {
Maybe move this block closer to line 764

[rvargas (doing something else), 2014-12-11 23:44:13]

(added comments)

On Wed, Dec 10, 2014 at 4:48 PM, <halliwell@chromium.org> wrote:
>
>
> https://codereview.chromium.org/790903003/diff/1/base/base.gypi
> File base/base.gypi (right):
>
> https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode982
> base/base.gypi:982: # Enable more direct string conversions on OS with
> native utf8 strings
> On 2014/12/10 01:28:52, gunsch wrote:
>
>> OS --> platforms
>>
>
> Done.
>
> https://codereview.chromium.org/790903003/diff/1/base/base.gypi#newcode983
> base/base.gypi:983: ['OS=="mac" or OS=="ios" or <(chromeos)==1 or
> <(chromecast)==1', {
> On 2014/12/10 01:28:52, gunsch wrote:
>
>> no need to wrap the chromeos/chromecast variables in <(...), just:
>>
>
>  chromeos==1 or chromecast==1
>>
>
> Yep, without <(...) around variables, you get name 'xxx' is not defined
> errors.
>
> https://codereview.chromium.org/790903003/diff/1/net/base/
> filename_util_unittest.cc
> File net/base/filename_util_unittest.cc (right):
>
> https://codereview.chromium.org/790903003/diff/1/net/base/
> filename_util_unittest.cc#newcode52
> net/base/filename_util_unittest.cc:52: #if !defined(OS_ANDROID)
> On 2014/12/10 21:06:50, rvargas wrote:
>
>> Don't you need something like if posix here?
>>
>
> Done.
>
> https://codereview.chromium.org/790903003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[halliwell, 2014-12-12 16:12:06]

The CQ bit was checked by halliwell@chromium.org

[commit-bot: I haz the power, 2014-12-12 16:12:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/790903003/40001

[commit-bot: I haz the power, 2014-12-12 17:21:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-12 17:22:00]

Patchset 3 (id:??) landed as
https://crrev.com/bd52a8a1d59abc3e9fab7856655a8d6dee8fa90b
Cr-Commit-Position: refs/heads/master@{#308109}