Add a pref name for share targets, and store their manifest data.

chrome/browser/webshare/share_target_pref_helper_unittest.cc

Index: chrome/browser/webshare/share_target_pref_helper_unittest.cc
diff --git a/chrome/browser/webshare/share_target_pref_helper_unittest.cc b/chrome/browser/webshare/share_target_pref_helper_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..63cee2755f35a25e27831fc3277c2e264fcb9cf6
--- /dev/null
+++ b/chrome/browser/webshare/share_target_pref_helper_unittest.cc
@@ -0,0 +1,145 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.

[Matt Giuca, 2017/01/23 03:00:58]

Copyright 2017.

[constantina, 2017/01/24 02:03:43]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/strings/string16.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/webshare/share_target_pref_helper.cc"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class PrefRegistrySimple;
+
+class ShareTargetPrefHelperUnittest : public testing::Test {
+ protected:
+  ShareTargetPrefHelperUnittest() {}
+  ~ShareTargetPrefHelperUnittest() override {}
+};
+
+TEST_F(ShareTargetPrefHelperUnittest, ShareTargetInfoFromManifestToPrefs) {

[Matt Giuca, 2017/01/23 03:00:58]

I think this could be broken up into multiple individual tests. This would help
to distinguish which of these are actually intended to be run in sequence and
which are independent of one another.

For example, adding sharetarget2.com should be in the same test as
sharetarget.com, since you are testing having multiple share targets. Adding
sharetarget2.com with a null should be in the same test as sharetarget2.com,
since you are testing the behaviour of adding it once, then again. But split
those up into two separate tests (this will involve duplicating the initial
"insert", which you should not feel bad about).

The test of "don't add a site that has a null template" can be a test by itself.
Etc.

This means putting constants into static members of the class (for reuse). And
setting up TestingPrefServiceSimple in a SetUp method, rather than at the top of
this test -- which is good practice anyway.

[constantina, 2017/01/24 02:03:43]

Done.

+  constexpr char kUrlTemplateKey[] = "url_template";
+
+  TestingPrefServiceSimple* pref_service = new TestingPrefServiceSimple();

[Matt Giuca, 2017/01/23 03:00:58]

This will leak memory. Can this be a non-pointer so it cleans itself up at the
end?

(After taking my above advice into account, this will need to be a class member,
so probably makes sense for it to be a unique_ptr which is initialized in the
SetUp method.)

[constantina, 2017/01/24 02:03:43]

Done.

+  pref_service->registry()->RegisterDictionaryPref(
+      prefs::kWebShareVisitedTargets);
+
+  std::string share_target_origin;
+  base::string16 url_string16;
+  const base::DictionaryValue* share_target_dict = nullptr;
+  const base::DictionaryValue* share_target_info_dict = nullptr;
+
+  // Add a share target to prefs that wasn't previously stored.
+  {
+    share_target_origin = "https://www.sharetarget.com/";
+    url_string16 = base::ASCIIToUTF16("share/?title={title}");
+
+    AddShareTargetToPrefs(share_target_origin,
+                          base::NullableString16(url_string16, false),
+                          pref_service);
+
+    share_target_dict =
+        pref_service->GetDictionary(prefs::kWebShareVisitedTargets);
+    EXPECT_EQ(1UL, share_target_dict->size());
+    ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template_in_dict;

[Matt Giuca, 2017/01/23 03:00:58]

I think you can drop "_in_dict" since there is no other variable called
"url_template".

[constantina, 2017/01/24 02:03:43]

Done.

+    ASSERT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey,

[Matt Giuca, 2017/01/23 03:00:58]

Does not have to be an ASSERT, since if this fails it won't result in a bad
state; just an empty string.

[constantina, 2017/01/24 02:03:43]

Done.

+                                                  &url_template_in_dict));
+    EXPECT_TRUE(base::EqualsASCII(url_string16, url_template_in_dict));
+  }
+
+  // Add same share target to prefs that was previously stored; shouldn't
+  // duplicate it.
+  {
+    AddShareTargetToPrefs(share_target_origin,
+                          base::NullableString16(url_string16, false),
+                          pref_service);
+
+    share_target_dict =
+        pref_service->GetDictionary(prefs::kWebShareVisitedTargets);
+    EXPECT_EQ(1UL, share_target_dict->size());
+    ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template_in_dict;
+    ASSERT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey,
+                                                  &url_template_in_dict));
+    EXPECT_TRUE(base::EqualsASCII(url_string16, url_template_in_dict));
+  }
+
+  // Add second share target to prefs that wasn't previously stored.
+  {
+    share_target_origin = "https://www.sharetarget2.com/";
+    url_string16 = base::ASCIIToUTF16("share/?title={title}");
+
+    AddShareTargetToPrefs(share_target_origin,
+                          base::NullableString16(url_string16, false),
+                          pref_service);
+
+    share_target_dict =
+        pref_service->GetDictionary(prefs::kWebShareVisitedTargets);
+    EXPECT_EQ(2UL, share_target_dict->size());
+    ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template_in_dict;
+    ASSERT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey,
+                                                  &url_template_in_dict));
+    EXPECT_TRUE(base::EqualsASCII(url_string16, url_template_in_dict));
+  }
+
+  // Share target already added now has null template. Remove from prefs.
+  {
+    share_target_origin = "https://www.sharetarget2.com/";
+
+    AddShareTargetToPrefs(share_target_origin,
+                          base::NullableString16(url_string16, true),

[Matt Giuca, 2017/01/23 03:00:58]

I don't think it's worth testing a *null* NullableString16 with a *non-empty*
string in it -- that's just a bad input.

So keep this test but just call base::NullableString16() which creates an empty,
null NullableString16.

[constantina, 2017/01/24 02:03:43]

Done.

+                          pref_service);
+
+    share_target_dict =
+        pref_service->GetDictionary(prefs::kWebShareVisitedTargets);
+    EXPECT_EQ(1UL, share_target_dict->size());
+  }
+
+  // Don't add a site that has a null template.
+  {
+    share_target_origin = "https://www.dudsharetarget.com/";
+
+    AddShareTargetToPrefs(share_target_origin,
+                          base::NullableString16(url_string16, true),
+                          pref_service);
+
+    share_target_dict =
+        pref_service->GetDictionary(prefs::kWebShareVisitedTargets);
+    EXPECT_EQ(1UL, share_target_dict->size());
+    ASSERT_FALSE(share_target_dict->GetDictionaryWithoutPathExpansion(
+        share_target_origin, &share_target_info_dict));
+  }
+
+  // Add same share target to prefs that was previously stored, with new
+  // url_template; should update the value.
+  {
+    share_target_origin = "https://www.sharetarget.com/";
+    url_string16 = base::ASCIIToUTF16("share/?title={title}&text={text}");
+
+    AddShareTargetToPrefs(share_target_origin,
+                          base::NullableString16(url_string16, false),
+                          pref_service);
+
+    share_target_dict =
+        pref_service->GetDictionary(prefs::kWebShareVisitedTargets);
+    EXPECT_EQ(1UL, share_target_dict->size());
+    ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template_in_dict;
+    ASSERT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey,
+                                                  &url_template_in_dict));
+    EXPECT_TRUE(base::EqualsASCII(url_string16, url_template_in_dict));
+  }
+}