Move enable_pdf to a buildflag header.

Removes the build flag from //build/config and moves it to a new .gni file in //pdf. Converts the define to a BUILDFLAG. Makes it possible to unconditionally depend on "//pdf" without checking the enable_pdf flag. This cleans up most of the callers. There is still a dependency on plugins and this assertion is moved to the top of //chrome/common/BUILD.gn. The assertion for the printing dependency on PDF is moved to the //printing/BUILD.gn so the enable_pdf flag isn't injected everywhere enable_printing is needed. Review-Url: https://codereview.chromium.org/2576573002 Cr-Commit-Position: refs/heads/master@{#438441}
Denger-Network · Dec 14, 2016 · e5cc5c6 · e5cc5c6
1 parent afd9972
commit e5cc5c6
Show file tree

Hide file tree

Showing 12 changed files with 122 additions and 109 deletions.
diff --git a/build/config/BUILD.gn b/build/config/BUILD.gn
@@ -58,9 +58,6 @@ declare_args() {
 config("feature_flags") {
   # Don't use deprecated V8 APIs anywhere.
   defines = [ "V8_DEPRECATION_WARNINGS" ]
-  if (enable_pdf) {
-    defines += [ "ENABLE_PDF=1" ]
-  }
   if (dcheck_always_on) {
     defines += [ "DCHECK_ALWAYS_ON=1" ]
   }

diff --git a/build/config/features.gni b/build/config/features.gni
@@ -21,8 +21,6 @@ if (is_android) {
 }
 
 declare_args() {
-  enable_pdf = !is_android && !is_ios && !is_chromecast
-
   # Enables Native Client support.
   # Temporarily disable nacl on arm64 linux to get rid of compilation errors.
   # TODO(mcgrathr): When mipsel-nacl-clang is available, drop the exclusion.

diff --git a/chrome/BUILD.gn b/chrome/BUILD.gn
@@ -239,13 +239,11 @@ if (!is_android && !is_mac) {
         # Needed to use the master_preferences functions
         "//chrome/installer/util:with_no_strings",
         "//content/public/app:both",
+        "//pdf",
 
         # For headless mode.
         "//headless:headless_shell_lib",
       ]
-      if (enable_plugins && enable_pdf) {
-        deps += [ "//pdf" ]
-      }
 
       public_deps = [
         ":xdg_mime",  # Needs to be public for installer to consume files.
@@ -378,13 +376,10 @@ if (is_win) {
       deps += [
         ":child_dependencies",
         "//content/public/app:both",
+        "//pdf",
       ]
     }
 
-    if (enable_plugins && enable_pdf && !is_multi_dll_chrome) {
-      deps += [ "//pdf" ]
-    }
-
     if (enable_package_mash_services) {
       deps += [ "//chrome/app/mash" ]
     }
@@ -420,6 +415,7 @@ if (is_win) {
         "//components/browser_watcher:browser_watcher_client",
         "//components/crash/content/app",
         "//content/public/app:child",
+        "//pdf",
       ]
 
       ldflags = [
@@ -439,10 +435,6 @@ if (is_win) {
         configs += [ "//build/config/win:no_incremental_linking" ]
       }
       configs += [ "//build/config/compiler/pgo:default_pgo_flags" ]
-
-      if (enable_plugins && enable_pdf) {
-        deps += [ "//pdf" ]
-      }
     }
   }
   copy("copy_first_run") {
@@ -980,6 +972,7 @@ if (is_win) {
       "//components/crash/content/app",
       "//components/policy:generated",
       "//content/public/app:both",
+      "//pdf",
       "//third_party/cld",
     ]
 
@@ -993,10 +986,6 @@ if (is_win) {
       "-ObjC",
     ]
 
-    if (enable_plugins && enable_pdf) {
-      deps += [ "//pdf" ]
-    }
-
     if (enable_package_mash_services) {
       deps += [ "//chrome/app/mash" ]
     }

diff --git a/chrome/app/BUILD.gn b/chrome/app/BUILD.gn
@@ -318,6 +318,7 @@ static_library("test_support") {
     "//components/startup_metric_utils/browser:lib",
     "//content/public/app:both",
     "//content/public/common",
+    "//pdf",
     "//ppapi/features",
     "//printing/features",
     "//ui/base",
@@ -344,10 +345,6 @@ static_library("test_support") {
     ]
   }
 
-  if (enable_pdf) {
-    deps += [ "//pdf" ]
-  }
-
   if (enable_plugins && enable_nacl) {
     deps += [
       "//components/nacl/browser",

diff --git a/chrome/app/chrome_main_delegate.cc b/chrome/app/chrome_main_delegate.cc
@@ -51,6 +51,7 @@
 #include "content/public/common/content_paths.h"
 #include "content/public/common/content_switches.h"
 #include "extensions/common/constants.h"
+#include "pdf/features.h"
 #include "ppapi/features/features.h"
 #include "printing/features/features.h"
 #include "ui/base/material_design/material_design_controller.h"
@@ -925,7 +926,7 @@ void ChromeMainDelegate::SandboxInitialized(const std::string& process_type) {
       nacl_plugin::PPP_InitializeModule,
       nacl_plugin::PPP_ShutdownModule);
 #endif
-#if BUILDFLAG(ENABLE_PLUGINS) && defined(ENABLE_PDF)
+#if BUILDFLAG(ENABLE_PLUGINS) && BUILDFLAG(ENABLE_PDF)
   ChromeContentClient::SetPDFEntryFunctions(
       chrome_pdf::PPP_GetInterface,
       chrome_pdf::PPP_InitializeModule,

diff --git a/chrome/common/BUILD.gn b/chrome/common/BUILD.gn
@@ -8,9 +8,14 @@ import("//chrome/common/features.gni")
 import("//chrome/process_version_rc_template.gni")  # For branding_file_path.
 import("//extensions/features/features.gni")
 import("//mojo/public/tools/bindings/mojom.gni")
+import("//pdf/features.gni")
 import("//ppapi/features/features.gni")
 import("//tools/grit/grit_rule.gni")
 
+if (enable_pdf) {
+  assert(enable_plugins, "PDF support requires plugins be enabled.")
+}
+
 grit("resources") {
   source = "common_resources.grd"
   use_qualified_include = true
@@ -221,6 +226,7 @@ static_library("common") {
     "//mojo/edk/system",
     "//mojo/public/cpp/bindings",
     "//net",
+    "//pdf:features",
     "//ppapi/features",
     "//printing/features",
     "//skia",

diff --git a/chrome/common/chrome_content_client.cc b/chrome/common/chrome_content_client.cc
@@ -44,6 +44,7 @@
 #include "extensions/features/features.h"
 #include "gpu/config/gpu_info.h"
 #include "net/http/http_util.h"
+#include "pdf/features.h"
 #include "ppapi/features/features.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/layout.h"
@@ -90,14 +91,14 @@
 namespace {
 
 #if BUILDFLAG(ENABLE_PLUGINS)
-#if defined(ENABLE_PDF)
+#if BUILDFLAG(ENABLE_PDF)
 const char kPDFPluginExtension[] = "pdf";
 const char kPDFPluginDescription[] = "Portable Document Format";
 const char kPDFPluginOutOfProcessMimeType[] =
     "application/x-google-chrome-pdf";
 const uint32_t kPDFPluginPermissions =
     ppapi::PERMISSION_PRIVATE | ppapi::PERMISSION_DEV;
-#endif  // defined(ENABLE_PDF)
+#endif  // BUILDFLAG(ENABLE_PDF)
 
 content::PepperPluginInfo::GetInterfaceFunc g_pdf_get_interface;
 content::PepperPluginInfo::PPP_InitializeModuleFunc g_pdf_initialize_module;
@@ -151,7 +152,7 @@ bool IsWidevineAvailable(base::FilePath* adapter_path,
 // not marked internal, aside from being automatically registered, they're just
 // regular plugins).
 void ComputeBuiltInPlugins(std::vector<content::PepperPluginInfo>* plugins) {
-#if defined(ENABLE_PDF)
+#if BUILDFLAG(ENABLE_PDF)
   content::PepperPluginInfo pdf_info;
   pdf_info.is_internal = true;
   pdf_info.is_out_of_process = true;
@@ -169,7 +170,7 @@ void ComputeBuiltInPlugins(std::vector<content::PepperPluginInfo>* plugins) {
   pdf_info.internal_entry_points.shutdown_module = g_pdf_shutdown_module;
   pdf_info.permissions = kPDFPluginPermissions;
   plugins->push_back(pdf_info);
-#endif  // defined(ENABLE_PDF)
+#endif  // BUILDFLAG(ENABLE_PDF)
 
 #if !defined(DISABLE_NACL)
   // Handle Native Client just like the PDF plugin. This means that it is

diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
@@ -130,6 +130,7 @@ static_library("test_support") {
     "//media:media_features",
     "//net",
     "//net:test_support",
+    "//pdf",
     "//ppapi/features",
     "//printing/features",
     "//skia",
@@ -227,10 +228,6 @@ static_library("test_support") {
     ]
   }
 
-  if (enable_plugins && enable_pdf) {
-    public_deps += [ "//pdf" ]
-  }
-
   if (use_ash) {
     sources += [
       "base/default_ash_event_generator_delegate.cc",

diff --git a/pdf/BUILD.gn b/pdf/BUILD.gn
@@ -2,86 +2,99 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import("//build/buildflag_header.gni")
 import("//build/config/features.gni")
+import("//pdf/features.gni")
 import("//testing/test.gni")
 import("//third_party/pdfium/pdfium.gni")
 
-assert(enable_pdf)
-
-pdf_engine = 0  # 0 PDFium
-
-static_library("pdf") {
-  deps = [
-    "//base",
-    "//gin",
-    "//net",
-    "//ppapi/cpp:objects",
-    "//ppapi/cpp/private:internal_module",
-    "//ui/base",
-  ]
+# Generate a buildflag header for compile-time checking of PDF support.
+buildflag_header("features") {
+  header = "features.h"
+  flags = [ "ENABLE_PDF=$enable_pdf" ]
+}
 
-  sources = [
-    "chunk_stream.cc",
-    "chunk_stream.h",
-    "document_loader.cc",
-    "document_loader.h",
-    "draw_utils.cc",
-    "draw_utils.h",
-    "out_of_process_instance.cc",
-    "out_of_process_instance.h",
-    "paint_aggregator.cc",
-    "paint_aggregator.h",
-    "paint_manager.cc",
-    "paint_manager.h",
-    "pdf.cc",
-    "pdf.h",
-    "pdf_engine.h",
-    "preview_mode_client.cc",
-    "preview_mode_client.h",
-  ]
+if (enable_pdf) {
+  pdf_engine = 0  # 0 PDFium
 
-  if (pdf_engine == 0) {
-    deps += [
-      "//pdf/pdfium/fuzzers",
-      "//third_party/pdfium",
+  static_library("pdf") {
+    deps = [
+      "//base",
+      "//gin",
+      "//net",
+      "//ppapi/cpp:objects",
+      "//ppapi/cpp/private:internal_module",
+      "//ui/base",
     ]
 
-    sources += [
-      "pdfium/pdfium_api_string_buffer_adapter.cc",
-      "pdfium/pdfium_api_string_buffer_adapter.h",
-      "pdfium/pdfium_assert_matching_enums.cc",
-      "pdfium/pdfium_engine.cc",
-      "pdfium/pdfium_engine.h",
-      "pdfium/pdfium_mem_buffer_file_read.cc",
-      "pdfium/pdfium_mem_buffer_file_read.h",
-      "pdfium/pdfium_mem_buffer_file_write.cc",
-      "pdfium/pdfium_mem_buffer_file_write.h",
-      "pdfium/pdfium_page.cc",
-      "pdfium/pdfium_page.h",
-      "pdfium/pdfium_range.cc",
-      "pdfium/pdfium_range.h",
+    sources = [
+      "chunk_stream.cc",
+      "chunk_stream.h",
+      "document_loader.cc",
+      "document_loader.h",
+      "draw_utils.cc",
+      "draw_utils.h",
+      "out_of_process_instance.cc",
+      "out_of_process_instance.h",
+      "paint_aggregator.cc",
+      "paint_aggregator.h",
+      "paint_manager.cc",
+      "paint_manager.h",
+      "pdf.cc",
+      "pdf.h",
+      "pdf_engine.h",
+      "preview_mode_client.cc",
+      "preview_mode_client.h",
     ]
-  }
 
-  defines = [ "PDFIUM_PRINT_TEXT_WITH_GDI" ]
-  if (pdf_enable_xfa) {
-    defines += [ "PDF_ENABLE_XFA" ]
+    if (pdf_engine == 0) {
+      deps += [
+        "//pdf/pdfium/fuzzers",
+        "//third_party/pdfium",
+      ]
+
+      sources += [
+        "pdfium/pdfium_api_string_buffer_adapter.cc",
+        "pdfium/pdfium_api_string_buffer_adapter.h",
+        "pdfium/pdfium_assert_matching_enums.cc",
+        "pdfium/pdfium_engine.cc",
+        "pdfium/pdfium_engine.h",
+        "pdfium/pdfium_mem_buffer_file_read.cc",
+        "pdfium/pdfium_mem_buffer_file_read.h",
+        "pdfium/pdfium_mem_buffer_file_write.cc",
+        "pdfium/pdfium_mem_buffer_file_write.h",
+        "pdfium/pdfium_page.cc",
+        "pdfium/pdfium_page.h",
+        "pdfium/pdfium_range.cc",
+        "pdfium/pdfium_range.h",
+      ]
+    }
+
+    defines = [ "PDFIUM_PRINT_TEXT_WITH_GDI" ]
+    if (pdf_enable_xfa) {
+      defines += [ "PDF_ENABLE_XFA" ]
+    }
   }
-}
 
-test("pdf_unittests") {
-  sources = [
-    "chunk_stream_unittest.cc",
-    "run_all_unittests.cc",
-  ]
+  test("pdf_unittests") {
+    sources = [
+      "chunk_stream_unittest.cc",
+      "run_all_unittests.cc",
+    ]
 
-  deps = [
-    ":pdf",
-    "//base",
-    "//base/test:test_support",
-    "//ppapi/c",
-    "//ppapi/cpp",
-    "//testing/gmock",
-    "//testing/gtest",
-  ]
+    deps = [
+      ":pdf",
+      "//base",
+      "//base/test:test_support",
+      "//ppapi/c",
+      "//ppapi/cpp",
+      "//testing/gmock",
+      "//testing/gtest",
+    ]
+  }
+} else {
+  # Dummy group when PDF support is disabled so targets can unconditionally
+  # depend on it.
+  group("pdf") {
+  }
 }
diff --git a/pdf/features.gni b/pdf/features.gni
@@ -0,0 +1,13 @@
+# Copyright 2016 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/chromecast_build.gni")
+
+# Most build code won't need to include this file. Instead you can
+# unconditionally depend on "//pdf" which will be a no-op when PDF support is
+# disabled.
+
+declare_args() {
+  enable_pdf = !is_android && !is_ios && !is_chromecast
+}