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;
+

[Matt Giuca, 2017/01/24 03:26:09]

I think this entire file (except for the forward declares) should be in an
anonymous namespace.

[constantina, 2017/01/24 23:39:05]

Done.

+class ShareTargetPrefHelperUnittest : public testing::Test {
+ protected:
+  ShareTargetPrefHelperUnittest() {}
+  ~ShareTargetPrefHelperUnittest() override {}
+
+  void SetUp() override {
+    pref_service.registry()->RegisterDictionaryPref(

[Matt Giuca, 2017/01/24 03:26:10]

This will need a corresponding TearDown which resets it back to a default state.

If that's not possible, make it a unique_ptr and construct a new
TestingPrefServiceSimple on SetUp.

[constantina, 2017/01/24 23:39:05]

As in, an UnregisterPref? I can't find anything that exists like that, so made
it a unique_ptr.

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

Acknowledged.

+        prefs::kWebShareVisitedTargets);
+
+    share_target_dict = nullptr;
+    share_target_info_dict = nullptr;
+  }
+
+  TestingPrefServiceSimple pref_service;

[Matt Giuca, 2017/01/24 03:26:10]

These should follow standard style rules: fields should be private and names
should end with underscores. Make a pref_service() getter which is protected.
This has the advantage that pref_service() can return a PrefService*,
abstracting away the fact that this is a TestingPrefServiceSimple from the rest
of the test.

[constantina, 2017/01/24 23:39:05]

Done.

+  std::string share_target_origin;

[Matt Giuca, 2017/01/24 03:26:10]

These four (share_target_origin .. share_target_info_dict) don't really belong
here. They don't represent pre-setup state or constant data used by all tests;
rather they are local variables that happen to be used in all tests.

I think move them back down into the individual tests.

[constantina, 2017/01/24 23:39:05]

Done.

+  base::Optional<std::string> expected_url_template;
+  const base::DictionaryValue* share_target_dict;
+  const base::DictionaryValue* share_target_info_dict;
+};
+
+constexpr char kUrlTemplateKey[] = "url_template";
+
+TEST_F(ShareTargetPrefHelperUnittest, AddMultipleShareTargets) {

[Matt Giuca, 2017/01/24 03:26:10]

Good, the tests feel much better split up.

[constantina, 2017/01/24 23:39:05]

:D

+  // Add a share target to prefs that wasn't previously stored.
+  {

[Matt Giuca, 2017/01/24 03:26:10]

I don't think these braces are really warranted. They aren't being used for
scoping (all your variables are being declared outside these blocks anyway),
just for logical separation. You already have the visual separation from the
blank lines and comments.

[constantina, 2017/01/24 23:39:05]

Done.

+    share_target_origin = "https://www.sharetarget.com/";
+    expected_url_template = base::Optional<std::string>("share/?title={title}");

[Matt Giuca, 2017/01/24 03:26:10]

This shouldn't be called "expected_url_template" because it's also used as the
*input* to the function-under-test. Just "url_template".

(The fact that it is also used as the expected value is fine; it just doesn't
justify it being called "expected".)

[constantina, 2017/01/24 23:39:05]

There's already a url_template, which you said to change from
url_template_in_dict, because there was no other url_template. I could change
that back and change the expected one to just url_template?

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

Oh I see what I did there.

Yeah I think it makes more sense to change it back: url_template_from_dict (or
in_dict) and url_template.

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

Done.

+
+    webshare::AddShareTargetToPrefs(
+        share_target_origin, 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(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template;
+    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.
+  {
+    share_target_origin = "https://www.sharetarget2.com/";
+    expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+    webshare::AddShareTargetToPrefs(share_target_origin, 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(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template;
+    EXPECT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey,
+                                                  &url_template));
+    EXPECT_EQ(url_template, expected_url_template);
+  }
+}
+
+TEST_F(ShareTargetPrefHelperUnittest, AddShareTargetTwice) {

[Matt Giuca, 2017/01/24 03:26:10]

I think this test can be removed ... it is essentially testing the same thing as
UpdateShareTarget. Isn't it?

[constantina, 2017/01/24 23:39:05]

One adds the same share target with the same url_template, and the other adds
the same share target with a different url_template. I would count those as
different cases, but can delete if you disagree.

+  share_target_origin = "https://www.sharetarget.com/";
+  expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+  // Add a share target to prefs that wasn't previously stored.
+  {
+    webshare::AddShareTargetToPrefs(
+        share_target_origin, 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(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template;
+    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.
+  {
+    webshare::AddShareTargetToPrefs(
+        share_target_origin, 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(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template;
+    EXPECT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey,
+                                                  &url_template));
+    EXPECT_EQ(url_template, expected_url_template);
+  }
+}
+
+TEST_F(ShareTargetPrefHelperUnittest, DontAddNonShareTarget) {

[Matt Giuca, 2017/01/24 03:26:10]

Move this down right before RemoveShareTarget....

[constantina, 2017/01/24 23:39:05]

Done.

+  // Don't add a site that has a null template.
+  {

[Matt Giuca, 2017/01/24 03:26:10]

Don't need the extra braces for a test that only has one block.

[constantina, 2017/01/24 23:39:05]

Done.

+    share_target_origin = "https://www.dudsharetarget.com/";
+    expected_url_template = base::nullopt;
+
+    webshare::AddShareTargetToPrefs(
+        share_target_origin, 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(
+        share_target_origin, &share_target_info_dict));
+  }
+}
+
+TEST_F(ShareTargetPrefHelperUnittest, UpdateShareTarget) {
+  // Add a share target to prefs that wasn't previously stored.
+  {
+    share_target_origin = "https://www.sharetarget.com/";
+    expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+    webshare::AddShareTargetToPrefs(
+        share_target_origin, 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(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template;
+    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.
+  {
+    share_target_origin = "https://www.sharetarget.com/";
+    expected_url_template =
+        base::Optional<std::string>("share/?title={title}&text={text}");
+
+    webshare::AddShareTargetToPrefs(
+        share_target_origin, 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(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template;
+    EXPECT_TRUE(share_target_info_dict->GetString(kUrlTemplateKey,
+                                                  &url_template));
+    EXPECT_EQ(url_template, expected_url_template);
+  }
+}
+
+TEST_F(ShareTargetPrefHelperUnittest, RemoveShareTargetThatIsNoLongerTarget) {

[Matt Giuca, 2017/01/24 03:26:10]

Maybe just rename this to "RemoveShareTarget".

[constantina, 2017/01/24 23:39:05]

Done.

+  // Add a share target to prefs that wasn't previously stored.
+  {
+    share_target_origin = "https://www.sharetarget.com/";
+    expected_url_template = base::Optional<std::string>("share/?title={title}");
+
+    webshare::AddShareTargetToPrefs(
+        share_target_origin, 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(
+        share_target_origin, &share_target_info_dict));
+    EXPECT_EQ(1UL, share_target_info_dict->size());
+    std::string url_template;
+    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.
+  {
+    share_target_origin = "https://www.sharetarget.com/";
+    expected_url_template = base::nullopt;
+
+    webshare::AddShareTargetToPrefs(
+        share_target_origin, expected_url_template, &pref_service);
+
+    share_target_dict =
+        pref_service.GetDictionary(prefs::kWebShareVisitedTargets);
+    EXPECT_EQ(0UL, share_target_dict->size());
+  }
+}