Make it possible to build url/ without ICU on android.

Make it possible to build url/ without ICU on android.

This is needed to reduce binary size of net/ when build as a
library.

BUG=362608
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268740

[mmenke, 2014-04-30 16:34:50]

Fix includes

[mmenke, 2014-04-30 18:56:32]

Remove extra header

[mmenke, 2014-05-01 19:15:01]

mef:  PTAL.

This depends on https://codereview.chromium.org/250233002/

Not sure if I should be calling url::android::RegisterJni from everywhere, only
when it matters, or if code that supports USE_ICU_ALTERNATIVES should just be
calling the new registration function directly.

[mef, 2014-05-01 19:45:14]

I think that the code that supports USE_ICU_ALTERNATIVES should be calling the
new registration function directly. Maybe we should add a comment somewhere?

https://codereview.chromium.org/257673002/diff/330001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/257673002/diff/330001/build/common.gypi#newco...
build/common.gypi:488: 'use_icu_alternatives%': 0,
so, the cronet build bot would have to set it to 1, right?

https://codereview.chromium.org/257673002/diff/330001/components/cronet/andro...
File components/cronet/android/cronet_jni.cc (right):

https://codereview.chromium.org/257673002/diff/330001/components/cronet/andro...
components/cronet/android/cronet_jni.cc:20: {"UrlAndroid",
url::android::RegisterJni},
I agree that this is the right place to register jni.

https://codereview.chromium.org/257673002/diff/330001/net/test/run_all_unitte...
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/257673002/diff/330001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:37: {"UrlAndroid", url::android::RegisterJni},
Hmm, does it mean that chrome would also use platform instead of ICU or this
only applies to test?

https://codereview.chromium.org/257673002/diff/330001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/257673002/diff/330001/url/BUILD.gn#newcode11
url/BUILD.gn:11: "android/url_jni_registrar.cc",
should this go into some android-specific section?

https://codereview.chromium.org/257673002/diff/330001/url/DEPS
File url/DEPS (right):

https://codereview.chromium.org/257673002/diff/330001/url/DEPS#newcode2
url/DEPS:2: "+jni",
what about non-android platforms? does it make sense for them to depend on jni?

https://codereview.chromium.org/257673002/diff/330001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/257673002/diff/330001/url/url.gyp#newcode106
url/url.gyp:106: ['use_icu_alternatives == 1', {
maybe add [and OS == "android"] ?

https://codereview.chromium.org/257673002/diff/330001/url/url_canon_icu_unitt...
File url/url_canon_icu_unittest.cc (right):

https://codereview.chromium.org/257673002/diff/330001/url/url_canon_icu_unitt...
url/url_canon_icu_unittest.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
2014? or this is just an extract of ICU dependencies, and therefore should
retain 2013?

[mmenke, 2014-05-01 21:01:33]

Thanks for the feedback!

https://codereview.chromium.org/257673002/diff/330001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/257673002/diff/330001/build/common.gypi#newco...
build/common.gypi:488: 'use_icu_alternatives%': 0,
On 2014/05/01 19:45:15, mef wrote:
> so, the cronet build bot would have to set it to 1, right?

Yes.

https://codereview.chromium.org/257673002/diff/330001/net/test/run_all_unitte...
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/257673002/diff/330001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:37: {"UrlAndroid", url::android::RegisterJni},
On 2014/05/01 19:45:15, mef wrote:
> Hmm, does it mean that chrome would also use platform instead of ICU or this
> only applies to test?

If you look at url::android::RegisterJni, it only registers it when
USE_ICU_ALTERNATIVES is defined.  Alternatives, we could add ifdefs here, and
call url::RegisterIcuAlternativesJni directly (And export that function).  So
this means net_unittests will register the alternatives or not depending on the
flag.

This will have no effect outside net_unittests.

https://codereview.chromium.org/257673002/diff/330001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/257673002/diff/330001/url/BUILD.gn#newcode11
url/BUILD.gn:11: "android/url_jni_registrar.cc",
On 2014/05/01 19:45:15, mef wrote:
> should this go into some android-specific section?

Files that have platform names in their paths aren't compiled on other
platforms, so it's not necessary - you can find cases where this is done, and
cases where it isn't, some I'm not sure what the preferred style is.

For what it's worth, net/net.gypi does not do that.

https://codereview.chromium.org/257673002/diff/330001/url/DEPS
File url/DEPS (right):

https://codereview.chromium.org/257673002/diff/330001/url/DEPS#newcode2
url/DEPS:2: "+jni",
On 2014/05/01 19:45:15, mef wrote:
> what about non-android platforms? does it make sense for them to depend on
jni?

DEPS files aren't per-platform, and aren't used for figuring out what to build. 
Instead, they're static checks run on includes.  We could do per-file deps, but
I'm not sure we get anything out of it.

Net does the exact same thing.

https://codereview.chromium.org/257673002/diff/330001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/257673002/diff/330001/url/url.gyp#newcode106
url/url.gyp:106: ['use_icu_alternatives == 1', {
On 2014/05/01 19:45:15, mef wrote:
> maybe add [and OS == "android"] ?

Nice catch, done!  Think it's kinda weird things didn't break without that.

https://codereview.chromium.org/257673002/diff/330001/url/url_canon_icu_unitt...
File url/url_canon_icu_unittest.cc (right):

https://codereview.chromium.org/257673002/diff/330001/url/url_canon_icu_unitt...
url/url_canon_icu_unittest.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/05/01 19:45:15, mef wrote:
> 2014? or this is just an extract of ICU dependencies, and therefore should
> retain 2013?

This is largely just a separation of the old UrlCanonTests that depended on ICU.
 I couldn't manage to get git to view this as a copy, for some reason.

Everything but the includes and the QueryWithConverter was just copied. 
QueryWithConverter was separated out from URLCanonTest.Query (Some of those
tests needed ICU, some did not).

[mef, 2014-05-02 15:10:48]

lgtm

https://codereview.chromium.org/257673002/diff/330001/net/test/run_all_unitte...
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/257673002/diff/330001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:37: {"UrlAndroid", url::android::RegisterJni},
On 2014/05/01 21:01:33, mmenke wrote:
> On 2014/05/01 19:45:15, mef wrote:
> > Hmm, does it mean that chrome would also use platform instead of ICU or this
> > only applies to test?
> 
> If you look at url::android::RegisterJni, it only registers it when
> USE_ICU_ALTERNATIVES is defined.  Alternatives, we could add ifdefs here, and
> call url::RegisterIcuAlternativesJni directly (And export that function).  So
> this means net_unittests will register the alternatives or not depending on
the
> flag.
> 
> This will have no effect outside net_unittests.
Sounds good, I guess it is cleaner not to have an extra #ifdef here.

https://codereview.chromium.org/257673002/diff/330001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/257673002/diff/330001/url/BUILD.gn#newcode11
url/BUILD.gn:11: "android/url_jni_registrar.cc",
On 2014/05/01 21:01:33, mmenke wrote:
> On 2014/05/01 19:45:15, mef wrote:
> > should this go into some android-specific section?
> 
> Files that have platform names in their paths aren't compiled on other
> platforms, so it's not necessary - you can find cases where this is done, and
> cases where it isn't, some I'm not sure what the preferred style is.
> 
> For what it's worth, net/net.gypi does not do that.
sgtm. I thought it only applied to name suffix, nice to know.

https://codereview.chromium.org/257673002/diff/330001/url/DEPS
File url/DEPS (right):

https://codereview.chromium.org/257673002/diff/330001/url/DEPS#newcode2
url/DEPS:2: "+jni",
On 2014/05/01 21:01:33, mmenke wrote:
> On 2014/05/01 19:45:15, mef wrote:
> > what about non-android platforms? does it make sense for them to depend on
> jni?
> 
> DEPS files aren't per-platform, and aren't used for figuring out what to
build. 
> Instead, they're static checks run on includes.  We could do per-file deps,
but
> I'm not sure we get anything out of it.
> 
> Net does the exact same thing.
sgtm

https://codereview.chromium.org/257673002/diff/330001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/257673002/diff/330001/url/url.gyp#newcode106
url/url.gyp:106: ['use_icu_alternatives == 1', {
On 2014/05/01 21:01:33, mmenke wrote:
> On 2014/05/01 19:45:15, mef wrote:
> > maybe add [and OS == "android"] ?
> 
> Nice catch, done!  Think it's kinda weird things didn't break without that. I
think it didn't break because use_icu_alternatives is 0 everywhere else?

https://codereview.chromium.org/257673002/diff/330001/url/url_canon_icu_unitt...
File url/url_canon_icu_unittest.cc (right):

https://codereview.chromium.org/257673002/diff/330001/url/url_canon_icu_unitt...
url/url_canon_icu_unittest.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/05/01 21:01:33, mmenke wrote:
> On 2014/05/01 19:45:15, mef wrote:
> > 2014? or this is just an extract of ICU dependencies, and therefore should
> > retain 2013?
> 
> This is largely just a separation of the old UrlCanonTests that depended on
ICU.
>  I couldn't manage to get git to view this as a copy, for some reason.
> 
> Everything but the includes and the QueryWithConverter was just copied. 
> QueryWithConverter was separated out from URLCanonTest.Query (Some of those
> tests needed ICU, some did not).
sgtm

[mmenke, 2014-05-02 15:30:48]

Thanks for the feedback!

https://codereview.chromium.org/257673002/diff/330001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/257673002/diff/330001/url/url.gyp#newcode106
url/url.gyp:106: ['use_icu_alternatives == 1', {
On 2014/05/02 15:10:48, mef wrote:
> On 2014/05/01 21:01:33, mmenke wrote:
> > On 2014/05/01 19:45:15, mef wrote:
> > > maybe add [and OS == "android"] ?
> > 
> > Nice catch, done!  Think it's kinda weird things didn't break without that.
I
> think it didn't break because use_icu_alternatives is 0 everywhere else?
> 

You're right - realized that after I made the comment.

[mmenke, 2014-05-02 15:33:06]

[+brettw]:  PTAL, thanks!

url_canon_icu_unittest.cc is split off from url_canon_unittest.cc, but I could
not get git to view it as a copy rather than as a new file, regardless of
similarity setting.

[brettw, 2014-05-02 23:51:02]

https://codereview.chromium.org/257673002/diff/450001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/257673002/diff/450001/build/common.gypi#newco...
build/common.gypi:488: 'use_icu_alternatives%': 0,
Who sets this? Can we just key off of anrdoid webview or something?

https://codereview.chromium.org/257673002/diff/450001/build/common.gypi#newco...
build/common.gypi:2619: 'defines': ['USE_ICU_ALTERNATIVES=1'],
This is a global defined for every single compile of every single target in the
entire build so that you can pass it into one file. CAn't you just move it to
the url.gyp file?

https://codereview.chromium.org/257673002/diff/450001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/257673002/diff/450001/url/BUILD.gn#newcode58
url/BUILD.gn:58: if (use_icu_alternatives) {
This will break the GN build since this value is never defined. We should answer
the question I wrote in the GYP file and then I it will tell us what to do here.

[mmenke, 2014-05-03 01:15:49]

https://codereview.chromium.org/257673002/diff/450001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/257673002/diff/450001/build/common.gypi#newco...
build/common.gypi:488: 'use_icu_alternatives%': 0,
On 2014/05/02 23:51:02, brettw wrote:
> Who sets this? Can we just key off of anrdoid webview or something?

Network stack when built as a library will need it (Currently in
components/cronet).  No other modules will need this.  I'm not sure where else
to put it to share stuff between modules.

https://codereview.chromium.org/257673002/diff/450001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/257673002/diff/450001/url/BUILD.gn#newcode58
url/BUILD.gn:58: if (use_icu_alternatives) {
On 2014/05/02 23:51:02, brettw wrote:
> This will break the GN build since this value is never defined. We should
answer
> the question I wrote in the GYP file and then I it will tell us what to do
here.

I hadn't realized gn wasn't yet being used or tested on any platform.

[mmenke, 2014-05-03 01:17:47]

https://codereview.chromium.org/257673002/diff/450001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/257673002/diff/450001/build/common.gypi#newco...
build/common.gypi:488: 'use_icu_alternatives%': 0,
On 2014/05/03 01:15:50, mmenke wrote:
> On 2014/05/02 23:51:02, brettw wrote:
> > Who sets this? Can we just key off of anrdoid webview or something?
> 
> Network stack when built as a library will need it (Currently in
> components/cronet).  No other modules will need this.  I'm not sure where else
> to put it to share stuff between modules.

To clarify, "no other modules" meaning other than net/ and url/.

[brettw, 2014-05-05 18:07:24]

On 2014/05/03 01:15:49, mmenke wrote:
> I hadn't realized gn wasn't yet being used or tested on any platform.

Recently found out that the GN build is begin run by the CQ trybots but not the
manually-triggered ones. Until this is fixed, you can manually check the
linux_chromium_gn* and android_chromium_gn_* ones for manual tries.

I guess for GYP we're stuck with the global one then. I wonder if we can give it
a better name. If this is for Android only, should it have Android in the name
of the define?

For the GN build, add this to the top of url/BUILD.gn:

  declare_args() {
    # Comment here explaining what this is for.
    use_icu_alternatives = false  # or whatever you rename this to in GYP...
  }

  # Sets the USE_ICU_ALTERNATIVES define based on the build flag.
  config("url_icu_config") {
    if (use_icu_alternatives) {
      defines = [ "USE_ICU_ALTERNATIVES=1" ]
    }
  }

Then in the url target below defines = [ ...:
  configs += [ ":url_icu_config" ]
(you don't have to make this conditional since it already conditionally defines
that flag inside the config).

When we get around needing this flag in net, all we have to do is add this
config name to the list of configs applying to the net target.

[mmenke, 2014-05-06 14:54:31]

Thanks for all the information!

We'll need the use_icu_alternatives variable in net/ as well, in addition to the
define.  Will we also need the declare_args line there as well?

We'd love to get the option working on iOS as well as Android, but that may not
be possible, due to lack of an IDN to ASCII function, so I've gone ahead and
renamed the values to USE_ICU_ALTERNATIVES_ON_ANDROID, at least for the moment.

[brettw, 2014-05-06 17:09:53]

On 2014/05/06 14:54:31, mmenke wrote:
> Thanks for all the information!
> 
> We'll need the use_icu_alternatives variable in net/ as well, in addition to
the
> define.  Will we also need the declare_args line there as well?

No, all you would need in net is the one line:
  configs += [ "//url:url_icu_config" ]
and you would get the #define in that module.

[brettw, 2014-05-06 17:12:34]

LGTM, I didn't review the details of any of the Android code.

[mmenke, 2014-05-06 18:09:25]

On 2014/05/06 17:12:34, brettw wrote:
> LGTM, I didn't review the details of any of the Android code.

Great, thanks again for your help!

[mmenke, 2014-05-06 18:09:31]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-05-06 18:11:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/257673002/530001

[commit-bot: I haz the power, 2014-05-07 14:05:49]

Change committed as 268740

[mmenke, 2014-05-07 16:22:17]

On 2014/05/07 14:05:49, I haz the power (commit-bot) wrote:
> Change committed as 268740

Brett:  It doesn't look like net/BUILD.gn is getting
use_icu_alternatives_on_android variable.

gn try result (Note that it's using base revision 268770, which includes this
CL):
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...

CL:  https://codereview.chromium.org/266053002/