Read share targets from prefstore, and filter by engagement.

chrome/browser/webshare/share_service_impl_unittest.cc

 // Copyright 2016 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 <memory>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #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"
 #include "url/gurl.h"
 
 namespace {
 
 constexpr char kTitle[] = "My title";
 constexpr char kText[] = "My text";
 constexpr char kUrlSpec[] = "https://www.google.com/";
 
 class ShareServiceTestImpl : public ShareServiceImpl {
  public:
   static ShareServiceTestImpl* Create(

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

Just noticed this (not directly applicable to this CL but already landed
code)... what is the lifetime of the ShareServiceTestImpl?

It creates a strong binding but does it ever get cleaned up anywhere? I'm not
really familiar with the lifetime of a strong binding (is it actually needed in
the test?). Do you have to call Close() manually in the TearDown?

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

I am not sure. I will have to ask Sam or someone else.

[Sam McNally, 2017/02/08 23:34:46]

A StrongBinding ties the lifetime of the object to the mojo connection. In a
same-thread case like this, it'll be deleted when the message loop runs after
the InterfacePtr owning this closes.

Using a mojo::Binding and owning the implementation directly from the test would
probably be easier to understand.

[constantina, 2017/02/09 05:40:10]

Done.

       blink::mojom::ShareServiceRequest request) {
     std::unique_ptr<ShareServiceTestImpl> share_service_helper =
         base::MakeUnique<ShareServiceTestImpl>();
     ShareServiceTestImpl* share_service_helper_raw = share_service_helper.get();
     mojo::MakeStrongBinding(std::move(share_service_helper),
                             std::move(request));
     return share_service_helper_raw;
   }
 
   void set_picker_result(base::Optional<base::string16> result) {
     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 {
  public:
   ShareServiceImplUnittest() = default;
   ~ShareServiceImplUnittest() override = default;
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
 
     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();
   }
 
   blink::mojom::ShareServicePtr share_service_;
   ShareServiceTestImpl* share_service_helper_;
   base::Closure on_callback_;
 };
 
 }  // namespace
 
 // 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();
 }
 
 // Tests the result of cancelling the share in the picker UI.
 TEST_F(ShareServiceImplUnittest, ShareCancel) {

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

Add a test where there are 2 sufficiently engaged targets but the user chooses
to cancel. (Right now, the only cancellation tests are those where there are no
targets in the dialog.)

(So there should be two cancellation tests: one with 0 targets and one with 2
targets.)

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

Done.

   // Ask that the dialog be cancelled.
   share_service_helper_->set_picker_result(base::nullopt);
 
   // 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;
   on_callback_ = run_loop.QuitClosure();
 
   const GURL url(kUrlSpec);
   share_service_->Share(kTitle, kText, url, callback);
 
   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) {
   const GURL url(kUrlSpec);
   std::string url_template_filled;
   bool succeeded;
 
   // No placeholders
   std::string url_template = "blank";
   succeeded = ShareServiceImpl::ReplacePlaceholders(url_template, kTitle, kText,
@@ skipping 183 matching lines @@
   EXPECT_TRUE(succeeded);
   EXPECT_EQ("%C3%A9", url_template_filled);
 
   // U+1F4A9
   url_template = "{title}";
   succeeded = ShareServiceImpl::ReplacePlaceholders(
       url_template, "\xf0\x9f\x92\xa9", kText, url, &url_template_filled);
   EXPECT_TRUE(succeeded);
   EXPECT_EQ("%F0%9F%92%A9", url_template_filled);
 }