Make ui gn check pass. Misc GN build improvements.

Content/public/app was messed up and didn't link everything properly, so this separates it out into browser and child like content/app is.

R=jamesr@chromium.org

Review URL: https://codereview.chromium.org/545313002

Cr-Commit-Position: refs/heads/master@{#293592}

build/config/BUILDCONFIG.gn

@@ -511,11 +511,13 @@ template("component") {
       set_sources_assignment_filter([])
 
       if (defined(invoker.all_dependent_configs)) { all_dependent_configs = invoker.all_dependent_configs }
+      if (defined(invoker.allow_circular_includes_from)) { allow_circular_includes_from = invoker.allow_circular_includes_from }
       if (defined(invoker.cflags)) { cflags = invoker.cflags }
       if (defined(invoker.cflags_c)) { cflags_c = invoker.cflags_c }
       if (defined(invoker.cflags_cc)) { cflags_cc = invoker.cflags_cc }
       if (defined(invoker.cflags_objc)) { cflags_objc = invoker.cflags_objc }
       if (defined(invoker.cflags_objcc)) { cflags_objcc = invoker.cflags_objcc }
+      if (defined(invoker.check_includes)) { check_includes = invoker.check_includes }
       if (defined(invoker.data)) { data = invoker.data }
       if (defined(invoker.datadeps)) { datadeps = invoker.datadeps }
       if (defined(invoker.defines)) { defines = invoker.defines }
@@ -530,6 +532,7 @@ template("component") {
       if (defined(invoker.output_name)) { output_name = invoker.output_name }
       if (defined(invoker.public)) { public = invoker.public }
       if (defined(invoker.sources)) { sources = invoker.sources }
+      if (defined(invoker.testonly)) { testonly = invoker.testonly }
       if (defined(invoker.visibility)) { visibility = invoker.visibility }
     }
   } else {
@@ -542,11 +545,13 @@ template("component") {
       set_sources_assignment_filter([])
 
       if (defined(invoker.all_dependent_configs)) { all_dependent_configs = invoker.all_dependent_configs }
+      if (defined(invoker.allow_circular_includes_from)) { allow_circular_includes_from = invoker.allow_circular_includes_from }
       if (defined(invoker.cflags)) { cflags = invoker.cflags }
       if (defined(invoker.cflags_c)) { cflags_c = invoker.cflags_c }
       if (defined(invoker.cflags_cc)) { cflags_cc = invoker.cflags_cc }
       if (defined(invoker.cflags_objc)) { cflags_objc = invoker.cflags_objc }
       if (defined(invoker.cflags_objcc)) { cflags_objcc = invoker.cflags_objcc }
+      if (defined(invoker.check_includes)) { check_includes = invoker.check_includes }
       if (defined(invoker.data)) { data = invoker.data }
       if (defined(invoker.datadeps)) { datadeps = invoker.datadeps }
       if (defined(invoker.defines)) { defines = invoker.defines }
@@ -561,6 +566,7 @@ template("component") {
       if (defined(invoker.output_name)) { output_name = invoker.output_name }
       if (defined(invoker.public)) { public = invoker.public }
       if (defined(invoker.sources)) { sources = invoker.sources }
+      if (defined(invoker.testonly)) { testonly = invoker.testonly }
       if (defined(invoker.visibility)) { visibility = invoker.visibility }
     }
   }
@@ -586,11 +592,13 @@ template("test") {
     set_sources_assignment_filter([])
 
     if (defined(invoker.all_dependent_configs)) { all_dependent_configs = invoker.all_dependent_configs }
+    if (defined(invoker.allow_circular_includes_from)) { allow_circular_includes_from = invoker.allow_circular_includes_from }
     if (defined(invoker.cflags)) { cflags = invoker.cflags }
     if (defined(invoker.cflags_c)) { cflags_c = invoker.cflags_c }
     if (defined(invoker.cflags_cc)) { cflags_cc = invoker.cflags_cc }
     if (defined(invoker.cflags_objc)) { cflags_objc = invoker.cflags_objc }
     if (defined(invoker.cflags_objcc)) { cflags_objcc = invoker.cflags_objcc }
+    if (defined(invoker.check_includes)) { check_includes = invoker.check_includes }
     if (defined(invoker.data)) { data = invoker.data }
     if (defined(invoker.datadeps)) { datadeps = invoker.datadeps }
     if (defined(invoker.defines)) { defines = invoker.defines }

build/secondary/tools/grit/grit_rule.gni

@@ -258,7 +258,7 @@ template("grit") {
   # The config and the action below get this visibility son only the generated
   # source set can depend on them. The variable "target_name" will get
   # overwritten inside the inner classes so we need to compute it here.
-  target_visibility = ":$target_name"
+  target_visibility = [ ":$target_name" ]
 
   # The current grit setup makes an file in $output_dir/grit/foo.h that
   # the source code expects to include via "grit/foo.h". It would be nice to
@@ -301,7 +301,20 @@ template("grit") {
 
     args += grit_flags + assert_files_flags
 
-    visibility = target_visibility
+    if (defined(invoker.visibility)) {
+      # This needs to include both what the invoker specified (since they
+      # probably include generated headers from this target), as well as the
+      # generated source set (since there's no guarantee that the visibility
+      # specified by the invoker includes our target).
+      #
+      # Only define visibility at all if the invoker specified it. Otherwise,
+      # we want to keep the public "no visibility specified" default.
+      visibility = target_visibility
+
+      # This won't work if the invoker specifies a list of targets (an uncommon
+      # case). We may need to enhance the language to make this possible.
+      visibility += [ invoker.visibility ]
+    }
 
     deps = [ "//tools/grit:grit_sources" ]
     if (defined(invoker.deps)) {

chrome/BUILD.gn

@@ -58,7 +58,7 @@ executable("chrome") {
       "//base/allocator",
       # Needed to use the master_preferences functions
       "//chrome/installer/util",
-      "//content/app:both",
+      "//content/public/app:both",
     ]
 
     # Needed for chrome_main.cc initialization of libraries.

content/BUILD.gn

@@ -13,7 +13,6 @@ content_shared_components = [
   "//content/gpu",
   "//content/plugin",
   "//content/ppapi_plugin",
-  "//content/public/app",
   "//content/public/browser:sources",
   "//content/public/child",
   "//content/public/common",
@@ -27,6 +26,7 @@ if (is_component_build) {
   shared_library("content") {
     deps = content_shared_components + [
       "//content/app",
+      "//content/public/app",
     ]
     forward_dependent_configs_from = deps
   }

content/public/app/BUILD.gn

@@ -2,7 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-source_set("app") {
+# Used internally to hold the sources shared between the various targets below.
+source_set("app_sources") {
   sources = [
     "android_library_loader_hooks.h",
     "content_main.h",
@@ -23,3 +24,45 @@ source_set("app") {
     "//content/public/utility",
   ]
 }
+
+# The structure of this is like the private content/app implementation.
+if (is_component_build) {
+
+  group("app") {
+    deps = [
+      ":app_sources",
+      "//content/app",
+    ]
+  }
+  group("browser") {
+    deps = [ "//content" ]
+  }
+  group("child") {
+    deps = [ "//content" ]
+  }
+  group("both") {
+    deps = [ "//content" ]
+  }
+
+} else {
+
+  group("browser") {
+    deps = [
+      ":app_sources",
+      "//content/app:browser"
+    ]
+  }
+  group("child") {
+    deps = [
+      ":app_sources",
+      "//content/app:child"
+    ]
+  }
+  group("both") {
+    deps = [
+      ":app_sources",
+      "//content/app:both"
+    ]
+  }
+
+}

content/test/BUILD.gn

@@ -15,7 +15,7 @@ content_tests_gypi_values = exec_script(
 # GYP version //content/content_tests.gypi:test_support_content
 static_library("test_support") {
   deps = [
-    "//content/public/app",
+    "//content/public/app:both",
     "//content/public/browser",
     "//content/public/common",
     "//net:test_support",

extensions/BUILD.gn

@@ -58,10 +58,14 @@ source_set("test_support") {
   sources = [
     "browser/api/dns/mock_host_resolver_creator.cc",
     "browser/api/dns/mock_host_resolver_creator.h",
+    "browser/api/storage/settings_test_util.cc",
+    "browser/api/storage/settings_test_util.h",
     "browser/api_test_utils.cc",
     "browser/api_test_utils.h",
     "browser/extensions_test.cc",
     "browser/extensions_test.h",
+    "browser/mock_extension_system.cc",
+    "browser/mock_extension_system.h",
     "browser/test_extensions_browser_client.cc",
     "browser/test_extensions_browser_client.h",
     "browser/test_management_policy.cc",
@@ -145,6 +149,10 @@ repack("shell_and_test_pak") {
 }
 
 # TODO(GYP): Enable this link errors are fixed.
+# This gives a link error in web_modal that is very mysterious. The GYP build
+# doesn't seem to pull in web_content_modal_dialog_manager.o since the build
+# does not have a reference to CreateNativeWebModalManager but it still links.
+# The GN build fails with this symbol being undefined.
 if (false) {
 # TODO(tfarina): Many extension unit tests run as part of Chrome"s
 # unit_tests target. They should be moved here, which may require some
@@ -154,8 +162,41 @@ test("unittests") {
   output_name = "extensions_unittests"
 
   sources = [
+    "browser/admin_policy_unittest.cc",
+    "browser/api/api_resource_manager_unittest.cc",
+    "browser/api/declarative/deduping_factory_unittest.cc",
+    "browser/api/sockets_tcp/sockets_tcp_api_unittest.cc",
+    "browser/api/storage/settings_quota_unittest.cc",
+    "browser/api/storage/storage_api_unittest.cc",
+    "browser/api/storage/storage_frontend_unittest.cc",
+    "browser/computed_hashes_unittest.cc",
+    "browser/content_hash_tree_unittest.cc",
+    "browser/event_listener_map_unittest.cc",
+    "browser/event_router_unittest.cc",
+    "browser/api_unittest.cc",
+    "browser/api_unittest.h",
+    "browser/extension_pref_value_map_unittest.cc",
+    "browser/extension_registry_unittest.cc",
+    "browser/file_highlighter_unittest.cc",
+    "browser/file_reader_unittest.cc",
+    "browser/guest_view/guest_view_manager_unittest.cc",
+    "browser/image_loader_unittest.cc",
+    "browser/image_util_unittest.cc",
     "browser/info_map_unittest.cc",
+    "browser/lazy_background_task_queue_unittest.cc",
+    "browser/management_policy_unittest.cc",
     "browser/process_manager_unittest.cc",
+    "browser/process_map_unittest.cc",
+    "browser/quota_service_unittest.cc",
+    "browser/runtime_data_unittest.cc",
+    "browser/value_store/leveldb_value_store_unittest.cc",
+    "browser/value_store/testing_value_store_unittest.cc",
+    "browser/value_store/value_store_change_unittest.cc",
+    "browser/value_store/value_store_frontend_unittest.cc",
+    "browser/value_store/value_store_unittest.cc",
+    "browser/value_store/value_store_unittest.h",
+    "browser/verified_contents_unittest.cc",
+    "browser/warning_service_unittest.cc",
     "common/api/sockets/sockets_manifest_permission_unittest.cc",
     "common/csp_validator_unittest.cc",
     "common/event_filter_unittest.cc",
@@ -185,14 +226,16 @@ test("unittests") {
     "renderer/script_context_set_unittest.cc",
     "renderer/utils_unittest.cc",
     "test/extensions_unittests_main.cc",
- ]
+  ]
 
   deps = [
     ":extensions_resources",
     ":shell_and_test_pak",
     ":test_support",
     "//base",
+    "//base:prefs_test_support",
     "//base/test:test_support",
+    "//components/keyed_service/content",
     "//content/test:test_support",
     "//device/serial",
     "//device/serial:test_util",
@@ -205,6 +248,7 @@ test("unittests") {
     "//mojo/system",
     "//testing/gmock",
     "//testing/gtest",
+    "//third_party/leveldatabase",
   ]
 
   if (is_win) {

extensions/browser/BUILD.gn

@@ -377,6 +377,7 @@ source_set("browser") {
       "//components/storage_monitor",
       "//crypto:platform",
       "//device/bluetooth",
+      "//device/core",
       "//device/hid",
       "//device/serial",
       "//device/usb",

extensions/extensions.gyp

@@ -871,6 +871,7 @@
         '<(SHARED_INTERMEDIATE_DIR)',
       ],
       'sources': [
+        # Note: sources list duplicated in GN build.
         'browser/api/dns/mock_host_resolver_creator.cc',
         'browser/api/dns/mock_host_resolver_creator.h',
         'browser/api/storage/settings_test_util.cc',

extensions/generated_extensions_api.gni

@@ -109,7 +109,11 @@ template("generated_extensions_api") {
         "--namespace=$root_namespace",
         "--generator=cpp",
         "--include-rules=$schema_include_rules" ]
-      visibility = target_visibility
+
+      if (defined(invoker.visibility)) {
+        # If visibility is restricted, add our own target to it.
+        visibility = [ invoker.visibility, target_visibility ]
+      }
     }
   }
 

mojo/public/tools/bindings/mojom.gni

@@ -12,8 +12,10 @@
 #       List of source .mojom files to compile.
 #
 #   deps (optional)
+#
 #   visibility (optional)
-#       Normal meaning.
+#       Normal meaning. However, this must be a list (normal visibility
+#       allows a single string).
 template("mojom") {
   assert(defined(invoker.sources),
          "\"sources\" must be defined for the $target_name template.")
@@ -75,11 +77,18 @@ template("mojom") {
     "{{source_gen_dir}}/{{source_name_part}}_mojom.py",
   ]
 
-  target_visibility = ":$target_name"
+  if (defined(invoker.visibility)) {
+    # Need to save this because the the target_name is overwritten inside the
+    # action to be that of the action itself. Only define this in the case the
+    # var is used to avoid unused var error.
+    target_visibility = ":$target_name"
+  }
 
   generator_target_name = target_name + "_generator"
   action_foreach(generator_target_name) {
-    visibility = target_visibility
+    if (defined(invoker.visibility)) {
+      visibility = [ target_visibility ] + invoker.visibility  # If you get an error here, make your visibility be a list.
+    }
     script = generator_script
     inputs = generator_sources
     sources = invoker.sources

tools/gn/input_file_manager.cc

@@ -215,11 +215,20 @@ const ParseNode* InputFileManager::SyncLoadFile(
     }
   }
 
-  // The other load could have failed. In this case that error will be printed
-  // to the console, but we need to return something here, so make up a
+  // The other load could have failed. In this case that error was probably
+  // printed to the console, but we need to return something here, so make up a
   // dummy error.
-  if (!data->parsed_root)
-    *err = Err(origin, "File parse failed");
+  //
+  // There is a race condition. The other load could have failed, but if the
+  // other thread is delayed for some reason, this thread could end up
+  // reporting the error to the scheduler first (since first error report
+  // wins). The user will see this one and the "real" one will be discarded.
+  if (!data->parsed_root) {
+    *err = Err(origin, "File parse failed.",
+        "If you see this, I'm really sorry, but a race condition has caused\n"
+        "me to eat your error message. It was crunchy. If the parse error\n"
+        "in your imported file isn't obvious, try re-running GN.");
+  }
   return data->parsed_root.get();
 }
 

ui/app_list/BUILD.gn

@@ -58,6 +58,7 @@ component("app_list") {
     "//base:i18n",
     "//base/third_party/dynamic_annotations",
     "//skia",
+    "//sync",
     "//third_party/icu",
     "//ui/accessibility",
     "//ui/base",

ui/aura/BUILD.gn

@@ -183,6 +183,7 @@ source_set("test_support") {
     "//ui/events:test_support",
     "//ui/gfx",
     "//ui/gfx/geometry",
+    "//ui/wm",
   ]
 
   if (is_win) {

ui/base/BUILD.gn

@@ -308,6 +308,10 @@ component("base") {
     ]
   } else {
     deps += [ "//ui/base/ime" ]
+
+    # http://crbug.com/408648 Circular includes from the ime sub-target to this
+    # one.
+    allow_circular_includes_from = [ "//ui/base/ime" ]
   }
 
   if (toolkit_views) {

ui/keyboard/BUILD.gn

@@ -41,6 +41,10 @@ component("keyboard") {
     "//ui/keyboard/webui",
     "//ui/wm",
   ]
+
+  # http://crbug.com/408651 Include cycle between this target and the "webui"
+  # sub-target.
+  allow_circular_includes_from = [ "//ui/keyboard/webui" ]
 }
 
 grit("resources_grit") {

ui/resources/BUILD.gn

@@ -15,7 +15,6 @@ group("resources") {
 }
 
 grit("ui_resources_grd") {
-  visibility = ":resources"
   source = "ui_resources.grd"
   outputs = [
     "grit/ui_resources.h",
@@ -27,7 +26,6 @@ grit("ui_resources_grd") {
 }
 
 grit("ui_unscaled_resources_grd") {
-  visibility = ":resources"
   source = "ui_unscaled_resources.grd"
   outputs = [
     "grit/ui_unscaled_resources.h",
@@ -36,7 +34,6 @@ grit("ui_unscaled_resources_grd") {
 }
 
 grit("webui_resources_grd") {
-  visibility = ":resources"
   source = "../webui/resources/webui_resources.grd"
   outputs = [
     "grit/webui_resources.h",

ui/shell_dialogs/BUILD.gn

@@ -66,9 +66,6 @@ component("shell_dialogs") {
   }
 }
 
-# TODO(GYP) enable shell_dialogs_unittests when media is converted.
-if (false) {
-
 test("shell_dialogs_unittests") {
   sources = [
     "select_file_dialog_win_unittest.cc",
@@ -82,5 +79,3 @@ test("shell_dialogs_unittests") {
     "//testing/gtest",
   ]
 }
-
-}  # if false

ui/wm/BUILD.gn

@@ -87,6 +87,9 @@ component("wm") {
     "//ui/gfx/geometry",
     "//ui/resources",
   ]
+
+  # http://crbug.com/408650 Circular dependency between this target and Aura.
+  allow_circular_includes_from = [ "//ui/aura" ]
 }
 
 static_library("test_support") {