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

net/BUILD.gn

 # Copyright (c) 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 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")
 
 # TODO(cjhopman): //build/config/android/rules.gni also imports grit_rule.gni.
 # Currently, that file can't be imported multiple times.  Make this always
 # imported when http://crbug.com/393704 is fixed.
 if (!is_android) {
   import("//tools/grit/grit_rule.gni")
 }
@@ skipping 36 matching lines @@
 config("net_config") {
   defines = []
   if (posix_avoid_mmap) {
     defines += [ "POSIX_AVOID_MMAP" ]
   }
   if (disable_file_support) {
     defines += [ "DISABLE_FILE_SUPPORT" ]
   }
 }
 
-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
 
   cflags = []
   defines = [
     # TODO(GYP) Note that the GYP file supports linux_link_kerberos (defaults to
     # 0) which implies that we run pkg_config on kerberos and link to that
     # rather than setting this define which will dynamically open it. That
     # doesn't seem to be set in the regular builds, so we're skipping this
     # capability here.
     "DLOPEN_KERBEROS",
     "NET_IMPLEMENTATION",
   ]
 
   configs += [
     "//build/config:precompiled_headers",
 
     # TODO(jschuh): crbug.com/167187 fix size_t to int truncations.
     "//build/config/compiler:no_size_t_to_int_warning",
   ]
   public_configs = [ ":net_config" ]
   include_dirs = []
 
   public_deps = [
     ":net_quic_proto",
     "//crypto",
     "//crypto:platform",
-    "//url",
   ]
   deps = [
     ":net_resources",
     "//base",
     "//base:prefs",
     "//base/third_party/dynamic_annotations",
     "//net/base/registry_controlled_domains",
     "//sdch",
     "//third_party/protobuf:protobuf_lite",
     "//third_party/zlib",
-    "//url",
   ]
 
   if (use_kerberos) {
     defines += [ "USE_KERBEROS" ]
     if (is_android) {
       include_dirs += [ "/usr/include/kerberosV" ]
     }
   } else {
     sources -= [
       "http/http_auth_gssapi_posix.cc",
@@ skipping 55 matching lines @@
       "ftp/ftp_server_type_histograms.h",
       "ftp/ftp_transaction.h",
       "ftp/ftp_transaction_factory.h",
       "ftp/ftp_util.cc",
       "ftp/ftp_util.h",
       "url_request/ftp_protocol_handler.cc",
       "url_request/ftp_protocol_handler.h",
       "url_request/url_request_ftp_job.cc",
       "url_request/url_request_ftp_job.h",
     ]
+  } else {
+    deps += [ "//third_party/icu" ]
   }
 
   if (enable_built_in_dns) {
     defines += [ "ENABLE_BUILT_IN_DNS" ]
   } else {
     sources -= [
       "dns/address_sorter_posix.cc",
       "dns/address_sorter_posix.h",
       "dns/dns_client.cc",
     ]
@@ skipping 303 matching lines @@
     sources += [
       "base/address_tracker_linux.cc",
       "base/address_tracker_linux.h",
       "base/network_interfaces_linux.cc",
       "base/network_interfaces_linux.h",
       "base/platform_mime_util_linux.cc",
     ]
     set_sources_assignment_filter(sources_assignment_filter)
     deps += [ ":net_jni_headers" ]
   }
+}
 
-  if (use_icu_alternatives_on_android) {
-    sources += [
+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",
+    ]
+

[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/net_string_util_icu_alternatives_android.cc",
       "base/net_string_util_icu_alternatives_android.h",
     ]
-  } else {
-    deps += [
-      "//base:i18n",
-      "//third_party/icu",
-    ]
-    sources += [
-      "base/filename_util_icu.cc",
-      "base/net_string_util_icu.cc",
-      "base/net_util_icu.cc",
-    ]
   }
 }
 
 grit("net_resources") {
   source = "base/net_resources.grd"
   use_qualified_include = true
   outputs = [
     "grit/net_resources.h",
     "net_resources.pak",
   ]
 }
 
 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",
     "quic/proto/source_address_token.proto",
   ]
   cc_generator_options = "dllexport_decl=NET_EXPORT_PRIVATE:"
   cc_include = "net/base/net_export.h"
 
   defines = [ "NET_IMPLEMENTATION" ]
 
@@ skipping 671 matching lines @@
 
 if (is_android) {
   generate_jni("net_jni_headers") {
     sources = [
       "android/java/src/org/chromium/net/AndroidCertVerifyResult.java",
       "android/java/src/org/chromium/net/AndroidKeyStore.java",
       "android/java/src/org/chromium/net/AndroidNetworkLibrary.java",
       "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",
     ]
     jni_package = "net"
   }
   generate_jni("net_test_jni_headers") {
     sources = [
       "android/javatests/src/org/chromium/net/AndroidKeyStoreTestUtil.java",
       "test/android/javatests/src/org/chromium/net/test/DummySpnegoAuthenticator.java",
@@ skipping 285 matching lines @@
     ]
   }
 
   if (!enable_built_in_dns) {
     sources -= [
       "dns/address_sorter_posix_unittest.cc",
       "dns/address_sorter_unittest.cc",
     ]
   }
 
-  # 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 -= [
       "proxy/proxy_resolver_v8_tracing_unittest.cc",
       "proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc",
       "proxy/proxy_resolver_v8_unittest.cc",
     ]
   }
 
   if (use_v8_in_net && !is_android) {
@@ skipping 88 matching lines @@
       # TODO(mmenke):  This depends on test_support_base, which depends on
       #                icu.  Figure out a way to remove that dependency.
       "//testing/android/native_test:native_test_native_code",
     ]
     set_sources_assignment_filter([])
     sources += [ "base/address_tracker_linux_unittest.cc" ]
     set_sources_assignment_filter(sources_assignment_filter)
     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) {
       data += [ "$root_out_dir/net_unittests.exe.pdb" ]
     } else if (is_mac) {
       data += [ "$root_out_dir/net_unittests.dSYM/" ]
     }
   }
 }
 
@@ skipping 26 matching lines @@
   if (enable_websockets) {
     sources += [ "websockets/websocket_frame_perftest.cc" ]
   }
 
   if (use_v8_in_net) {
     deps += [ ":net_with_v8" ]
   } else {
     sources -= [ "proxy/proxy_resolver_perftest.cc" ]
   }
 }