Deduplicate Monochrome locale .paks

ui/base/resource/resource_bundle_android.cc

 // Copyright (c) 2012 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.
 
 #include "ui/base/resource/resource_bundle_android.h"
 
 #include "base/android/apk_assets.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/path_service.h"
 #include "jni/ResourceBundle_jni.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/data_pack.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/ui_base_paths.h"
 
 namespace ui {
 
 namespace {
 
 bool g_locale_paks_in_apk = false;
+bool g_load_secondary_locale_paks = false;
 // It is okay to cache and share these file descriptors since the
 // ResourceBundle singleton never closes the handles.
 int g_chrome_100_percent_fd = -1;
 int g_resources_pack_fd = -1;
 int g_locale_pack_fd = -1;
+int g_secondary_locale_pack_fd = -1;
 base::MemoryMappedFile::Region g_chrome_100_percent_region;
 base::MemoryMappedFile::Region g_resources_pack_region;
 base::MemoryMappedFile::Region g_locale_pack_region;
+base::MemoryMappedFile::Region g_secondary_locale_pack_region;
 
 bool LoadFromApkOrFile(const char* apk_path,
                        const base::FilePath* disk_path,
                        int* fd_out,
-                       base::MemoryMappedFile::Region* region_out) {
+                       base::MemoryMappedFile::Region* out_region) {

[agrieve, 2017/06/29 01:09:53]

nit: this now doesn't match the other out param (fd_out).

[F, 2017/06/29 18:31:20]

Done. I put "out" in front of "fd/region" because that's how
GetCommonResourcesPackFd etc. do it. 

   DCHECK_EQ(*fd_out, -1) << "Attempt to load " << apk_path << " twice.";
   if (apk_path != nullptr) {
-    *fd_out = base::android::OpenApkAsset(apk_path, region_out);
+    *fd_out = base::android::OpenApkAsset(apk_path, out_region);
   }
   // For unit tests, the file exists on disk.
   if (*fd_out < 0 && disk_path != nullptr) {
     int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
     *fd_out = base::File(*disk_path, flags).TakePlatformFile();
-    *region_out = base::MemoryMappedFile::Region::kWholeFile;
+    *out_region = base::MemoryMappedFile::Region::kWholeFile;
   }
   bool success = *fd_out >= 0;
   if (!success) {
     LOG(ERROR) << "Failed to open pak file: " << apk_path;
   }
   return success;
 }
 
+int LoadLocalePakFromApk(const std::string& app_locale,

[agrieve, 2017/06/29 01:09:53]

nit: might be nice to match the signature of the above method (two outs and
returning a bool). Or change the other one to match this one.

[F, 2017/06/29 18:31:21]

Resolved offline.

+                         base::MemoryMappedFile::Region* out_region) {
+  std::string locale_path_within_apk =
+      GetPathForAndroidLocalePakWithinApk(app_locale);
+  if (locale_path_within_apk.empty()) {
+    LOG(WARNING) << "locale_path_within_apk.empty() for locale "

[agrieve, 2017/06/29 01:09:53]

nit: Probably worth using ERROR here

[F, 2017/06/29 18:31:20]

Done.

+                 << app_locale;
+    return -1;
+  }
+  return base::android::OpenApkAsset(locale_path_within_apk, out_region);
+}
+
+std::unique_ptr<DataPack> LoadDataPackFromLocalePak(
+    int locale_pack_fd,
+    const base::MemoryMappedFile::Region& region) {
+  std::unique_ptr<DataPack> data_pack(new DataPack(SCALE_FACTOR_100P));
+  if (!data_pack->LoadFromFileRegion(base::File(locale_pack_fd), region)) {
+    LOG(ERROR) << "failed to load locale.pak";

[agrieve, 2017/06/29 01:09:53]

nit: just << to the NOTREACHED().

[F, 2017/06/29 18:31:21]

Resolved offline.

+    NOTREACHED();
+    return nullptr;
+  }
+  return data_pack;
+}
+
 }  // namespace
 
 void ResourceBundle::LoadCommonResources() {
   base::FilePath disk_path;
   PathService::Get(ui::DIR_RESOURCE_PAKS_ANDROID, &disk_path);
   disk_path = disk_path.AppendASCII("chrome_100_percent.pak");
   if (LoadFromApkOrFile("assets/chrome_100_percent.pak",
                         &disk_path,
                         &g_chrome_100_percent_fd,
                         &g_chrome_100_percent_region)) {
     AddDataPackFromFileRegion(base::File(g_chrome_100_percent_fd),
                               g_chrome_100_percent_region, SCALE_FACTOR_100P);
   }
 }
 
 bool ResourceBundle::LocaleDataPakExists(const std::string& locale) {
   if (g_locale_paks_in_apk) {
     return !GetPathForAndroidLocalePakWithinApk(locale).empty();
   }
   return !GetLocaleFilePath(locale, true).empty();
 }
 
 std::string ResourceBundle::LoadLocaleResources(
     const std::string& pref_locale) {
-  DCHECK(!locale_resources_data_.get()) << "locale.pak already loaded";
+  DCHECK(!locale_resources_data_.get() &&
+         !secondary_locale_resources_data_.get())
+             << "locale.pak already loaded";
   if (g_locale_pack_fd != -1) {
     LOG(WARNING)
         << "Unexpected (outside of tests): Loading a second locale pak file.";
   }
   std::string app_locale = l10n_util::GetApplicationLocale(pref_locale);
+
+  // Load primary locale .pak file.
   if (g_locale_paks_in_apk) {
-    std::string locale_path_within_apk =
-        GetPathForAndroidLocalePakWithinApk(app_locale);
-    if (locale_path_within_apk.empty()) {
-      LOG(WARNING) << "locale_path_within_apk.empty() for locale "
-                   << app_locale;
-      return std::string();
-    }
-    g_locale_pack_fd = base::android::OpenApkAsset(locale_path_within_apk,
-                                                   &g_locale_pack_region);
+    g_locale_pack_fd = LoadLocalePakFromApk(app_locale, &g_locale_pack_region);
   } else {
     base::FilePath locale_file_path = GetOverriddenPakPath();
     if (locale_file_path.empty())
       locale_file_path = GetLocaleFilePath(app_locale, true);
 
     if (locale_file_path.empty()) {
       // It's possible that there is no locale.pak.
       LOG(WARNING) << "locale_file_path.empty() for locale " << app_locale;
       return std::string();
     }
     int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
     g_locale_pack_fd = base::File(locale_file_path, flags).TakePlatformFile();
     g_locale_pack_region = base::MemoryMappedFile::Region::kWholeFile;
   }
 
-  std::unique_ptr<DataPack> data_pack(new DataPack(SCALE_FACTOR_100P));
-  if (!data_pack->LoadFromFileRegion(base::File(g_locale_pack_fd),
-                                     g_locale_pack_region)) {
-    LOG(ERROR) << "failed to load locale.pak";
-    NOTREACHED();
+  locale_resources_data_ = LoadDataPackFromLocalePak(
+      g_locale_pack_fd, g_locale_pack_region);
+
+  if (!locale_resources_data_.get())
     return std::string();
+
+  // Load secondary locale .pak file if it exists. For debug build monochrome,
+  // a secondary locale pak will always be loaded; however, it should be
+  // unnecessary for loading locale resources.

[agrieve, 2017/06/29 01:09:53]

nit: say why it's unnecessary.

[F, 2017/06/29 18:31:21]

Done.

+  if (g_load_secondary_locale_paks) {
+    g_secondary_locale_pack_fd = LoadLocalePakFromApk(
+        app_locale, &g_secondary_locale_pack_region);
+
+    secondary_locale_resources_data_ = LoadDataPackFromLocalePak(
+        g_secondary_locale_pack_fd, g_secondary_locale_pack_region);
+
+    if (!secondary_locale_resources_data_.get())

[agrieve, 2017/06/29 01:09:53]

nit: probably fine to omit this early return since we'll at least have the
primary pak?

[F, 2017/06/29 18:31:21]

Resolved offline.

+      return std::string();
   }
 
-  locale_resources_data_ = std::move(data_pack);
   return app_locale;
 }
 
 gfx::Image& ResourceBundle::GetNativeImageNamed(int resource_id) {
   return GetImageNamed(resource_id);
 }
 
 void SetLocalePaksStoredInApk(bool value) {
   g_locale_paks_in_apk = value;
 }
 
+void SetLoadSecondaryLocalePaks(bool value) {
+  g_load_secondary_locale_paks = value;
+}
+
 void LoadMainAndroidPackFile(const char* path_within_apk,
                              const base::FilePath& disk_file_path) {
   if (LoadFromApkOrFile(path_within_apk,
                         &disk_file_path,
                         &g_resources_pack_fd,
                         &g_resources_pack_region)) {
     ResourceBundle::GetSharedInstance().AddDataPackFromFileRegion(
         base::File(g_resources_pack_fd), g_resources_pack_region,
         SCALE_FACTOR_NONE);
   }
@@ skipping 10 matching lines @@
   *out_region = g_chrome_100_percent_region;
   return g_chrome_100_percent_fd;
 }
 
 int GetLocalePackFd(base::MemoryMappedFile::Region* out_region) {
   DCHECK_GE(g_locale_pack_fd, 0);
   *out_region = g_locale_pack_region;
   return g_locale_pack_fd;
 }
 
+int GetSecondaryLocalePackFd(base::MemoryMappedFile::Region* out_region) {
+  *out_region = g_secondary_locale_pack_region;
+  return g_secondary_locale_pack_fd;
+}
+
 std::string GetPathForAndroidLocalePakWithinApk(const std::string& locale) {
   JNIEnv* env = base::android::AttachCurrentThread();
   base::android::ScopedJavaLocalRef<jstring> ret =
       Java_ResourceBundle_getLocalePakResourcePath(
           env, base::android::ConvertUTF8ToJavaString(env, locale));
   if (ret.obj() == nullptr) {
     return std::string();
   }
   return base::android::ConvertJavaStringToUTF8(env, ret.obj());
 }
 
 float GetPrimaryDisplayScale() {
   return Java_ResourceBundle_getPrimaryDisplayScale(
       base::android::AttachCurrentThread());
 }
 
 }  // namespace ui