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

chrome/browser/webshare/share_target_pref_helper_unittest.cc

+// Copyright 2017 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 "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.h"
+#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;
+
+namespace {
+
+class ShareTargetPrefHelperUnittest : public testing::Test {
+ protected:
+  ShareTargetPrefHelperUnittest() {}
+  ~ShareTargetPrefHelperUnittest() override {}
+
+  void SetUp() override {
+    pref_service_ = std::unique_ptr<TestingPrefServiceSimple>(

[Matt Giuca, 2017/01/25 00:25:54]

pref_service_.reset(new TPSS());

Prefer reset over construction of a new object and assignment. Just less
verbose. (Also *potentially* more efficient but likely to be the same either
way; that's not the main reason.)

[constantina, 2017/01/25 03:18:27]

Done.

+        new TestingPrefServiceSimple());
+    pref_service_->registry()->RegisterDictionaryPref(
+        prefs::kWebShareVisitedTargets);
+  }
+
+  PrefService* pref_service() { return pref_service_.get(); }
+
+ private:
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
+};
+
+constexpr char kUrlTemplateKey[] = "url_template";
+
+TEST_F(ShareTargetPrefHelperUnittest, AddMultipleShareTargets) {
+  std::string manifest_url;

[Matt Giuca, 2017/01/25 00:25:53]

nit: I *think* these should be declared along side their first assignment (no
strong feeling though).

[constantina, 2017/01/25 03:18:27]

Acknowledged. I think it makes the subtests look inconsistent and messy. But, I
could add scoping braces again, and declare the variable in both subtest scopes?

Also, url_template and the DictionaryValues need to be declared anyway, to pass
them as output parameters.

[Matt Giuca, 2017/01/30 03:32:39]

I think the thing is you shouldn't be thinking of them as "subtests" but all
part of one test. (If you are testing multiple independent things then they
should be split up, and you did that.) I get what you mean by "inconsistent" (if
the top one declares and the bottom one re-uses) but it doesn't have to be
consistent since they aren't independent anyway.

This recommendation comes from the style guide:
https://google.github.io/styleguide/cppguide.html#Local_Variables
"We encourage you to declare [local variables] ... as close to the first use as
possible."

The things that get assigned first (manifest_url, expected_url_template,
share_target_dict) should be declared at their first assignment. The things that
are first used as output parameters (url_template, share_target_info_dict) need
to be declared on their own line, but should be done immediately before use.

[constantina, 2017/01/31 23:37:06]

That makes sense. Done.

+  base::Optional<std::string> expected_url_template;
+  std::string url_template;
+  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.
+  manifest_url = "https://www.sharetarget.com/manifest.json";
+  expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(1UL, share_target_dict->size());
+  ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+      manifest_url, &share_target_info_dict));
+  EXPECT_EQ(1UL, share_target_info_dict->size());
+  EXPECT_TRUE(
+      share_target_info_dict->GetString(kUrlTemplateKey, &url_template));
+  EXPECT_EQ(url_template, expected_url_template);
+
+  // Add second share target to prefs that wasn't previously stored.
+  manifest_url = "https://www.sharetarget2.com/manifest.json";
+  expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(2UL, share_target_dict->size());
+  ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+      manifest_url, &share_target_info_dict));
+  EXPECT_EQ(1UL, share_target_info_dict->size());
+  EXPECT_TRUE(
+      share_target_info_dict->GetString(kUrlTemplateKey, &url_template));
+  EXPECT_EQ(url_template, expected_url_template);
+}
+
+TEST_F(ShareTargetPrefHelperUnittest, AddShareTargetTwice) {
+  std::string manifest_url;

[Matt Giuca, 2017/01/25 00:25:54]

Again, just declare these as you need them. (Avoids needing to assign nullptr
just to assign it again later.)

In this one, you aren't changing manifest_url and expected_url_template between
the two adds, so they can be constants. manifest_url should be const char [].
And rename them to kXXX.

[constantina, 2017/01/25 03:18:27]

As I said in last comment, I have to declare them anyway. I initially had to
assign nullptr to suppress a warning. But it doesn't appear now when I remove
nullptr.

Constants are done though.

+  base::Optional<std::string> expected_url_template;
+  std::string url_template;
+  const base::DictionaryValue* share_target_dict = nullptr;
+  const base::DictionaryValue* share_target_info_dict = nullptr;
+
+  manifest_url = "https://www.sharetarget.com/manifest.json";
+  expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+  // Add a share target to prefs that wasn't previously stored.
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(1UL, share_target_dict->size());
+  ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+      manifest_url, &share_target_info_dict));
+  EXPECT_EQ(1UL, share_target_info_dict->size());
+  EXPECT_TRUE(
+      share_target_info_dict->GetString(kUrlTemplateKey, &url_template));
+  EXPECT_EQ(url_template, expected_url_template);
+
+  // Add same share target to prefs that was previously stored; shouldn't
+  // duplicate it.
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(1UL, share_target_dict->size());
+  ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+      manifest_url, &share_target_info_dict));
+  EXPECT_EQ(1UL, share_target_info_dict->size());
+  EXPECT_TRUE(
+      share_target_info_dict->GetString(kUrlTemplateKey, &url_template));
+  EXPECT_EQ(url_template, expected_url_template);
+}
+
+TEST_F(ShareTargetPrefHelperUnittest, UpdateShareTarget) {
+  std::string manifest_url;
+  base::Optional<std::string> expected_url_template;
+  std::string url_template;
+  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.
+  manifest_url = "https://www.sharetarget.com/manifest.json";
+  expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(1UL, share_target_dict->size());
+  ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+      manifest_url, &share_target_info_dict));
+  EXPECT_EQ(1UL, share_target_info_dict->size());
+  EXPECT_TRUE(
+      share_target_info_dict->GetString(kUrlTemplateKey, &url_template));
+  EXPECT_EQ(url_template, expected_url_template);
+
+  // Add same share target to prefs that was previously stored, with new
+  // url_template; should update the value.
+  manifest_url = "https://www.sharetarget.com/manifest.json";
+  expected_url_template =
+      base::Optional<std::string>("share/?title={title}&text={text}");
+
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(1UL, share_target_dict->size());
+  ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+      manifest_url, &share_target_info_dict));
+  EXPECT_EQ(1UL, share_target_info_dict->size());
+  EXPECT_TRUE(
+      share_target_info_dict->GetString(kUrlTemplateKey, &url_template));
+  EXPECT_EQ(url_template, expected_url_template);
+}
+
+TEST_F(ShareTargetPrefHelperUnittest, DontAddNonShareTarget) {
+  std::string manifest_url;

[Matt Giuca, 2017/01/25 00:25:54]

As with AddShareTarget, these can be constants because they are never changed.

[constantina, 2017/01/25 03:18:27]

Done.

+  base::Optional<std::string> expected_url_template;
+  std::string url_template;
+  const base::DictionaryValue* share_target_dict = nullptr;
+  const base::DictionaryValue* share_target_info_dict = nullptr;
+
+  // Don't add a site that has a null template.
+  manifest_url = "https://www.dudsharetarget.com/manifest.json";
+  expected_url_template = base::nullopt;
+
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(0UL, share_target_dict->size());
+  ASSERT_FALSE(share_target_dict->GetDictionaryWithoutPathExpansion(
+      manifest_url, &share_target_info_dict));
+}
+
+TEST_F(ShareTargetPrefHelperUnittest, RemoveShareTarget) {
+  std::string manifest_url;
+  base::Optional<std::string> expected_url_template;
+  std::string url_template;
+  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.
+  manifest_url = "https://www.sharetarget.com/manifest.json";
+  expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(1UL, share_target_dict->size());
+  ASSERT_TRUE(share_target_dict->GetDictionaryWithoutPathExpansion(
+      manifest_url, &share_target_info_dict));
+  EXPECT_EQ(1UL, share_target_info_dict->size());
+  EXPECT_TRUE(
+      share_target_info_dict->GetString(kUrlTemplateKey, &url_template));
+  EXPECT_EQ(url_template, expected_url_template);
+
+  // Share target already added now has null template. Remove from prefs.
+  manifest_url = "https://www.sharetarget.com/manifest.json";
+  expected_url_template = base::nullopt;
+
+  AddShareTargetToPrefs(manifest_url, expected_url_template,
+                        pref_service());
+
+  share_target_dict =
+      pref_service()->GetDictionary(prefs::kWebShareVisitedTargets);
+  EXPECT_EQ(0UL, share_target_dict->size());
+}
+
+}  // namespace