Reland: Make separate net and url GN targets with and without ICU

net/BUILD.gn

Index: net/BUILD.gn
diff --git a/net/BUILD.gn b/net/BUILD.gn
index 5d9801b21ae33cbd2a2070c8821bd35a15a7bc40..24544235474a8555cae75cec13343dcc32187b33 100644
--- a/net/BUILD.gn
+++ b/net/BUILD.gn
@@ -6,7 +6,6 @@ import("//build/config/crypto.gni")
 import("//build/config/features.gni")
 import("//build/config/ui.gni")
 import("//build/module_args/v8.gni")
-import("//url/config.gni")
 import("//testing/test.gni")
 import("//third_party/icu/config.gni")
 import("//third_party/protobuf/proto_library.gni")
@@ -63,7 +62,7 @@ config("net_config") {
   }
 }
 
-component("net") {
+source_set("net_common") {

[mmenke, 2015/08/20 15:37:47]

Should we restrict visibility to the two new source sets?

visibility = [":net", ":net_small"]

I believe

   sources =
       gypi_values.net_nacl_common_sources + gypi_values.net_non_nacl_sources
 
@@ -91,7 +90,6 @@ component("net") {
     ":net_quic_proto",
     "//crypto",
     "//crypto:platform",
-    "//url",
   ]
   deps = [
     ":net_resources",
@@ -102,7 +100,6 @@ component("net") {
     "//sdch",
     "//third_party/protobuf:protobuf_lite",
     "//third_party/zlib",
-    "//url",
   ]
 
   if (use_kerberos) {
@@ -178,6 +175,8 @@ component("net") {
       "url_request/url_request_ftp_job.cc",
       "url_request/url_request_ftp_job.h",
     ]
+  } else {
+    deps += [ "//third_party/icu" ]
   }
 
   if (enable_built_in_dns) {
@@ -501,21 +500,43 @@ component("net") {
     set_sources_assignment_filter(sources_assignment_filter)
     deps += [ ":net_jni_headers" ]
   }
+}
 
-  if (use_icu_alternatives_on_android) {
-    sources += [
-      "base/net_string_util_icu_alternatives_android.cc",
-      "base/net_string_util_icu_alternatives_android.h",
+component("net") {
+  public_deps = [
+    ":net_common",
+    "//url:url",
+  ]
+
+  defines = [ "NET_IMPLEMENTATION" ]
+
+  # ICU support.
+  deps = [
+    "//base:i18n",
+    "//third_party/icu",
+  ]
+  sources = [
+    "base/filename_util_icu.cc",
+    "base/net_string_util_icu.cc",
+    "base/net_util_icu.cc",
+  ]
+}
+
+if (is_android) {
+  # same as net, but with ICU dependency stripped.

[mmenke, 2015/08/20 15:37:47]

nit:  same -> Same

[xunjieli, 2015/08/21 19:17:25]

Done.

+  component("net_small") {
+    defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]

[mmenke, 2015/08/20 15:37:47]

We weren't defining this before, were we?  Do we need to add it in this CL?

[xunjieli, 2015/08/20 17:55:35]

We had this define in url/BUILD.gn: 

config("url_icu_config") {
  if (use_icu_alternatives_on_android) {
    defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
  }
}

USE_ICU_ALTERNATIVES_ON_ANDROID is used in net/android/net_jni_registrar.cc.

On second thought, I think this CL won't work.
source_set(net_common) does not know the defines we have in net_small, unlike
how gyp's "includes:net_common.gpyi" works. So we can't set
USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or DISABLE_FTP_SUPPORT in
net_small and expect these will be used in compiling net_common's sources.

Do you see a way to make this work other than duplicating the original net
target in its entirety?

+    public_deps = [
+      ":net_common",
+      "//url:url_lib_use_icu_alternatives_on_android",
     ]
-  } else {
-    deps += [
-      "//base:i18n",
-      "//third_party/icu",
+

[mmenke, 2015/08/19 22:28:26]

I think you may need NET_IMPLEMENTATION here, too.

[xunjieli, 2015/08/20 14:28:45]

net_string_util_icu_alternatives_android.h does not have any NET_EXPORT macros,
so I did not add NET_IMPLEMENTATION here. Let me know if you'd like me to do
that :)

[mmenke, 2015/08/20 15:37:47]

Looks like you're right - it's not needed.  This is also a net implementation,
though, and could legitimately have implementations of such functions, so I
think we should probably just include it, anyways.

+    deps = [
+      ":net_jni_headers",
     ]
-    sources += [
-      "base/filename_util_icu.cc",
-      "base/net_string_util_icu.cc",
-      "base/net_util_icu.cc",
+    sources = [
+      "base/net_string_util_icu_alternatives_android.cc",
+      "base/net_string_util_icu_alternatives_android.h",
     ]
   }
 }
@@ -530,7 +551,7 @@ grit("net_resources") {
 }
 
 proto_library("net_quic_proto") {
-  visibility = [ "//net" ]  # Part of the net component.
+  visibility = [ ":net_common" ]  # Part of net_common.
 
   sources = [
     "quic/proto/cached_network_parameters.proto",
@@ -1222,6 +1243,7 @@ if (is_android) {
       "android/java/src/org/chromium/net/AndroidPrivateKey.java",
       "android/java/src/org/chromium/net/GURLUtils.java",
       "android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java",
+      "android/java/src/org/chromium/net/NetStringUtil.java",
       "android/java/src/org/chromium/net/NetworkChangeNotifier.java",
       "android/java/src/org/chromium/net/ProxyChangeListener.java",
       "android/java/src/org/chromium/net/X509Util.java",
@@ -1527,10 +1549,7 @@ test("net_unittests") {
     ]
   }
 
-  # Always need use_v8_in_net to be 1 to run on Android, so just remove
-  # net_unittest's dependency on v8 when using icu alternatives instead of
-  # setting use_v8_in_net to 0.
-  if (use_v8_in_net && !use_icu_alternatives_on_android) {
+  if (use_v8_in_net) {
     deps += [ ":net_with_v8" ]
   } else {
     sources -= [
@@ -1639,14 +1658,6 @@ test("net_unittests") {
     isolate_file = "net_unittests.isolate"
   }
 
-  if (use_icu_alternatives_on_android) {
-    sources -= [
-      "base/filename_util_unittest.cc",
-      "base/net_util_icu_unittest.cc",
-    ]
-    deps -= [ "//base:i18n" ]
-  }
-
   # Symbols for crashes when running tests on swarming.
   if (symbol_level > 0) {
     if (is_win) {