Handle empty string in GetSavePasswordDialogTitleTextAndLinkRange gracefully

chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc

Index: chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
diff --git a/chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc b/chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
index bbc2a7dc44caff80e7eb74957a6d0d955942414b..2377a608aec5598f99d1c7700ba7a4afa73c80a0 100644
--- a/chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
+++ b/chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
@@ -7,17 +7,62 @@
 #include <stddef.h>
 
 #include "base/macros.h"
+#include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/range/range.h"
 #include "url/gurl.h"
 
 namespace {
 
+class EmptyDelegate : public ui::ResourceBundle::Delegate {
+ public:
+  EmptyDelegate() {}
+  ~EmptyDelegate() override {}
+
+  // ui::ResourceBundle::Delegate
+  base::FilePath GetPathForResourcePack(const base::FilePath& pack_path,
+                                        ui::ScaleFactor scale_factor) override {
+    return base::FilePath();
+  }
+
+  base::FilePath GetPathForLocalePack(const base::FilePath& pack_path,
+                                      const std::string& locale) override {
+    return base::FilePath();
+  }
+
+  gfx::Image GetImageNamed(int resource_id) override { return gfx::Image(); }
+
+  gfx::Image GetNativeImageNamed(int resource_id) override {
+    return gfx::Image();
+  }
+
+  base::RefCountedMemory* LoadDataResourceBytes(
+      int resource_id,
+      ui::ScaleFactor scale_factor) override {
+    return nullptr;
+  }
+
+  bool GetRawDataResource(int resource_id,
+                          ui::ScaleFactor scale_factor,
+                          base::StringPiece* value) override {
+    return false;
+  }
+
+  bool GetLocalizedString(int message_id, base::string16* value) override {
+    value->clear();
+    return true;

[vasilii, 2016/11/24 17:26:28]

It's strange to return true in production if the read failed. I think for the
sake of testing it's OK.

[vabr (Chromium), 2016/11/25 09:57:28]

This is now removed.
(I agree this was strange. In the real bug the empty string did not come from a
delegate, I suspect, but at that version of my test, the delegate seemed the
best way to emulate the situation. Not any longer. :))

+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EmptyDelegate);
+};
+
 const struct {
   const char* const user_visible_url;
   const char* const form_origin_url;
@@ -124,6 +169,24 @@ TEST(ManagePasswordsViewUtilTest, GetSavePasswordDialogTitleTextAndLinkRange) {
   }
 }
 
+// Check that empty localised strings do not cause a crash.
+TEST(ManagePasswordsViewUtilTest,
+     GetSavePasswordDialogTitleTextAndLinkRange_EmptyStrings) {
+  EmptyDelegate delegate;
+
+  ui::ResourceBundle::CleanupSharedInstance();
+  ui::ResourceBundle::InitSharedInstanceWithLocale(

[vasilii, 2016/11/24 17:26:28]

I'm afraid that your test doesn't leave the global object in the same state when
it started. It will lead to incomprehensible flakiness of random tests.

[vabr (Chromium), 2016/11/25 09:57:28]

That's a good point. Also, I realised that the delegate is unnecessarily heavy
for just overriding string definitions. So I created a new scoper for that,
which should address the clean-up concern and use the simpler way of overriding.

+      std::string(), &delegate,
+      ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
+
+  base::string16 title;
+  gfx::Range title_link_range;
+  const GURL kExample = GURL("http://example.org");

[vasilii, 2016/11/24 17:26:28]

Why not const GURL kExample("http://example.org");

[vabr (Chromium), 2016/11/25 09:57:28]

My oversight during copy-pasting.
Fixed now.

+  GetSavePasswordDialogTitleTextAndLinkRange(kExample, kExample, true,
+                                             PasswordTitleType::SAVE_PASSWORD,
+                                             &title, &title_link_range);
+}
+
 TEST(ManagePasswordsViewUtilTest, GetManagePasswordsDialogTitleText) {
   for (size_t i = 0; i < arraysize(kDomainsTestCases); ++i) {
     SCOPED_TRACE(testing::Message() << "user_visible_url = "