Read share targets from prefstore, and filter by engagement.

chrome/browser/webshare/share_service_impl_unittest.cc

Index: chrome/browser/webshare/share_service_impl_unittest.cc
diff --git a/chrome/browser/webshare/share_service_impl_unittest.cc b/chrome/browser/webshare/share_service_impl_unittest.cc
index 1bf936b8c40a389d7b0fc6fc6d2c78337cc2d8a8..827c76a5ba3c715d32a912fa958592ffeee63b40 100644
--- a/chrome/browser/webshare/share_service_impl_unittest.cc
+++ b/chrome/browser/webshare/share_service_impl_unittest.cc
@@ -9,7 +9,11 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/webshare/share_service_impl.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "components/prefs/testing_pref_service.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -37,22 +41,68 @@ class ShareServiceTestImpl : public ShareServiceImpl {
     picker_result_ = result;
   }
 
+  void SetUpPrefs() {

[Matt Giuca, 2017/02/03 02:40:24]

This can just go in the constructor (since you call it immediately after calling
the ctor anyway).

[constantina, 2017/02/08 23:25:17]

Done.

+    pref_service_.reset(new TestingPrefServiceSimple());
+    pref_service_->registry()->RegisterDictionaryPref(
+        prefs::kWebShareVisitedTargets);
+  }
+
+  void AddShareTarget(std::string manifest_url, std::string url_template) {

[Matt Giuca, 2017/02/03 02:40:24]

const std::string&

[constantina, 2017/02/08 23:25:16]

Done.

+    DictionaryPrefUpdate update(GetPrefService(),
+                                prefs::kWebShareVisitedTargets);
+    base::DictionaryValue* share_target_dict = update.Get();
+
+    std::unique_ptr<base::DictionaryValue> origin_dict(
+        new base::DictionaryValue);
+
+    constexpr char kUrlTemplateKey[] = "url_template";
+    origin_dict->SetStringWithoutPathExpansion(kUrlTemplateKey, url_template);
+
+    share_target_dict->SetWithoutPathExpansion(manifest_url,
+                                               std::move(origin_dict));
+  }
+
+  void AddEngagementForTarget(std::string manifest_url,

[Matt Giuca, 2017/02/03 02:40:24]

How about SetEngagementForTarget (rather than Add).

Also const std::string&

[constantina, 2017/02/08 23:25:16]

Done.

+                              blink::mojom::EngagementLevel level) {
+    engagement_map[manifest_url] = level;
+  }
+
+  void ClearEngagements() { engagement_map.clear(); }

[Matt Giuca, 2017/02/03 02:40:24]

This isn't needed (since the whole ShareServiceTestImpl is created on each
SetUp, you don't need to clear this on TearDown).

[constantina, 2017/02/08 23:25:16]

Done.

+
   const std::string& GetLastUsedTargetURL() { return last_used_target_url_; }
 
+  const std::vector<base::string16>& GetTargetsInPicker() {
+    return targets_in_picker_;
+  }
+
  private:
   base::Optional<base::string16> picker_result_ = base::nullopt;

[Matt Giuca, 2017/02/03 02:40:24]

base::nullopt not needed (it's the default) ... sorry this is from another CL
but a quick fix.

[constantina, 2017/02/08 23:25:16]

Done.

   std::string last_used_target_url_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
+  std::map<std::string, blink::mojom::EngagementLevel> engagement_map;

[Matt Giuca, 2017/02/03 02:40:24]

engagement_map_

[constantina, 2017/02/08 23:25:17]

Done.

+  std::vector<base::string16> targets_in_picker_;
 
   void ShowPickerDialog(
       const std::vector<base::string16>& targets,
       const base::Callback<void(base::Optional<base::string16>)>& callback)
       override {
+    targets_in_picker_ = targets;
+    if (picker_result_ != base::nullopt) {

[constantina, 2017/02/08 23:25:16]

Don't need this anymore; not testing ShareServiceImpl implementation.

+      EXPECT_TRUE(find(targets.begin(), targets.end(), picker_result_) !=
+                  targets.end());
+    }
     callback.Run(picker_result_);
   }
 
   void OpenTargetURL(const GURL& target_url) override {
     last_used_target_url_ = target_url.spec();
   }
+
+  PrefService* GetPrefService() override { return pref_service_.get(); }
+
+  blink::mojom::EngagementLevel GetEngagementLevel(const GURL& url) override {
+    return engagement_map[url.spec()];
+  }
 };
 
 class ShareServiceImplUnittest : public ChromeRenderViewHostTestHarness {
@@ -65,16 +115,24 @@ class ShareServiceImplUnittest : public ChromeRenderViewHostTestHarness {
 
     share_service_helper_ =
         ShareServiceTestImpl::Create(mojo::MakeRequest(&share_service_));
+    share_service_helper_->SetUpPrefs();
   }
 
-  void TearDown() override { ChromeRenderViewHostTestHarness::TearDown(); }
+  void TearDown() override {
+    ChromeRenderViewHostTestHarness::TearDown();

[Matt Giuca, 2017/02/03 02:40:24]

Is this override needed at all? (Before this CL) it was just calling its base
class TearDown().

[constantina, 2017/02/08 23:25:17]

I think we worked out when we first wrote the tests that we had to call the base
class constructor ourselves, and it doesn't work otherwise. Can confirm now it
doesn't work if I remove.

+    share_service_helper_->ClearEngagements();

[Matt Giuca, 2017/02/03 02:40:24]

As stated above, this isn't needed because the entire share_service_helper_ is
destroyed and recreated on each SetUp.

[constantina, 2017/02/08 23:25:16]

Done.

+  }
 
   void DidShare(const std::string& expected_target_url,
+                const std::vector<base::string16>& expected_targets_in_picker,

[Matt Giuca, 2017/02/03 02:40:24]

Little nit pick: could this argument go first? Just because the logical order in
my mind is:
1. The targets go into the picker (check that),
2. The user picks a target,
3. We get back a picked URL (check that),
4. We return a mojo result (param) (check that).

Also do the checks in the body of this function in that same order (first check
targets, then URL, then param).

Also (carry-over from a previous CL)... why is this called param? Isn't it the
response from the Mojo service? In the mojom, it's called "error" not "param".
Could you rename this to expected_error and error, for clarity?

[constantina, 2017/02/08 23:25:16]

Done.

                 const base::Optional<std::string>& expected_param,
                 const base::Optional<std::string>& param) {
     EXPECT_EQ(expected_param, param);
     std::string target_url = share_service_helper_->GetLastUsedTargetURL();
     EXPECT_EQ(expected_target_url, target_url);
+    std::vector<base::string16> targets_in_picker =
+        share_service_helper_->GetTargetsInPicker();
+    EXPECT_EQ(expected_targets_in_picker, targets_in_picker);

[Matt Giuca, 2017/02/03 02:40:24]

Good test that the targets actually show up in the dialog!

[constantina, 2017/02/08 23:25:17]

:D

 
     if (!on_callback_.is_null())
       on_callback_.Run();
@@ -90,21 +148,29 @@ class ShareServiceImplUnittest : public ChromeRenderViewHostTestHarness {
 // Basic test to check the Share method calls the callback with the expected
 // parameters.
 TEST_F(ShareServiceImplUnittest, ShareCallbackParams) {
+  std::string manifest_url =
+      "https://wicg.github.io/web-share-target/demos/manifest.json";

[Matt Giuca, 2017/02/03 02:40:24]

Now that we are (as of this CL) no longer hard-coding the github sites at all, I
think it makes sense to change these tests to fictional URLs (since usually
tests won't use real-world data).

You can make these templates and strings a bit simpler then.

[constantina, 2017/02/08 23:25:16]

Done.

+  std::string url_template =
+      "sharetarget.html?title={title}&text={text}&url={url}";
   std::string expected_url =
       "https://wicg.github.io/web-share-target/demos/"
       "sharetarget.html?title=My%20title&text=My%20text&url=https%3A%2F%2Fwww."
       "google.com%2F";
-  share_service_helper_->set_picker_result(
-      base::ASCIIToUTF16("https://wicg.github.io/web-share-target/demos/"));
 
-  const GURL url(kUrlSpec);
+  share_service_helper_->set_picker_result(base::ASCIIToUTF16(manifest_url));
+  share_service_helper_->AddShareTarget(manifest_url, url_template);

[Matt Giuca, 2017/02/03 02:40:24]

Perhaps add a second target here (since none of the tests currently test >1
target actually showing up in the dialog).

Just copy the cpyro target from ShareWithSomeInsuffientlyEngagedTargets, but
change the target engagements: one should be LOW but the other should be HIGH as
a quick test that highly-engaged targets also show up.

(Hmm... if you consider all my advice in this file, I'm basically asking you to
have the exact same two targets in 3 tests, which suggests maybe this should be
done in the SetUp, except for that pesky ShareCancel test that doesn't want any
targets. At least move some of the constants like manifest URLs into top-level
constants.)

[constantina, 2017/02/08 23:25:17]

Done. Did a bit of a refactor.

+  share_service_helper_->AddEngagementForTarget(
+      manifest_url, blink::mojom::EngagementLevel::LOW);
+
+  std::vector<base::string16> expected_targets{base::UTF8ToUTF16(manifest_url)};
   base::Callback<void(const base::Optional<std::string>&)> callback =
       base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
-                 expected_url, base::Optional<std::string>());
+                 expected_url, expected_targets, base::Optional<std::string>());
 
   base::RunLoop run_loop;
   on_callback_ = run_loop.QuitClosure();
 
+  const GURL url(kUrlSpec);
   share_service_->Share(kTitle, kText, url, callback);
 
   run_loop.Run();
@@ -118,7 +184,7 @@ TEST_F(ShareServiceImplUnittest, ShareCancel) {
   // Expect an error message in response.
   base::Callback<void(const base::Optional<std::string>&)> callback =
       base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
-                 std::string(),
+                 std::string(), std::vector<base::string16>(),
                  base::Optional<std::string>("Share was cancelled"));
 
   base::RunLoop run_loop;
@@ -130,6 +196,70 @@ TEST_F(ShareServiceImplUnittest, ShareCancel) {
   run_loop.Run();
 }
 
+// Test to check that no targets were in picker if they didn't have enough
+// engagement. User forced to cancel.
+TEST_F(ShareServiceImplUnittest, ShareWithInsuffientlyEngagedTargets) {

[Matt Giuca, 2017/02/03 02:40:24]

I don't think it's worth having this test. It essentially covers the
intersection of ShareCancel and ShareWithSomeInsuffientlyEngagedTargets; that
logic is already covered by those two tests.

(Especially since I asked you above to add yet another test of cancellation when
there are actual targets to choose from.)

[constantina, 2017/02/08 23:25:17]

Removed.

+  std::string manifest_url =
+      "https://wicg.github.io/web-share-target/demos/manifest.json";
+  std::string url_template =
+      "sharetarget.html?title={title}&text={text}&url={url}";
+
+  share_service_helper_->set_picker_result(base::nullopt);
+  share_service_helper_->AddShareTarget(manifest_url, url_template);
+  share_service_helper_->AddEngagementForTarget(
+      manifest_url, blink::mojom::EngagementLevel::MINIMAL);
+
+  base::Callback<void(const base::Optional<std::string>&)> callback =
+      base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
+                 std::string(), std::vector<base::string16>(),
+                 base::Optional<std::string>("Share was cancelled"));
+
+  base::RunLoop run_loop;
+  on_callback_ = run_loop.QuitClosure();
+
+  const GURL url(kUrlSpec);
+  share_service_->Share(kTitle, kText, url, callback);
+
+  run_loop.Run();
+}
+
+// Test to check that only targets with enough engagement were in picker.
+TEST_F(ShareServiceImplUnittest, ShareWithSomeInsuffientlyEngagedTargets) {

[Matt Giuca, 2017/02/03 02:40:24]

Insufficiently

[constantina, 2017/02/08 23:25:17]

Done.

+  std::string manifest_url_wicg =
+      "https://wicg.github.io/web-share-target/demos/manifest.json";
+  std::string manifest_url_cpyro =
+      "https://cpyro.github.io/web-share-target/demos/manifest.json";
+  std::string url_template =
+      "sharetarget.html?title={title}&text={text}&url={url}";
+  std::string expected_url =
+      "https://cpyro.github.io/web-share-target/demos/"
+      "sharetarget.html?title=My%20title&text=My%20text&url=https%3A%2F%2Fwww."
+      "google.com%2F";
+
+  share_service_helper_->set_picker_result(
+      base::UTF8ToUTF16(manifest_url_cpyro));
+  share_service_helper_->AddShareTarget(manifest_url_wicg, url_template);
+  share_service_helper_->AddShareTarget(manifest_url_cpyro, url_template);
+  share_service_helper_->AddEngagementForTarget(
+      manifest_url_wicg, blink::mojom::EngagementLevel::MINIMAL);
+  share_service_helper_->AddEngagementForTarget(
+      manifest_url_cpyro, blink::mojom::EngagementLevel::LOW);
+
+  std::vector<base::string16> expected_targets{
+      base::UTF8ToUTF16(manifest_url_cpyro)};
+  base::Callback<void(const base::Optional<std::string>&)> callback =
+      base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
+                 expected_url, expected_targets, base::Optional<std::string>());
+
+  base::RunLoop run_loop;
+  on_callback_ = run_loop.QuitClosure();
+
+  const GURL url(kUrlSpec);
+  share_service_->Share(kTitle, kText, url, callback);
+
+  run_loop.Run();
+}
+
 // Replace various numbers of placeholders in various orders. Placeholders are
 // adjacent to eachother; there are no padding characters.
 TEST_F(ShareServiceImplUnittest, ReplacePlaceholders) {