Remove net & url small, iOS ICU alternatives, unit tests.

Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced |use_platform_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small| target.
4. Renamed 'define' USE_ICU_ALTERNATIVES_ON_ANDROID to
   USE_PLATFORM_ICU_ALTERNATIVES.
5. Added use_platform_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.
13. Added new flag 'disable_brotli_filter' which is false by default.
    Set the flag to true in cr_cronet.py.
14. Converted 'define' USE_PLATFORM_ICU_ALTERNATIVES to build flags.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92

BUG=601172
Committed: https://crrev.com/abe280e7da8b757d8f622cc9c9421880aaa38231
Cr-Commit-Position: refs/heads/master@{#387382}

[kapishnikov, 2016-03-28 22:27:46]

kapishnikov@chromium.org changed reviewers:
+ bengr@chromium.org, brettw@chromium.org, mef@chromium.org,
xunjieli@chromium.org

[kapishnikov, 2016-03-28 22:27:46]

bengr@chromium.org: Please review changes in 

mef@chromium.org: Please review changes in 

xunjieli@chromium.org: Please review changes in 

brettw@chromium.org: Please review changes in 

This change will require changes on Cronet bots to set use_icu_alternative flag
value to true. I am not sure whether there are non-Cronet bots that are building
net_small and will be impacted by changes in cronet, google_apis,
data_reduction_proxy.

[kapishnikov, 2016-03-28 22:31:06]

Description was changed from

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced |use_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small target|.
4. Added gzipped.html to the assets of cronet test app when GN is used.
   This fixed some failing cronet tests.
5. Added use_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92
==========

to

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced |use_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small target|.
4. Added gzipped.html to the assets of cronet test app when GN is used.
   This fixed some failing cronet tests.
5. Added use_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92
==========

[kapishnikov, 2016-03-28 22:31:06]

kapishnikov@chromium.org changed reviewers:
+ pauljensen@chromium.org

[kapishnikov, 2016-03-31 13:58:53]

On 2016/03/28 22:27:46, kapishnikov wrote:
> mailto:bengr@chromium.org: Please review changes in 
> 
> mailto:mef@chromium.org: Please review changes in 
> 
> mailto:xunjieli@chromium.org: Please review changes in 
> 
> mailto:brettw@chromium.org: Please review changes in 
> 
> This change will require changes on Cronet bots to set use_icu_alternative
flag
> value to true. I am not sure whether there are non-Cronet bots that are
building
> net_small and will be impacted by changes in cronet, google_apis,
> data_reduction_proxy.

a friendly ping

[xunjieli, 2016-03-31 13:59:41]

https://codereview.chromium.org/1839803002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1839803002/diff/20001/build/common.gypi#newco...
build/common.gypi:616: 'use_icu_alternatives%': 0,
the "third-party ICU implementation" is a little broad. 

Is this flag more like using platform's ICU implementation (and not any
third-party implementation)? use_platform_icu_alternatives?

https://codereview.chromium.org/1839803002/diff/20001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/20001/components/cronet/andro...
components/cronet/android/BUILD.gn:499:
"test/assets/test/gzipped.html.mock-http-headers",
Can we add the missing assets in a separate CL?

https://codereview.chromium.org/1839803002/diff/20001/components/data_reducti...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/1839803002/diff/20001/components/data_reducti...
components/data_reduction_proxy.gypi:85: # Small versions of libraries for
Cronet.
Probably need to get rid of these special purpose targets as a follow-up?

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn#newcode459
net/BUILD.gn:459: # Use ICU alternative on Android & Disable Brotli support.
Could you file a crbug to say that brotli doesn't work with icu alternatives and
put the number here?

https://codereview.chromium.org/1839803002/diff/20001/net/base/net_string_uti...
File net/base/net_string_util_icu_alternatives_ios.mm (right):

https://codereview.chromium.org/1839803002/diff/20001/net/base/net_string_uti...
net/base/net_string_util_icu_alternatives_ios.mm:18: DCHECK(false) << "Not
implemented yet.";
#include "base/logging.h"

https://codereview.chromium.org/1839803002/diff/20001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1839803002/diff/20001/net/net.gyp#newcode145
net/net.gyp:145: 'USE_ICU_ALTERNATIVES_ON_IOS=1',
USE_ICU_ALTERNATIVES_ON_ANDROID and USE_ICU_ALTERNATIVES_ON_IOS probably can be 

USE_PLATFORM_ICU_ALTERNATIVES ?

https://codereview.chromium.org/1839803002/diff/20001/url/url_canon_icu_alter...
File url/url_canon_icu_alternatives_ios.mm (right):

https://codereview.chromium.org/1839803002/diff/20001/url/url_canon_icu_alter...
url/url_canon_icu_alternatives_ios.mm:15: // Only allow ASCII in Cronet on iOS
to avoid ICU dependency. Use NSString+IDN
Can we be more general and not mention Cronet in the comment?

[kapishnikov, 2016-03-31 16:25:53]

https://codereview.chromium.org/1839803002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1839803002/diff/20001/build/common.gypi#newco...
build/common.gypi:616: 'use_icu_alternatives%': 0,
On 2016/03/31 13:59:40, xunjieli wrote:
> the "third-party ICU implementation" is a little broad. 
> 
> Is this flag more like using platform's ICU implementation (and not any
> third-party implementation)? use_platform_icu_alternatives?

Done. Renamed to use_platform_icu_alternatives.

https://codereview.chromium.org/1839803002/diff/20001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/20001/components/cronet/andro...
components/cronet/android/BUILD.gn:499:
"test/assets/test/gzipped.html.mock-http-headers",
On 2016/03/31 13:59:40, xunjieli wrote:
> Can we add the missing assets in a separate CL? 

Done. https://codereview.chromium.org/1850443005/

https://codereview.chromium.org/1839803002/diff/20001/components/data_reducti...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/1839803002/diff/20001/components/data_reducti...
components/data_reduction_proxy.gypi:85: # Small versions of libraries for
Cronet.
On 2016/03/31 13:59:40, xunjieli wrote:
> Probably need to get rid of these special purpose targets as a follow-up?

The difference between |data_reduction_proxy_core_browser_small| and
|data_reduction_proxy_core_browser| is more than referencing net & net_small,
e.g. |data_reduction_proxy_core_browser| has
'../third_party/leveldatabase/leveldatabase.gyp:leveldatabase' as the
dependency.

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn#newcode459
net/BUILD.gn:459: # Use ICU alternative on Android & Disable Brotli support.
On 2016/03/31 13:59:40, xunjieli wrote:
> Could you file a crbug to say that brotli doesn't work with icu alternatives
and
> put the number here?

I am not sure but I think Brotli is independent from ICU alternatives. I guess
we didn't want to include Brotli support in the small configuration. We could
add another build flag like "disable_brotli_support' along with
'use_icu_alternatives' but as I understood we are still experimenting with it.

https://codereview.chromium.org/1839803002/diff/20001/net/base/net_string_uti...
File net/base/net_string_util_icu_alternatives_ios.mm (right):

https://codereview.chromium.org/1839803002/diff/20001/net/base/net_string_uti...
net/base/net_string_util_icu_alternatives_ios.mm:18: DCHECK(false) << "Not
implemented yet.";
On 2016/03/31 13:59:40, xunjieli wrote:
> #include "base/logging.h"

Done.

https://codereview.chromium.org/1839803002/diff/20001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1839803002/diff/20001/net/net.gyp#newcode145
net/net.gyp:145: 'USE_ICU_ALTERNATIVES_ON_IOS=1',
On 2016/03/31 13:59:41, xunjieli wrote:
> USE_ICU_ALTERNATIVES_ON_ANDROID and USE_ICU_ALTERNATIVES_ON_IOS probably can
be 
> 
> USE_PLATFORM_ICU_ALTERNATIVES ?

Done. Renamed to USE_PLATFORM_ICU_ALTERNATIVES.

https://codereview.chromium.org/1839803002/diff/20001/url/url_canon_icu_alter...
File url/url_canon_icu_alternatives_ios.mm (right):

https://codereview.chromium.org/1839803002/diff/20001/url/url_canon_icu_alter...
url/url_canon_icu_alternatives_ios.mm:15: // Only allow ASCII in Cronet on iOS
to avoid ICU dependency. Use NSString+IDN
On 2016/03/31 13:59:41, xunjieli wrote:
> Can we be more general and not mention Cronet in the comment? 

Done.

[xunjieli, 2016-03-31 19:17:54]

https://codereview.chromium.org/1839803002/diff/20001/components/data_reducti...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/1839803002/diff/20001/components/data_reducti...
components/data_reduction_proxy.gypi:85: # Small versions of libraries for
Cronet.
On 2016/03/31 16:25:52, kapishnikov wrote:
> On 2016/03/31 13:59:40, xunjieli wrote:
> > Probably need to get rid of these special purpose targets as a follow-up?
> 
> The difference between |data_reduction_proxy_core_browser_small| and
> |data_reduction_proxy_core_browser| is more than referencing net & net_small,
> e.g. |data_reduction_proxy_core_browser| has
> '../third_party/leveldatabase/leveldatabase.gyp:leveldatabase' as the
> dependency.

Acknowledged.

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn#newcode459
net/BUILD.gn:459: # Use ICU alternative on Android & Disable Brotli support.
On 2016/03/31 16:25:52, kapishnikov wrote:
> On 2016/03/31 13:59:40, xunjieli wrote:
> > Could you file a crbug to say that brotli doesn't work with icu alternatives
> and
> > put the number here?
> 
> I am not sure but I think Brotli is independent from ICU alternatives. I guess
> we didn't want to include Brotli support in the small configuration. We could
> add another build flag like "disable_brotli_support' along with
> 'use_icu_alternatives' but as I understood we are still experimenting with it.


I see. But it is confusing to lump these two things together here. It seems like
that brotli support and icu are somehow related. I think adding another build
flag makes sense.

https://codereview.chromium.org/1839803002/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/60001/net/BUILD.gn#newcode1702
net/BUILD.gn:1702: #Unit tests that are not supported by the current ICU
alternatives on iOS.
nit: need a space

https://codereview.chromium.org/1839803002/diff/60001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1839803002/diff/60001/net/net.gyp#newcode126
net/net.gyp:126: 'enable_websockets': 0,
I don't think these three variables should be set here. Same goes to the
defines.

https://codereview.chromium.org/1839803002/diff/60001/url/features.gni
File url/features.gni (right):

https://codereview.chromium.org/1839803002/diff/60001/url/features.gni#newcode1
url/features.gni:1: # Copyright 2015 The Chromium Authors. All rights reserved.
2016

[kapishnikov, 2016-03-31 21:08:36]

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn#newcode459
net/BUILD.gn:459: # Use ICU alternative on Android & Disable Brotli support.
On 2016/03/31 19:17:53, xunjieli wrote:
> On 2016/03/31 16:25:52, kapishnikov wrote:
> > On 2016/03/31 13:59:40, xunjieli wrote:
> > > Could you file a crbug to say that brotli doesn't work with icu
alternatives
> > and
> > > put the number here?
> > 
> > I am not sure but I think Brotli is independent from ICU alternatives. I
guess
> > we didn't want to include Brotli support in the small configuration. We
could
> > add another build flag like "disable_brotli_support' along with
> > 'use_icu_alternatives' but as I understood we are still experimenting with
it.
> 
> 
> I see. But it is confusing to lump these two things together here. It seems
like
> that brotli support and icu are somehow related. I think adding another build
> flag makes sense.

I agree. If we introduce the new 'disable_brotli_support' global flag, we should
respect it everywhere where it makes sense. Ots & woff2 third party depend on
brotli but I think these should always depend on the real brotli implementation.
We can either address it in this or a different CL.

https://codereview.chromium.org/1839803002/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/60001/net/BUILD.gn#newcode1702
net/BUILD.gn:1702: #Unit tests that are not supported by the current ICU
alternatives on iOS.
On 2016/03/31 19:17:54, xunjieli wrote:
> nit: need a space

Done.

https://codereview.chromium.org/1839803002/diff/60001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1839803002/diff/60001/net/net.gyp#newcode126
net/net.gyp:126: 'enable_websockets': 0,
On 2016/03/31 19:17:54, xunjieli wrote:
> I don't think these three variables should be set here. Same goes to the
> defines.

Done. Removed from here and ios. Also moved the common 'defines' up.

https://codereview.chromium.org/1839803002/diff/60001/url/features.gni
File url/features.gni (right):

https://codereview.chromium.org/1839803002/diff/60001/url/features.gni#newcode1
url/features.gni:1: # Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/03/31 19:17:54, xunjieli wrote:
> 2016

Done.

[xunjieli, 2016-04-01 14:16:44]

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/20001/net/BUILD.gn#newcode459
net/BUILD.gn:459: # Use ICU alternative on Android & Disable Brotli support.
On 2016/03/31 21:08:35, kapishnikov wrote:
> On 2016/03/31 19:17:53, xunjieli wrote:
> > On 2016/03/31 16:25:52, kapishnikov wrote:
> > > On 2016/03/31 13:59:40, xunjieli wrote:
> > > > Could you file a crbug to say that brotli doesn't work with icu
> alternatives
> > > and
> > > > put the number here?
> > > 
> > > I am not sure but I think Brotli is independent from ICU alternatives. I
> guess
> > > we didn't want to include Brotli support in the small configuration. We
> could
> > > add another build flag like "disable_brotli_support' along with
> > > 'use_icu_alternatives' but as I understood we are still experimenting with
> it.
> > 
> > 
> > I see. But it is confusing to lump these two things together here. It seems
> like
> > that brotli support and icu are somehow related. I think adding another
build
> > flag makes sense.
> 
> I agree. If we introduce the new 'disable_brotli_support' global flag, we
should
> respect it everywhere where it makes sense. Ots & woff2 third party depend on
> brotli but I think these should always depend on the real brotli
implementation.
> We can either address it in this or a different CL.

I am fine with doing it as a follow-up. Maybe file a bug and link the bug number
here?

https://codereview.chromium.org/1839803002/diff/80001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/80001/net/BUILD.gn#newcode467
net/BUILD.gn:467: "DISABLE_FTP_SUPPORT",
These two defines should not be here?

https://codereview.chromium.org/1839803002/diff/80001/net/BUILD.gn#newcode479
net/BUILD.gn:479: "DISABLE_FTP_SUPPORT",
These two defines should not be here?

https://codereview.chromium.org/1839803002/diff/80001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1839803002/diff/80001/net/net.gyp#newcode119
net/net.gyp:119: 'DISABLE_FTP_SUPPORT=1',
The two defines (other than USE_PLATFORM_ICU_ALTERNATIVES) should not be here.

https://codereview.chromium.org/1839803002/diff/80001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/80001/url/BUILD.gn#newcode140
url/BUILD.gn:140: #Unit tests that are not supported by the current ICU
alternatives on iOS.
nit: need a space.

[bengr, 2016-04-01 17:46:06]

Is there a bug for this CL?

https://codereview.chromium.org/1839803002/diff/80001/components/data_reducti...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/1839803002/diff/80001/components/data_reducti...
components/data_reduction_proxy.gypi:90: 'target_name':
'data_reduction_proxy_core_browser_small',
Do we still need _small versions of these targets if we're no longer using
net_small and url_lib_use_icu_alternatives_on_android?

Could you get rid of these? Here and line 108?

[kapishnikov, 2016-04-01 18:55:01]

https://codereview.chromium.org/1839803002/diff/80001/components/data_reducti...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/1839803002/diff/80001/components/data_reducti...
components/data_reduction_proxy.gypi:90: 'target_name':
'data_reduction_proxy_core_browser_small',
On 2016/04/01 17:46:06, bengr wrote:
> Do we still need _small versions of these targets if we're no longer using
> net_small and url_lib_use_icu_alternatives_on_android?
> 
> Could you get rid of these? Here and line 108?

The difference between |data_reduction_proxy_core_browser_small| and
|data_reduction_proxy_core_browser| is more than referencing net & net_small,
e.g. |data_reduction_proxy_core_browser| has
'../third_party/leveldatabase/leveldatabase.gyp:leveldatabase' as the
dependency.

I got rid of |data_reduction_proxy_core_common_small|.

https://codereview.chromium.org/1839803002/diff/80001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/80001/net/BUILD.gn#newcode467
net/BUILD.gn:467: "DISABLE_FTP_SUPPORT",
On 2016/04/01 14:16:43, xunjieli wrote:
> These two defines should not be here?

Nice catch. These two defines come from "net_config":
  public_configs = [ ":net_config" ]

Fixed.

https://codereview.chromium.org/1839803002/diff/80001/net/BUILD.gn#newcode479
net/BUILD.gn:479: "DISABLE_FTP_SUPPORT",
On 2016/04/01 14:16:43, xunjieli wrote:
> These two defines should not be here?

Removed.

https://codereview.chromium.org/1839803002/diff/80001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1839803002/diff/80001/net/net.gyp#newcode119
net/net.gyp:119: 'DISABLE_FTP_SUPPORT=1',
On 2016/04/01 14:16:43, xunjieli wrote:
> The two defines (other than USE_PLATFORM_ICU_ALTERNATIVES) should not be here.


Yes, these ones are defined in common.gypi. Thanks!

https://codereview.chromium.org/1839803002/diff/80001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/80001/url/BUILD.gn#newcode140
url/BUILD.gn:140: #Unit tests that are not supported by the current ICU
alternatives on iOS.
On 2016/04/01 14:16:43, xunjieli wrote:
> nit: need a space.

Done.

[xunjieli, 2016-04-01 19:14:45]

xunjieli@chromium.org changed reviewers:
+ eustas@chromium.org

[xunjieli, 2016-04-01 19:14:46]

lgtm mod the comment below.
+ eustas: please review the additional disable brotli flag.

https://codereview.chromium.org/1839803002/diff/120001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/1839803002/diff/120001/url/url.gyp#newcode105
url/url.gyp:105: 'url_canon_icu_unittest.cc',
This list of unittests in this gyp file is different from the one in gn.

[mef, 2016-04-01 20:42:50]

looks pretty good!

https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi...
components/cronet.gypi:179: 'target_name': 'cronet_static_small',
do we still need cronet_static_small vs cronet_static?

https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi...
components/cronet.gypi:182: 'USE_PLATFORM_ICU_ALTERNATIVES=1',
should this be conditioned on 'use_platform_icu_alternatives' flag?

https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
File components/cronet/android/BUILD.gn (left):

https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
components/cronet/android/BUILD.gn:191:
cronet_static_tmpl("cronet_static_small") {
yay!

https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
File components/cronet/android/cronet_library_loader.cc (right):

https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
components/cronet/android/cronet_library_loader.cc:33: #if
defined(USE_PLATFORM_ICU_ALTERNATIVES)
maybe we should enter a crbug to convert this into buildflag?

https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
components/data_reduction_proxy.gypi:90: 'target_name':
'data_reduction_proxy_core_browser_small',
should this be called 'data_reduction_proxy_core_browser'?

https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
components/data_reduction_proxy/core/browser/BUILD.gn:50:
source_set("browser_small") {
is it still needed?

https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn#newcode462
net/BUILD.gn:462: if (is_android && use_platform_icu_alternatives) {
are nested ifs allowed in gn? 

If so, maybe do 
if (use_platform_icu_alternatives) {
  if (is_android) {
  } else if (is_ios) {
  }
} else {
}

OTOH it is unclear what to do if use_platform_icu_alternatives is set and it is
not android or ios, so feel free to ignore/argue.

https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn#newcode469
net/BUILD.gn:469: deps += [ ":net_jni_headers" ]
Is it needed? I'd think that net_jni_headers is added on android anyway.
OTOH, shouldn't we add java files here as well?

https://codereview.chromium.org/1839803002/diff/140001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/140001/url/BUILD.gn#newcode68
url/BUILD.gn:68: if (is_android && use_platform_icu_alternatives) {
maybe split this into 3 separate ifs?

if (!use_platform_icu_alternatives) {
}

if (is_android && use_platform_icu_alternatives) {
}

if (is_ios && use_platform_icu_alternatives) {
}


that would better match unittests.

https://codereview.chromium.org/1839803002/diff/140001/url/features.gni
File url/features.gni (right):

https://codereview.chromium.org/1839803002/diff/140001/url/features.gni#newcode9
url/features.gni:9: use_platform_icu_alternatives = false
Just to confirm, this flag is used in url, net and cronet?

[mef, 2016-04-03 18:42:06]

On 2016/04/01 20:42:50, mef wrote:
> looks pretty good!
> 
> https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi
> File components/cronet.gypi (right):
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi...
> components/cronet.gypi:179: 'target_name': 'cronet_static_small',
> do we still need cronet_static_small vs cronet_static?
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi...
> components/cronet.gypi:182: 'USE_PLATFORM_ICU_ALTERNATIVES=1',
> should this be conditioned on 'use_platform_icu_alternatives' flag?
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
> File components/cronet/android/BUILD.gn (left):
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
> components/cronet/android/BUILD.gn:191:
> cronet_static_tmpl("cronet_static_small") {
> yay!
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
> File components/cronet/android/cronet_library_loader.cc (right):
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
> components/cronet/android/cronet_library_loader.cc:33: #if
> defined(USE_PLATFORM_ICU_ALTERNATIVES)
> maybe we should enter a crbug to convert this into buildflag?
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
> File components/data_reduction_proxy.gypi (right):
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
> components/data_reduction_proxy.gypi:90: 'target_name':
> 'data_reduction_proxy_core_browser_small',
> should this be called 'data_reduction_proxy_core_browser'?
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
> File components/data_reduction_proxy/core/browser/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
> components/data_reduction_proxy/core/browser/BUILD.gn:50:
> source_set("browser_small") {
> is it still needed?
> 
> https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn
> File net/BUILD.gn (right):
> 
> https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn#newcode462
> net/BUILD.gn:462: if (is_android && use_platform_icu_alternatives) {
> are nested ifs allowed in gn? 
> 
> If so, maybe do 
> if (use_platform_icu_alternatives) {
>   if (is_android) {
>   } else if (is_ios) {
>   }
> } else {
> }
> 
> OTOH it is unclear what to do if use_platform_icu_alternatives is set and it
is
> not android or ios, so feel free to ignore/argue.
> 
> https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn#newcode469
> net/BUILD.gn:469: deps += [ ":net_jni_headers" ]
> Is it needed? I'd think that net_jni_headers is added on android anyway.
> OTOH, shouldn't we add java files here as well?
> 
> https://codereview.chromium.org/1839803002/diff/140001/url/BUILD.gn
> File url/BUILD.gn (right):
> 
> https://codereview.chromium.org/1839803002/diff/140001/url/BUILD.gn#newcode68
> url/BUILD.gn:68: if (is_android && use_platform_icu_alternatives) {
> maybe split this into 3 separate ifs?
> 
> if (!use_platform_icu_alternatives) {
> }
> 
> if (is_android && use_platform_icu_alternatives) {
> }
> 
> if (is_ios && use_platform_icu_alternatives) {
> }
> 
> 
> that would better match unittests.
> 
> https://codereview.chromium.org/1839803002/diff/140001/url/features.gni
> File url/features.gni (right):
> 
>
https://codereview.chromium.org/1839803002/diff/140001/url/features.gni#newcode9
> url/features.gni:9: use_platform_icu_alternatives = false
> Just to confirm, this flag is used in url, net and cronet?

Also, I think it should mention 'use_platform_icu_alternatives' flag in CL
description.

[eustas, 2016-04-04 11:07:04]

eustas@chromium.org changed reviewers:
+ rsleevi@chromium.org

[eustas, 2016-04-04 11:07:05]

I've thought that we are trying to avoid global build flags:
https://codereview.chromium.org/1431723002/#msg61
+Ryan

[Ryan Sleevi, 2016-04-04 17:38:18]

On 2016/04/04 11:07:05, eustas wrote:
> I've thought that we are trying to avoid global build flags:
> https://codereview.chromium.org/1431723002/#msg61
> +Ryan

We are. I see Brett Wilson is on the review list. Please explicitly confirm that
brettw is happy with the overall build system directions, as he can speak more
about the concerns and alternatives.

[kapishnikov, 2016-04-04 20:17:27]

https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi...
components/cronet.gypi:179: 'target_name': 'cronet_static_small',
On 2016/04/01 20:42:49, mef wrote:
> do we still need cronet_static_small vs cronet_static?

cronet_static_small depends on
components/components.gyp:data_reduction_proxy_core_browser_small, which does
not depend on third_party/leveldatabase/leveldatabase.gyp:leveldatabase. That is
in contrast to data_reduction_proxy_core_browser target that depends on
leveldatabase. We could introduce a new disable_leveldatabase flag but to make
it easier let's keep this target for now.

https://codereview.chromium.org/1839803002/diff/140001/components/cronet.gypi...
components/cronet.gypi:182: 'USE_PLATFORM_ICU_ALTERNATIVES=1',
On 2016/04/01 20:42:49, mef wrote:
> should this be conditioned on 'use_platform_icu_alternatives' flag?

Done. Actually removed USE_PLATFORM_ICU_ALTERNATIVES, DISABLE_FILE_SUPPORT and
DISABLE_FTP_SUPPORT defines from here since they are defined in common.gypi.

https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
File components/cronet/android/BUILD.gn (left):

https://codereview.chromium.org/1839803002/diff/140001/components/cronet/andr...
components/cronet/android/BUILD.gn:191:
cronet_static_tmpl("cronet_static_small") {
On 2016/04/01 20:42:49, mef wrote:
> yay!

Returned the target back since it depends on browser:browser_small.

https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
components/data_reduction_proxy.gypi:90: 'target_name':
'data_reduction_proxy_core_browser_small',
On 2016/04/01 20:42:49, mef wrote:
> should this be called 'data_reduction_proxy_core_browser'?

This target does not depend on
third_party/leveldatabase/leveldatabase.gyp:leveldatabase.

https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/140001/components/data_reduct...
components/data_reduction_proxy/core/browser/BUILD.gn:50:
source_set("browser_small") {
On 2016/04/01 20:42:49, mef wrote:
> is it still needed?

Yes, the other target adds "data_store_impl.cc", "data_store_impl.h" and
"leveldatabase" that we don't want to include.

https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn#newcode462
net/BUILD.gn:462: if (is_android && use_platform_icu_alternatives) {
On 2016/04/01 20:42:50, mef wrote:
> are nested ifs allowed in gn? 
> 
> If so, maybe do 
> if (use_platform_icu_alternatives) {
>   if (is_android) {
>   } else if (is_ios) {
>   }
> } else {
> }
> 
> OTOH it is unclear what to do if use_platform_icu_alternatives is set and it
is
> not android or ios, so feel free to ignore/argue.
> 

Done with failing the build if use_platform_icu_alternatives is set but the
platform is not supported.

https://codereview.chromium.org/1839803002/diff/140001/net/BUILD.gn#newcode469
net/BUILD.gn:469: deps += [ ":net_jni_headers" ]
On 2016/04/01 20:42:50, mef wrote:
> Is it needed? I'd think that net_jni_headers is added on android anyway.
> OTOH, shouldn't we add java files here as well?

net_jni_headers are added automatically only if "is_nacl == true". I am not sure
what is the relationship between net_jni_headers and nacl.

https://codereview.chromium.org/1839803002/diff/140001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/140001/url/BUILD.gn#newcode68
url/BUILD.gn:68: if (is_android && use_platform_icu_alternatives) {
On 2016/04/01 20:42:50, mef wrote:
> maybe split this into 3 separate ifs?
> 
> if (!use_platform_icu_alternatives) {
> }
> 
> if (is_android && use_platform_icu_alternatives) {
> }
> 
> if (is_ios && use_platform_icu_alternatives) {
> }
> 
> 
> that would better match unittests.
> 

It is possible that none of the conditions will match. I changed it the same way
as in 'net', failing if ICU alternative is not supported on a given platform.

https://codereview.chromium.org/1839803002/diff/140001/url/features.gni
File url/features.gni (right):

https://codereview.chromium.org/1839803002/diff/140001/url/features.gni#newcode9
url/features.gni:9: use_platform_icu_alternatives = false
On 2016/04/01 20:42:50, mef wrote:
> Just to confirm, this flag is used in url, net and cronet?

This flag is used in "net" & "url". "cronet" does not use it.

[kapishnikov, 2016-04-04 20:18:51]

Description was changed from

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced |use_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small target|.
4. Added gzipped.html to the assets of cronet test app when GN is used.
   This fixed some failing cronet tests.
5. Added use_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92
==========

to

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced
|use_platform_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small| target.
4. Renamed 'define' USE_ICU_ALTERNATIVES_ON_ANDROID to
   USE_PLATFORM_ICU_ALTERNATIVES.
5. Added use_platform_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.
13. Added new flag 'disable_brotli_filter' which is false by default.
    Set the flag to true in cr_cronet.py.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92
==========

[kapishnikov, 2016-04-04 20:55:49]

On 2016/04/04 17:38:18, Ryan Sleevi wrote:
> On 2016/04/04 11:07:05, eustas wrote:
> > I've thought that we are trying to avoid global build flags:
> > https://codereview.chromium.org/1431723002/#msg61
> > +Ryan
> 
> We are. I see Brett Wilson is on the review list. Please explicitly confirm
that
> brettw is happy with the overall build system directions, as he can speak more
> about the concerns and alternatives.

I was thinking of creating different targets for different configurations
without using the flags but quickly realized that the approach is not scalable.
E.g., let's take "net" target. There are three flags that affect the build:
DISABLE_FILE_SUPPORT, DISABLE_FTP_SUPPORT and USE_PLATFORM_ICU_ALTERNATIVES.
Theoretically, we should be able to build "net" with any combinations of these
flags. Does it mean that there should be 8 targets? Moreover, the number of
targets propagates to all dependents. E.g., let's assume that we want to build
Cronet with different "net" configurations (i.e. different combinations of the
above mentioned flags). Should Cronet has 8 targets then?

I agree that avoiding the global flags is better. E.g., that would allow us to
run net_unittests and net_small_unittests on a single bot without reconfiguring
the build. Is there any better approach that does not use the global flags and
is scalable at the same time?

[mef, 2016-04-04 22:25:44]

https://codereview.chromium.org/1839803002/diff/160001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/components/cronet/andr...
components/cronet/android/BUILD.gn:200:
"//components/data_reduction_proxy/core/common:common_small",
it seems that common_small target is no longer defined.

https://codereview.chromium.org/1839803002/diff/160001/components/data_reduct...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/components/data_reduct...
components/data_reduction_proxy/core/browser/BUILD.gn:61:
"//google_apis:google_apis",
it seems that usually we don't repeat target name if it is the same as project
name (e.g. "//google_apis").

https://codereview.chromium.org/1839803002/diff/160001/components/data_reduct...
components/data_reduction_proxy/core/browser/BUILD.gn:70: sources += [
bengr: Would it make sense to extract data_store_impl.* into separate target and
get rid of browser/browser_small duplication?

https://codereview.chromium.org/1839803002/diff/160001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/net/BUILD.gn#newcode493
net/BUILD.gn:493: # use third-party ICU implementation.
nit: Use (capital).

https://codereview.chromium.org/1839803002/diff/160001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/url/BUILD.gn#newcode147
url/BUILD.gn:147: "url_canon_icu_unittest.cc",
It's a bit strange that it fails on Android, shouldn't it work through platform
callbacks?

https://codereview.chromium.org/1839803002/diff/160001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/1839803002/diff/160001/url/url.gyp#newcode105
url/url.gyp:105: 'url_canon_icu_unittest.cc',
Strange, that this list of disabled tests is different from url/BUILD.gn.
Also a bit strange that it fails, shouldn't it use java callbacks?

[brettw, 2016-04-04 22:54:11]

https://codereview.chromium.org/1839803002/diff/160001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1839803002/diff/160001/build/common.gypi#newc...
build/common.gypi:3037: 'defines': ['USE_PLATFORM_ICU_ALTERNATIVES=1'],
Instead of this and the place(s) you set this define in the GN build, we should
generate a header with this defined so that it isn't global like GYP, nor
fragile like GN. See build/buildflag_header.gni/.gypi for docs.

https://codereview.chromium.org/1839803002/diff/160001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/components/cronet/andr...
components/cronet/android/BUILD.gn:193: "//net:net",
Just "//net" and "//url" here.

https://codereview.chromium.org/1839803002/diff/160001/components/data_reduct...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/components/data_reduct...
components/data_reduction_proxy/core/browser/BUILD.gn:55:
"//components/data_reduction_proxy/core/common:common",
As mef says below, don't duplicate the target name, just remove ":common"

https://codereview.chromium.org/1839803002/diff/160001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/url/BUILD.gn#newcode97
url/BUILD.gn:97: # use third-party ICU implementation.
This comment is confusing me since it might be saying "not ICU but another
implementation of ICU". I'd just say "Use ICU."

https://codereview.chromium.org/1839803002/diff/160001/url/features.gni
File url/features.gni (right):

https://codereview.chromium.org/1839803002/diff/160001/url/features.gni#newcode8
url/features.gni:8: # Enables the use of ICU alternatives in lieu of the
third-party ICU implementation.
This should mention what the scipr of the alternatives is. From the placement of
this, it looks like it only affects url parsing and above, but not blink? That
seems surprising to me, but I guess this is only for cronet. In that case, we
should be explicit about this here.

Also: 80 cols.

[kapishnikov, 2016-04-05 15:22:04]

https://codereview.chromium.org/1839803002/diff/160001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1839803002/diff/160001/build/common.gypi#newc...
build/common.gypi:3037: 'defines': ['USE_PLATFORM_ICU_ALTERNATIVES=1'],
On 2016/04/04 22:54:11, brettw wrote:
> Instead of this and the place(s) you set this define in the GN build, we
should
> generate a header with this defined so that it isn't global like GYP, nor
> fragile like GN. See build/buildflag_header.gni/.gypi for docs.

That is nice. I will work on it.

https://codereview.chromium.org/1839803002/diff/160001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/components/cronet/andr...
components/cronet/android/BUILD.gn:193: "//net:net",
On 2016/04/04 22:54:11, brettw wrote:
> Just "//net" and "//url" here.

Done.

https://codereview.chromium.org/1839803002/diff/160001/components/cronet/andr...
components/cronet/android/BUILD.gn:200:
"//components/data_reduction_proxy/core/common:common_small",
On 2016/04/04 22:25:43, mef wrote:
> it seems that common_small target is no longer defined.

Done.

https://codereview.chromium.org/1839803002/diff/160001/components/data_reduct...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/components/data_reduct...
components/data_reduction_proxy/core/browser/BUILD.gn:55:
"//components/data_reduction_proxy/core/common:common",
On 2016/04/04 22:54:11, brettw wrote:
> As mef says below, don't duplicate the target name, just remove ":common"

Done.

https://codereview.chromium.org/1839803002/diff/160001/components/data_reduct...
components/data_reduction_proxy/core/browser/BUILD.gn:61:
"//google_apis:google_apis",
On 2016/04/04 22:25:43, mef wrote:
> it seems that usually we don't repeat target name if it is the same as project
> name (e.g. "//google_apis").

Done.

https://codereview.chromium.org/1839803002/diff/160001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/net/BUILD.gn#newcode493
net/BUILD.gn:493: # use third-party ICU implementation.
On 2016/04/04 22:25:43, mef wrote:
> nit: Use (capital).

Done.

https://codereview.chromium.org/1839803002/diff/160001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/160001/url/BUILD.gn#newcode97
url/BUILD.gn:97: # use third-party ICU implementation.
On 2016/04/04 22:54:11, brettw wrote:
> This comment is confusing me since it might be saying "not ICU but another
> implementation of ICU". I'd just say "Use ICU."

Done.

https://codereview.chromium.org/1839803002/diff/160001/url/BUILD.gn#newcode147
url/BUILD.gn:147: "url_canon_icu_unittest.cc",
On 2016/04/04 22:25:43, mef wrote:
> It's a bit strange that it fails on Android, shouldn't it work through
platform
> callbacks?

The test depends on ICUCharsetConverter
https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon_icu_...

I found only one implementation of it:
https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon_icu.cc
, which depends on third_party/icu.

https://codereview.chromium.org/1839803002/diff/160001/url/features.gni
File url/features.gni (right):

https://codereview.chromium.org/1839803002/diff/160001/url/features.gni#newcode8
url/features.gni:8: # Enables the use of ICU alternatives in lieu of the
third-party ICU implementation.
On 2016/04/04 22:54:11, brettw wrote:
> This should mention what the scipr of the alternatives is. From the placement
of
> this, it looks like it only affects url parsing and above, but not blink? That
> seems surprising to me, but I guess this is only for cronet. In that case, we
> should be explicit about this here.
> 
> Also: 80 cols.

Done.

https://codereview.chromium.org/1839803002/diff/160001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/1839803002/diff/160001/url/url.gyp#newcode105
url/url.gyp:105: 'url_canon_icu_unittest.cc',
On 2016/04/04 22:25:44, mef wrote:
> Strange, that this list of disabled tests is different from url/BUILD.gn.
> Also a bit strange that it fails, shouldn't it use java callbacks?

See the previous comment regarding 'url_canon_icu_unittest.cc'.

I found the reason why the greater number of tests failed with GN. There was no
"deps += [ ":url_java" ]" in the 'url_unittests' target.

This fixed:
      "origin_unittest.cc",
      "scheme_host_port_unittest.cc",

Still two tests in 'url_canon_unittest.cc' are failing. E.g.:
C   14.472s Main  ../../url/url_canon_unittest.cc:594: Failure
C   14.472s Main  Value of: out_str
C   14.472s Main    Actual: "xn--rys3683h.cn"
C   14.472s Main  Expected: std::string(host_cases[i].expected)
C   14.472s Main  Which is: "%F0%AF%A1%A8%E5%A7%BB.cn"

I will investigate it further.

[kapishnikov, 2016-04-05 22:10:15]

https://codereview.chromium.org/1839803002/diff/160001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1839803002/diff/160001/build/common.gypi#newc...
build/common.gypi:3037: 'defines': ['USE_PLATFORM_ICU_ALTERNATIVES=1'],
On 2016/04/05 15:22:04, kapishnikov wrote:
> On 2016/04/04 22:54:11, brettw wrote:
> > Instead of this and the place(s) you set this define in the GN build, we
> should
> > generate a header with this defined so that it isn't global like GYP, nor
> > fragile like GN. See build/buildflag_header.gni/.gypi for docs.
> 
> That is nice. I will work on it.

Done. Changed to buildflag headers. The failure on android_compile_dbg bot is
expected since we haven't added use_platform_icu_alternatives=1 flag for it.

[kapishnikov, 2016-04-06 20:34:47]

Description was changed from

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced
|use_platform_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small| target.
4. Renamed 'define' USE_ICU_ALTERNATIVES_ON_ANDROID to
   USE_PLATFORM_ICU_ALTERNATIVES.
5. Added use_platform_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.
13. Added new flag 'disable_brotli_filter' which is false by default.
    Set the flag to true in cr_cronet.py.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92
==========

to

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced
|use_platform_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small| target.
4. Renamed 'define' USE_ICU_ALTERNATIVES_ON_ANDROID to
   USE_PLATFORM_ICU_ALTERNATIVES.
5. Added use_platform_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.
13. Added new flag 'disable_brotli_filter' which is false by default.
    Set the flag to true in cr_cronet.py.
14. Converted 'define' USE_PLATFORM_ICU_ALTERNATIVES to build flags.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92

BUG=601172
==========

[mef, 2016-04-08 13:33:15]

lgtm, I think this is much cleaner withoud net_small et al.

https://codereview.chromium.org/1839803002/diff/280001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/280001/url/BUILD.gn#newcode136
url/BUILD.gn:136: deps = []
is this needed?

[kapishnikov, 2016-04-08 13:54:32]

https://codereview.chromium.org/1839803002/diff/280001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/280001/url/BUILD.gn#newcode136
url/BUILD.gn:136: deps = []
On 2016/04/08 13:33:15, mef wrote:
> is this needed?

Yes, this is needed because the variable should be defined before += operator
can be used. However, it can be replaced with deps from line 164, which we add
unconditionally.

[kapishnikov, 2016-04-08 14:20:28]

https://codereview.chromium.org/1839803002/diff/280001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/280001/url/BUILD.gn#newcode136
url/BUILD.gn:136: deps = []
On 2016/04/08 13:54:31, kapishnikov wrote:
> On 2016/04/08 13:33:15, mef wrote:
> > is this needed?
> 
> Yes, this is needed because the variable should be defined before += operator
> can be used. However, it can be replaced with deps from line 164, which we add
> unconditionally.

Done.

[mef, 2016-04-08 16:16:29]

On 2016/04/08 14:20:28, kapishnikov wrote:
> https://codereview.chromium.org/1839803002/diff/280001/url/BUILD.gn
> File url/BUILD.gn (right):
> 
> https://codereview.chromium.org/1839803002/diff/280001/url/BUILD.gn#newcode136
> url/BUILD.gn:136: deps = []
> On 2016/04/08 13:54:31, kapishnikov wrote:
> > On 2016/04/08 13:33:15, mef wrote:
> > > is this needed?
> > 
> > Yes, this is needed because the variable should be defined before +=
operator
> > can be used. However, it can be replaced with deps from line 164, which we
add
> > unconditionally.
> 
> Done.

bengr: could you OWNER-approve components/data_reduction_proxy*
brettw: could you OWNER-approve url/*
jbudorick: could you OWNER-approve build/common.gypi and tools/mb/*

[brettw, 2016-04-08 21:40:06]

This is looking great! Sorry for being slow...

https://codereview.chromium.org/1839803002/diff/300001/net/BUILD.gn
File net/BUILD.gn (left):

https://codereview.chromium.org/1839803002/diff/300001/net/BUILD.gn#oldcode94
net/BUILD.gn:94: net_shared_sources = gypi_values.net_nacl_common_sources
This should all be moved into the net target now and just called "sources",
"deps" and "public_deps".

https://codereview.chromium.org/1839803002/diff/300001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/300001/net/BUILD.gn#newcode1673
net/BUILD.gn:1673: # Unit tests that are not supported by the current ICU
alternatives on Android.
Line length.

https://codereview.chromium.org/1839803002/diff/300001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1839803002/diff/300001/net/net.gyp#newcode165
net/net.gyp:165: 'dependencies': [
GYP hard deps aren't transitive and the generated file is needed by files in
this target, so you also need a net_features dependency on the net_unittests
target (GYP only).

https://codereview.chromium.org/1839803002/diff/300001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/300001/url/BUILD.gn#newcode103
url/BUILD.gn:103: "//third_party/icu:icudata",
You should be able to delete this icudata reference (not sure why the old file
had this, but it's referenced by the toplevel icu target so we don't need to
list it again here)

https://codereview.chromium.org/1839803002/diff/300001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/1839803002/diff/300001/url/url.gyp#newcode177
url/url.gyp:177: 'target_name': 'url_features',
Do we really need two of these? It makes sense to me that net depends on a
url_features target. Then we can delete the net_features one.

[kapishnikov, 2016-04-11 19:58:48]

Hi Brett,

Thanks for the review. Your suggestions have been implemented.

- Andrei

https://codereview.chromium.org/1839803002/diff/300001/net/BUILD.gn
File net/BUILD.gn (left):

https://codereview.chromium.org/1839803002/diff/300001/net/BUILD.gn#oldcode94
net/BUILD.gn:94: net_shared_sources = gypi_values.net_nacl_common_sources
On 2016/04/08 21:40:05, brettw wrote:
> This should all be moved into the net target now and just called "sources",
> "deps" and "public_deps".

Done, except for net_configs since it is used by multiple targets.

https://codereview.chromium.org/1839803002/diff/300001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/300001/net/BUILD.gn#newcode1673
net/BUILD.gn:1673: # Unit tests that are not supported by the current ICU
alternatives on Android.
On 2016/04/08 21:40:06, brettw wrote:
> Line length.

Done.

https://codereview.chromium.org/1839803002/diff/300001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1839803002/diff/300001/net/net.gyp#newcode165
net/net.gyp:165: 'dependencies': [
On 2016/04/08 21:40:06, brettw wrote:
> GYP hard deps aren't transitive and the generated file is needed by files in
> this target, so you also need a net_features dependency on the net_unittests
> target (GYP only).

Done.

https://codereview.chromium.org/1839803002/diff/300001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1839803002/diff/300001/url/BUILD.gn#newcode103
url/BUILD.gn:103: "//third_party/icu:icudata",
On 2016/04/08 21:40:06, brettw wrote:
> You should be able to delete this icudata reference (not sure why the old file
> had this, but it's referenced by the toplevel icu target so we don't need to
> list it again here)

Done.

https://codereview.chromium.org/1839803002/diff/300001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/1839803002/diff/300001/url/url.gyp#newcode177
url/url.gyp:177: 'target_name': 'url_features',
On 2016/04/08 21:40:06, brettw wrote:
> Do we really need two of these? It makes sense to me that net depends on a
> url_features target. Then we can delete the net_features one.

Done. Got rid of |net_features| and |components_cronet_android_features|. In
both places, url_features are referenced instead. Same done for for GN.

[kapishnikov, 2016-04-13 13:43:25]

A friendly ping. This CL waits for the OWNER approval.

bengr: could you OWNER-approve components/data_reduction_proxy*
brettw: could you OWNER-approve url/*
jbudorick: could you OWNER-approve build/common.gypi and tools/mb/*

[brettw, 2016-04-14 18:00:58]

lgtm

[kapishnikov, 2016-04-14 18:04:23]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2016-04-14 18:04:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org, mef@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1839803002/#ps420001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-14 18:04:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839803002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839803002/420001

[commit-bot: I haz the power, 2016-04-14 19:07:31]

Description was changed from

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced
|use_platform_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small| target.
4. Renamed 'define' USE_ICU_ALTERNATIVES_ON_ANDROID to
   USE_PLATFORM_ICU_ALTERNATIVES.
5. Added use_platform_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.
13. Added new flag 'disable_brotli_filter' which is false by default.
    Set the flag to true in cr_cronet.py.
14. Converted 'define' USE_PLATFORM_ICU_ALTERNATIVES to build flags.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92

BUG=601172
==========

to

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced
|use_platform_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small| target.
4. Renamed 'define' USE_ICU_ALTERNATIVES_ON_ANDROID to
   USE_PLATFORM_ICU_ALTERNATIVES.
5. Added use_platform_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.
13. Added new flag 'disable_brotli_filter' which is false by default.
    Set the flag to true in cr_cronet.py.
14. Converted 'define' USE_PLATFORM_ICU_ALTERNATIVES to build flags.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92

BUG=601172
==========

[commit-bot: I haz the power, 2016-04-14 19:07:32]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2016-04-14 19:10:12]

Description was changed from

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced
|use_platform_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small| target.
4. Renamed 'define' USE_ICU_ALTERNATIVES_ON_ANDROID to
   USE_PLATFORM_ICU_ALTERNATIVES.
5. Added use_platform_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.
13. Added new flag 'disable_brotli_filter' which is false by default.
    Set the flag to true in cr_cronet.py.
14. Converted 'define' USE_PLATFORM_ICU_ALTERNATIVES to build flags.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92

BUG=601172
==========

to

==========
Remove net & url small, iOS ICU alternatives, unit tests.

1. Removed |net_small| and |url_small| targets.
2. To keep supporting the small builds, introduced
|use_platform_icu_alternatives|
   boolean flag that indicates whether the net and url targets should be
   built with ICU alternatives. The default value of the flag is false.
3. Removed |cronet_static_small| target.
4. Renamed 'define' USE_ICU_ALTERNATIVES_ON_ANDROID to
   USE_PLATFORM_ICU_ALTERNATIVES.
5. Added use_platform_icu_alternatives=1/true to gyp/gn cr_cronet.py.
6. Changed cronet_reduction_proxy component to stop using net & url small.
7. Removed |common_small| template from data_reduction_proxy/core/common
8. Removed |google_apis_small| template from google_apis.
9. Added simplistic ICU alternatives for iOS
10. Fixed linking problem when net_unitests were build with GN (not all
    websockets tests were excluded when enable_websockets was set to false).
11. Added support for net_unitests for the small net/url variation
    (there were no tests for it before).
12. Created the list of unit tests that are failing with the current
    alternative ICU implementations on Android and iOS. Added them to the
    exclude list.
13. Added new flag 'disable_brotli_filter' which is false by default.
    Set the flag to true in cr_cronet.py.
14. Converted 'define' USE_PLATFORM_ICU_ALTERNATIVES to build flags.

The current number of passing tests:
net_with_icu_alternative iOS: 12548
net_with_icu iOS: 14549
net_with_icu_alternative Android: 15000
net_with_icu Android: 15223
url_with_icu_alternative iOS: 49
url_with_icu iOS: 91
url_with_icu_alternative Android: 50
url_with_icu Android: 92

BUG=601172

Committed: https://crrev.com/abe280e7da8b757d8f622cc9c9421880aaa38231
Cr-Commit-Position: refs/heads/master@{#387382}
==========

[commit-bot: I haz the power, 2016-04-14 19:10:14]

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