Revert of Deduplicate Monochrome locale .paks (patchset chromium#4 id…

…:180001 of https://codereview.chromium.org/2933343002/ ) Reason for revert: Chrome is now missing 5 locale strings. The reason is that these strings are believed to be in WebView locales because they're whitelisted, but since WebView uses multiple whitelists to pack its locale strings, these strings are actually not contained by WebView Original issue's description: > Deduplicate Monochrome locale .paks > > This CL removes WebView strings from Chrome locale .paks in Monochrome > using a generated resource whitelist. While WebView's locale resource > logic remains the same, Chrome in Monochrome now loads a secondary > locale pack file as a fallback when a string cannot be found in the > primary locale pack file. > > This CL reduces binary size by over 400KB. > > BUG=724110 > > Review-Url: https://codereview.chromium.org/2933343002 > Cr-Commit-Position: refs/heads/master@{#485635} > Committed: https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc1d2ea64497e6 TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=724110, 741798 Review-Url: https://codereview.chromium.org/2980773002 Cr-Commit-Position: refs/heads/master@{#486397}
hylschwy · Jul 13, 2017 · df9f33f · df9f33f
1 parent c0ce314
commit df9f33f
Show file tree

Hide file tree

Showing 12 changed files with 57 additions and 259 deletions.
diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn
@@ -41,8 +41,6 @@ app_hooks_impl = "java/src/org/chromium/chrome/browser/AppHooksImpl.java"
 if (enable_resource_whitelist_generation) {
   monochrome_resource_whitelist =
       "$target_gen_dir/monochrome_resource_whitelist.txt"
-  monochrome_locale_whitelist =
-      "$target_gen_dir/monochrome_locale_whitelist.txt"
 }
 
 jinja_template("chrome_public_android_manifest") {
@@ -732,38 +730,9 @@ if (current_toolchain == default_toolchain) {
               "/libmonochrome$shlib_extension.whitelist"
       output = monochrome_resource_whitelist
     }
-
-    action("monochrome_locale_whitelist") {
-      script = "//tools/resources/filter_resource_whitelist.py"
-
-      _system_webview_pak_whitelist =
-          "$root_gen_dir/android_webview/system_webview_pak_whitelist.txt"
-
-      inputs = [
-        monochrome_resource_whitelist,
-        _system_webview_pak_whitelist,
-      ]
-
-      outputs = [
-        monochrome_locale_whitelist,
-      ]
-
-      deps = [
-        ":monochrome_resource_whitelist",
-        "//android_webview:system_webview_pak_whitelist",
-      ]
-
-      args = [
-        "--input",
-        rebase_path(monochrome_resource_whitelist, root_build_dir),
-        "--filter",
-        rebase_path(_system_webview_pak_whitelist, root_build_dir),
-        "--output",
-        rebase_path(monochrome_locale_whitelist, root_build_dir),
-      ]
-    } 
   }
 
+  # This target does not output locale paks.
   chrome_paks("monochrome_paks") {
     output_dir = "$target_gen_dir/$target_name"
 
@@ -772,26 +741,13 @@ if (current_toolchain == default_toolchain) {
       "//android_webview:generate_aw_resources",
     ]
 
+    exclude_locale_paks = true
+
     if (enable_resource_whitelist_generation) {
       repack_whitelist = monochrome_resource_whitelist
       deps += [ ":monochrome_resource_whitelist" ]
-      locale_whitelist = monochrome_locale_whitelist
-      deps += [ ":monochrome_locale_whitelist" ]
     }
   }
-
-  # This target is separate from monochrome_pak_assets because it does not
-  # disable compression.
-  android_assets("monochrome_locale_pak_assets") {
-    sources = []
-    foreach(_locale, locales - android_chrome_omitted_locales) {
-      sources += [ "$target_gen_dir/monochrome_paks/locales/$_locale.pak" ]
-    }
-
-    deps = [
-      ":monochrome_paks",
-    ]
-  }
 
   # This target explicitly includes locale paks via deps.
   android_assets("monochrome_pak_assets") {
@@ -802,7 +758,7 @@ if (current_toolchain == default_toolchain) {
     disable_compression = true
 
     deps = [
-      ":monochrome_locale_pak_assets",
+      ":chrome_public_locale_pak_assets",
       ":monochrome_paks",
       "//android_webview:locale_pak_assets",
     ]

diff --git a/chrome/app/chrome_main_delegate.cc b/chrome/app/chrome_main_delegate.cc
@@ -877,16 +877,6 @@ void ChromeMainDelegate::PreSandboxStartup() {
     ResourceBundle::InitSharedInstanceWithPakFileRegion(base::File(pak_fd),
                                                         pak_region);
 
-    // Load secondary locale .pak file if it exists.
-    pak_fd = global_descriptors->MaybeGet(kAndroidSecondaryLocalePakDescriptor);
-    if (pak_fd != -1) {
-      pak_region = global_descriptors->GetRegion(
-          kAndroidSecondaryLocalePakDescriptor);
-      ResourceBundle::GetSharedInstance().
-          LoadSecondaryLocaleDataWithPakFileRegion(
-              base::File(pak_fd), pak_region);
-    }
-
     int extra_pak_keys[] = {
       kAndroidChrome100PercentPakDescriptor,
       kAndroidUIResourcesPakDescriptor,

diff --git a/chrome/browser/chrome_browser_main_android.cc b/chrome/browser/chrome_browser_main_android.cc
@@ -80,10 +80,6 @@ int ChromeBrowserMainPartsAndroid::PreCreateThreads() {
             crash_dump_dir, kAndroidMinidumpDescriptor));
   }
 
-  // Auto-detect based on en-US whether secondary locale .pak files exist.
-  ui::SetLoadSecondaryLocalePaks(
-      !ui::GetPathForAndroidLocalePakWithinApk("en-US").empty());
-
   return ChromeBrowserMainParts::PreCreateThreads();
 }
 

diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -2815,12 +2815,6 @@ void ChromeContentBrowserClient::GetAdditionalMappedFilesForChildProcess(
   fd = ui::GetLocalePackFd(&region);
   mappings->ShareWithRegion(kAndroidLocalePakDescriptor, fd, region);
 
-  // Optional secondary locale .pak file.
-  fd = ui::GetSecondaryLocalePackFd(&region);
-  if (fd != -1) {
-    mappings->ShareWithRegion(kAndroidSecondaryLocalePakDescriptor, fd, region);
-  }
-
   breakpad::CrashDumpObserver::GetInstance()->BrowserChildProcessStarted(
       child_process_id, mappings);
 

diff --git a/chrome/chrome_paks.gni b/chrome/chrome_paks.gni
@@ -184,7 +184,7 @@ template("chrome_extra_paks") {
 #   output_dir [required]: Directory to output .pak files. Locale .pak files
 #       will always be place in $output_dir/locales
 #   additional_extra_paks: List of extra .pak sources for resources.pak.
-#   locale_whitelist: if set, override repack_whitelist for locale .pak files.
+#   exclude_locale_paks: if set to true, skip chrome_repack_locales.
 #   copy_data_to_bundle:
 #   deps:
 #   output_dir:
@@ -234,26 +234,24 @@ template("chrome_paks") {
     }
   }
 
-  chrome_repack_locales("${target_name}_locales") {
-    forward_variables_from(invoker,
-                           [
-                             "copy_data_to_bundle",
-                             "deps",
-                             "visibility",
-                           ])
-    if (defined(invoker.locale_whitelist)) {
-      repack_whitelist = invoker.locale_whitelist
-    } else if (defined(invoker.repack_whitelist)) {
-      repack_whitelist = invoker.repack_whitelist
-    }
+  if (!defined(invoker.exclude_locale_paks) || !invoker.exclude_locale_paks) {
+    chrome_repack_locales("${target_name}_locales") {
+      forward_variables_from(invoker,
+                             [
+                               "copy_data_to_bundle",
+                               "deps",
+                               "repack_whitelist",
+                               "visibility",
+                             ])
 
-    input_locales = locales
-    output_dir = "${invoker.output_dir}/locales"
+      input_locales = locales
+      output_dir = "${invoker.output_dir}/locales"
 
-    if (is_mac) {
-      output_locales = locales_as_mac_outputs
-    } else {
-      output_locales = locales
+      if (is_mac) {
+        output_locales = locales_as_mac_outputs
+      } else {
+        output_locales = locales
+      }
     }
   }
 
@@ -262,8 +260,10 @@ template("chrome_paks") {
     public_deps = [
       ":${target_name}_100_percent",
       ":${target_name}_extra",
-      ":${target_name}_locales",
     ]
+    if (!defined(invoker.exclude_locale_paks) || !invoker.exclude_locale_paks) {
+      public_deps += [ ":${target_name}_locales" ]
+    }
     if (enable_hidpi) {
       public_deps += [ ":${target_name}_200_percent" ]
     }

diff --git a/chrome/common/descriptors_android.h b/chrome/common/descriptors_android.h
@@ -11,7 +11,6 @@
 enum {
 #if defined(OS_ANDROID)
   kAndroidLocalePakDescriptor = kContentIPCDescriptorMax + 1,
-  kAndroidSecondaryLocalePakDescriptor,
   kAndroidChrome100PercentPakDescriptor,
   kAndroidUIResourcesPakDescriptor,
   kAndroidMinidumpDescriptor,

diff --git a/tools/resources/OWNERS b/tools/resources/OWNERS
@@ -1,4 +1,2 @@
 per-file generate_resource_whitelist.*=agrieve@chromium.org
 per-file generate_resource_whitelist.*=estevenson@chromium.org
-per-file filter_resource_whitelist.*=agrieve@chromium.org
-per-file filter_resource_whitelist.*=zpeng@chromium.org
diff --git a/tools/resources/filter_resource_whitelist.py b/tools/resources/filter_resource_whitelist.py
diff --git a/ui/base/resource/resource_bundle.cc b/ui/base/resource/resource_bundle.cc
@@ -211,7 +211,7 @@ void ResourceBundle::InitSharedInstanceWithPakFileRegion(
     base::File pak_file,
     const base::MemoryMappedFile::Region& region) {
   InitSharedInstance(NULL);
-  auto data_pack = base::MakeUnique<DataPack>(SCALE_FACTOR_100P);
+  std::unique_ptr<DataPack> data_pack(new DataPack(SCALE_FACTOR_100P));
   if (!data_pack->LoadFromFileRegion(std::move(pak_file), region)) {
     NOTREACHED() << "failed to load pak file";
     return;
@@ -248,17 +248,6 @@ ResourceBundle& ResourceBundle::GetSharedInstance() {
   return *g_shared_instance_;
 }
 
-void ResourceBundle::LoadSecondaryLocaleDataWithPakFileRegion(
-    base::File pak_file,
-    const base::MemoryMappedFile::Region& region) {
-  auto data_pack = base::MakeUnique<DataPack>(SCALE_FACTOR_100P);
-  if (!data_pack->LoadFromFileRegion(std::move(pak_file), region)) {
-    NOTREACHED() << "failed to load secondary pak file";
-    return;
-  }
-  secondary_locale_resources_data_ = std::move(data_pack);
-}
-
 #if !defined(OS_ANDROID)
 bool ResourceBundle::LocaleDataPakExists(const std::string& locale) {
   return !GetLocaleFilePath(locale, true).empty();
@@ -400,7 +389,6 @@ void ResourceBundle::LoadTestResources(const base::FilePath& path,
 
 void ResourceBundle::UnloadLocaleResources() {
   locale_resources_data_.reset();
-  secondary_locale_resources_data_.reset();
 }
 
 void ResourceBundle::OverrideLocalePakForTest(const base::FilePath& pak_path) {
@@ -563,27 +551,20 @@ base::string16 ResourceBundle::GetLocalizedString(int message_id) {
   }
 
   base::StringPiece data;
-  ResourceHandle::TextEncodingType encoding =
-      locale_resources_data_->GetTextEncodingType();
   if (!locale_resources_data_->GetStringPiece(static_cast<uint16_t>(message_id),
                                               &data)) {
-    if (secondary_locale_resources_data_.get() &&
-        secondary_locale_resources_data_->GetStringPiece(
-            static_cast<uint16_t>(message_id), &data)) {
-      // Fall back on the secondary locale pak if it exists.
-      encoding = secondary_locale_resources_data_->GetTextEncodingType();
-    } else {
-      // Fall back on the main data pack (shouldn't be any strings here except
-      // in unittests).
-      data = GetRawDataResource(message_id);
-      if (data.empty()) {
-        NOTREACHED() << "unable to find resource: " << message_id;
-        return base::string16();
-      }
+    // Fall back on the main data pack (shouldn't be any strings here except in
+    // unittests).
+    data = GetRawDataResource(message_id);
+    if (data.empty()) {
+      NOTREACHED() << "unable to find resource: " << message_id;
+      return base::string16();
     }
   }
 
   // Strings should not be loaded from a data pack that contains binary data.
+  ResourceHandle::TextEncodingType encoding =
+      locale_resources_data_->GetTextEncodingType();
   DCHECK(encoding == ResourceHandle::UTF16 || encoding == ResourceHandle::UTF8)
       << "requested localized string from binary pack file";
 
@@ -603,20 +584,12 @@ base::RefCountedMemory* ResourceBundle::LoadLocalizedResourceBytes(
   {
     base::AutoLock lock_scope(*locale_resources_data_lock_);
     base::StringPiece data;
-
     if (locale_resources_data_.get() &&
         locale_resources_data_->GetStringPiece(
             static_cast<uint16_t>(resource_id), &data) &&
         !data.empty()) {
       return new base::RefCountedStaticMemory(data.data(), data.length());
     }
-
-    if (secondary_locale_resources_data_.get() &&
-        secondary_locale_resources_data_->GetStringPiece(
-            static_cast<uint16_t>(resource_id), &data) &&
-        !data.empty()) {
-      return new base::RefCountedStaticMemory(data.data(), data.length());
-    }
   }
   // Release lock_scope and fall back to main data pack.
   return LoadDataResourceBytes(resource_id);

diff --git a/ui/base/resource/resource_bundle.h b/ui/base/resource/resource_bundle.h
@@ -151,11 +151,6 @@ class UI_BASE_EXPORT ResourceBundle {
   // Return the global resource loader instance.
   static ResourceBundle& GetSharedInstance();
 
-  // Loads a secondary locale data pack using the given file region.
-  void LoadSecondaryLocaleDataWithPakFileRegion(
-      base::File pak_file,
-      const base::MemoryMappedFile::Region& region);
-
   // Check if the .pak for the given locale exists.
   bool LocaleDataPakExists(const std::string& locale);
 
@@ -404,7 +399,6 @@ class UI_BASE_EXPORT ResourceBundle {
 
   // Handles for data sources.
   std::unique_ptr<ResourceHandle> locale_resources_data_;
-  std::unique_ptr<ResourceHandle> secondary_locale_resources_data_;
   std::vector<std::unique_ptr<ResourceHandle>> data_packs_;
 
   // The maximum scale factor currently loaded.