Revert of Remove extraneous GetValidLocales calls. (patchset #6 id:100001 of https://codereview.chromium.org/1281523005/ )

Reason for revert:
Test breakage in build http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29/builds/46007 .

Stack traces point to extension_l10n_util.

Original issue's description:
> Remove extraneous GetValidLocales calls.
>
> GetValidLocales is already called on first extension install. There's no need to check the locales again (i.e. on extension loading) until a new install. GetValidLocales can be costly since it stats all subdirectories and messages.json of an extension's _locales subfolder.
>
> BUG=516745
>
> Committed: https://crrev.com/0db5da63004e13f458c5a76391da1b309222d71e
> Cr-Commit-Position: refs/heads/master@{#343805}

TBR=asargent@chromium.org,erikchen@chromium.org,robliao@chromium.org,sydli@google.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=516745

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

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

4 files changed, 34 insertions, 23 deletions

diff --git a/extensions/common/extension_l10n_util.cc b/extensions/common/extension_l10n_util.cc
index dd96b07..dbdd5d9 100644
--- a/extensions/common/extension_l10n_util.cc
+++ b/extensions/common/extension_l10n_util.cc
@@ -351,7 +351,6 @@ bool GetValidLocales(const base::FilePath& locale_path,
     }
     if (!AddLocale(
             chrome_locales, locale_folder, locale_name, valid_locales, error)) {
-      valid_locales->clear();
       return false;
     }
   }
@@ -368,6 +367,7 @@ extensions::MessageBundle* LoadMessageCatalogs(
     const base::FilePath& locale_path,
     const std::string& default_locale,
     const std::string& application_locale,
+    const std::set<std::string>& valid_locales,
     std::string* error) {
   std::vector<std::string> all_fallback_locales;
   GetAllFallbackLocales(
@@ -376,9 +376,7 @@ extensions::MessageBundle* LoadMessageCatalogs(
   std::vector<linked_ptr<base::DictionaryValue> > catalogs;
   for (size_t i = 0; i < all_fallback_locales.size(); ++i) {
     // Skip all parent locales that are not supplied.
-    base::FilePath this_locale_path =
-        locale_path.AppendASCII(all_fallback_locales[i]);
-    if (!base::PathExists(this_locale_path))
+    if (valid_locales.find(all_fallback_locales[i]) == valid_locales.end())
       continue;
     linked_ptr<base::DictionaryValue> catalog(
         LoadMessageFile(locale_path, all_fallback_locales[i], error));
diff --git a/extensions/common/extension_l10n_util.h b/extensions/common/extension_l10n_util.h
index 0f7f8c9..b52a7e6 100644
--- a/extensions/common/extension_l10n_util.h
+++ b/extensions/common/extension_l10n_util.h
@@ -74,13 +74,15 @@ void GetAllFallbackLocales(const std::string& application_locale,
                            const std::string& default_locale,
                            std::vector<std::string>* all_fallback_locales);
 
-// Fill |valid_locales| with all valid locales under |locale_path|.
-// |valid_locales| is the intersection of the set of locales supported by
-// Chrome and the set of locales specified by |locale_path|.
-// Returns true if vaild_locales contains at least one locale, false otherwise.
-// |error| contains an error message when a locale is corrupt or missing.
+// Adds valid locales to the extension.
+// 1. Do nothing if _locales directory is missing (not an error).
+// 2. Get list of Chrome locales.
+// 3. Enumerate all subdirectories of _locales directory.
+// 4. Intersect both lists, and add intersection to the extension.
+// Returns false if any of supplied locales don't match chrome list of locales.
+// Fills out error with offending locale name.
 bool GetValidLocales(const base::FilePath& locale_path,
-                     std::set<std::string>* valid_locales,
+                     std::set<std::string>* locales,
                      std::string* error);
 
 // Loads messages file for default locale, and application locales (application
@@ -92,6 +94,7 @@ extensions::MessageBundle* LoadMessageCatalogs(
     const base::FilePath& locale_path,
     const std::string& default_locale,
     const std::string& app_locale,
+    const std::set<std::string>& valid_locales,
     std::string* error);
 
 // Loads message catalogs for all locales to check for validity.
diff --git a/extensions/common/extension_l10n_util_unittest.cc b/extensions/common/extension_l10n_util_unittest.cc
index d269501..03feeab 100644
--- a/extensions/common/extension_l10n_util_unittest.cc
+++ b/extensions/common/extension_l10n_util_unittest.cc
@@ -128,8 +128,12 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsValidFallback) {
       install_dir.AppendASCII("extension_with_locales").Append(kLocaleFolder);
 
   std::string error;
+  std::set<std::string> locales;
+  EXPECT_TRUE(
+      extension_l10n_util::GetValidLocales(install_dir, &locales, &error));
+
   scoped_ptr<MessageBundle> bundle(extension_l10n_util::LoadMessageCatalogs(
-      install_dir, "sr", "en_US", &error));
+      install_dir, "sr", "en_US", locales, &error));
   ASSERT_FALSE(NULL == bundle.get());
   EXPECT_TRUE(error.empty());
   EXPECT_EQ("Color", bundle->GetL10nMessage("color"));
@@ -142,12 +146,13 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsMissingFiles) {
 
   base::FilePath src_path = temp.path().Append(kLocaleFolder);
   ASSERT_TRUE(base::CreateDirectory(src_path));
-  ASSERT_TRUE(base::CreateDirectory(src_path.AppendASCII("en")));
-  ASSERT_TRUE(base::CreateDirectory(src_path.AppendASCII("sr")));
 
+  std::set<std::string> valid_locales;
+  valid_locales.insert("sr");
+  valid_locales.insert("en");
   std::string error;
-  EXPECT_TRUE(NULL == extension_l10n_util::LoadMessageCatalogs(src_path, "en",
-                                                               "sr", &error));
+  EXPECT_TRUE(NULL == extension_l10n_util::LoadMessageCatalogs(
+                          src_path, "en", "sr", valid_locales, &error));
   EXPECT_FALSE(error.empty());
 }
 
@@ -165,9 +170,12 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsBadJSONFormat) {
   base::FilePath messages_file = locale.Append(kMessagesFilename);
   ASSERT_TRUE(base::WriteFile(messages_file, data.c_str(), data.length()));
 
+  std::set<std::string> valid_locales;
+  valid_locales.insert("sr");
+  valid_locales.insert("en_US");
   std::string error;
   EXPECT_TRUE(NULL == extension_l10n_util::LoadMessageCatalogs(
-                          src_path, "en_US", "sr", &error));
+                          src_path, "en_US", "sr", valid_locales, &error));
   EXPECT_EQ(ErrorUtils::FormatErrorMessage(
                 errors::kLocalesInvalidLocale,
                 base::UTF16ToUTF8(messages_file.LossyDisplayName()),
@@ -197,11 +205,15 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsDuplicateKeys) {
   ASSERT_TRUE(base::WriteFile(
       locale_2.Append(kMessagesFilename), data.c_str(), data.length()));
 
+  std::set<std::string> valid_locales;
+  valid_locales.insert("sr");
+  valid_locales.insert("en");
   std::string error;
   // JSON parser hides duplicates. We are going to get only one key/value
   // pair at the end.
   scoped_ptr<MessageBundle> message_bundle(
-      extension_l10n_util::LoadMessageCatalogs(src_path, "en", "sr", &error));
+      extension_l10n_util::LoadMessageCatalogs(
+          src_path, "en", "sr", valid_locales, &error));
   EXPECT_TRUE(NULL != message_bundle.get());
   EXPECT_TRUE(error.empty());
 }
diff --git a/extensions/common/file_util.cc b/extensions/common/file_util.cc
index ac335cb..867d864 100644
--- a/extensions/common/file_util.cc
+++ b/extensions/common/file_util.cc
@@ -493,13 +493,10 @@ MessageBundle* LoadMessageBundle(
     return NULL;
 
   std::set<std::string> locales;
-  std::set<std::string> chrome_locales;
-  extension_l10n_util::GetAllLocales(&chrome_locales);
+  if (!extension_l10n_util::GetValidLocales(locale_path, &locales, error))
+    return NULL;
 
-  base::FilePath default_locale_path = locale_path.AppendASCII(default_locale);
-  if (default_locale.empty() ||
-      chrome_locales.find(default_locale) == locales.end() ||
-      !base::PathExists(default_locale_path)) {
+  if (default_locale.empty() || locales.find(default_locale) == locales.end()) {
     *error = l10n_util::GetStringUTF8(
         IDS_EXTENSION_LOCALES_NO_DEFAULT_LOCALE_SPECIFIED);
     return NULL;
@@ -510,6 +507,7 @@ MessageBundle* LoadMessageBundle(
           locale_path,
           default_locale,
           extension_l10n_util::CurrentLocaleOrDefault(),
+          locales,
           error);
 
   return message_bundle;