Default share to Share Target: Partial impl. of Web Share for Desktop.

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 fe0e1c75e9ac1b5cb1708cdcc19d736677a47c0e..ac6455134598834324784a34bd05916bc6e66dec 100644
--- a/chrome/browser/webshare/share_service_impl_unittest.cc
+++ b/chrome/browser/webshare/share_service_impl_unittest.cc
@@ -2,55 +2,187 @@
 // 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 "chrome/browser/webshare/share_service_impl.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.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"
 
-class ShareServiceTest : public ChromeRenderViewHostTestHarness {
+class ShareServiceImplTestHelper : public ShareServiceImpl {

[Matt Giuca, 2016/12/21 07:13:49]

"TestHelper" implies that it is a collection of helper methods. Rather, this
*is* the Share Service (but subclassed with extra test magic).

How about: ShareServiceTestImpl or ShareServiceImplForTest?

[constantina, 2017/01/04 07:01:45]

Done.

+ public:
+  static ShareServiceImplTestHelper* Create(
+      blink::mojom::ShareServiceRequest request) {
+    std::unique_ptr<ShareServiceImplTestHelper> share_service_helper =
+        base::MakeUnique<ShareServiceImplTestHelper>();
+    ShareServiceImplTestHelper* share_service_helper_raw =
+        share_service_helper.get();
+    mojo::MakeStrongBinding(std::move(share_service_helper),
+                            std::move(request));
+    return share_service_helper_raw;
+  }
+
+  void openTargetURL(GURL target_url) override {

[Matt Giuca, 2016/12/21 07:13:49]

const GURL&

Get into the habit of always using "const X&" in place of X whenever X is a
non-primitive value parameter. It means one less copy of the data.

[Matt Giuca, 2016/12/21 07:13:50]

Nit: Names should start with a capital letter.

[constantina, 2017/01/04 07:01:45]

Done.

[constantina, 2017/01/04 07:01:45]

Done.

+    last_used_target_url_ = target_url.spec();
+  }
+
+  std::string last_used_target_url_;

[Matt Giuca, 2016/12/21 07:13:49]

Even in a test, you shouldn't have public fields. Make this private and provide
a getter.

[constantina, 2017/01/04 07:01:45]

Done.

+};
+
+class ShareServiceImplUnittest : public ChromeRenderViewHostTestHarness {
  public:
-  ShareServiceTest() = default;
-  ~ShareServiceTest() override = default;
+  ShareServiceImplUnittest() = default;
+  ~ShareServiceImplUnittest() override = default;
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
 
-    ShareServiceImpl::Create(mojo::GetProxy(&share_service_));
+    share_service_helper_ =
+        ShareServiceImplTestHelper::Create(mojo::GetProxy(&share_service_));
   }
 
-  void TearDown() override {
-    ChromeRenderViewHostTestHarness::TearDown();
-  }
+  void TearDown() override { ChromeRenderViewHostTestHarness::TearDown(); }
 
-  void DidShare(const base::Optional<std::string>& expected,
-                const base::Optional<std::string>& str) {
-    EXPECT_EQ(expected, str);
+  void DidShare(const std::string expected_target_url,
+                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_->last_used_target_url_;
+    EXPECT_EQ(expected_target_url, target_url);
 
     if (!on_callback_.is_null())
       on_callback_.Run();
   }
 
   blink::mojom::ShareServicePtr share_service_;
+  ShareServiceImplTestHelper* share_service_helper_;
   base::Closure on_callback_;
 };
 
 // Basic test to check the Share method calls the callback with the expected
 // parameters.
-TEST_F(ShareServiceTest, ShareCallbackParams) {
+TEST_F(ShareServiceImplUnittest, ShareCallbackParams) {
+  std::string target_url =

[Matt Giuca, 2016/12/21 07:13:50]

nit: name this expected_url.

[constantina, 2017/01/04 07:01:45]

Done.

+      "https://wicg.github.io/web-share-target/demos/"
+      "sharetarget.html?title=title&text=text&url=https%3A%2F%2Fwww.google.com%"
+      "2F";
+
   const GURL url("https://www.google.com");
+  base::Callback<void(const base::Optional<std::string>&)> callback =
+      base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
+                 target_url, base::Optional<std::string>());
 
   base::RunLoop run_loop;
   on_callback_ = run_loop.QuitClosure();
 
-  base::Callback<void(const base::Optional<std::string>&)> callback =
-      base::Bind(
-          &ShareServiceTest::DidShare, base::Unretained(this),
-          base::Optional<std::string>("Not implemented: navigator.share"));
   share_service_->Share("title", "text", url, callback);

[Matt Giuca, 2016/12/21 07:13:49]

Make title and text constants too.

In fact, these are kind of the same in all tests; why not make them global
consts and just reuse them in both tests (which would make the title "My title"
and "My text").

[constantina, 2017/01/04 07:01:45]

Done.

 
   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 std::string title = "My title";

[Matt Giuca, 2016/12/21 07:13:50]

As above, use const char kTitle[] = "My title", etc.

The static allocation thing doesn't really matter for tests but still good
practice.

[constantina, 2017/01/04 07:01:45]

Done.

+  const std::string text = "My text";
+  const GURL url("https://www.google.com/");
+
+  // One title placeholder

[Matt Giuca, 2016/12/21 07:13:50]

Nit: Full stops at the end.

[constantina, 2017/01/04 07:01:45]

Done.

+  std::string url_template = "%{title}";
+  std::string url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ("My%20title", url_template_filled);
+
+  // One text placeholder
+  url_template = "%{text}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ("My%20text", url_template_filled);
+
+  // One url placeholder
+  url_template = "%{url}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ("https%3A%2F%2Fwww.google.com%2F", url_template_filled);
+
+  // One of each placeholder, in title, text, url order
+  url_template = "%{title}%{text}%{url}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ("My%20titleMy%20texthttps%3A%2F%2Fwww.google.com%2F",
+            url_template_filled);
+
+  // One of each placeholder, in url, text, title order
+  url_template = "%{url}%{text}%{title}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ("https%3A%2F%2Fwww.google.com%2FMy%20textMy%20title",
+            url_template_filled);
+
+  // Two of each placeholder, some next to each other, others not
+  url_template = "%{title}%{url}%{text}%{text}%{title}%{url}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ(
+      "My%20titlehttps%3A%2F%2Fwww.google.com%2FMy%20textMy%20textMy%20"
+      "titlehttps%3A%2F%2Fwww.google.com%2F",
+      url_template_filled);
+}

[Matt Giuca, 2016/12/21 07:13:49]

Some other things to test:
- Badly nested placeholders: "%{title", "%{title{", "%{title %{url}}", "%{}".
- Unexpected placeholders: "%{badplaceholder}". These should just be substituted
with "", for future compatibility.
- Format string with no placeholder: "example.com".
- Empty format string: "".
- Format string with percent escapes in it: "%20%{title}" (to make sure you
don't crap out when you see a '%' that isn't followed by a '{').

And testing some other substitution strings:
- Containing percent-escapes: title="My%20title"; should expand to
"My%2520title".
- Containing "%{": title="My %{text}" substituted into "%{title}%{text}" should
become "My%20%25{text}My%20text", and not get double-substituted. (This would
create an illegal URL but that's not your concern.)
- Containing other characters. It might be worth having a string that simply has
all the ASCII characters, and note exactly which ones need to get escaped:
   
"\0\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f
!\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~"
(Maybe split control and non-control characters out into separate tests.)
- Containing Unicode characters (Chinese, etc). That needs to work.

[constantina, 2017/01/04 07:01:45]

Done.

+
+// Replace various numbers of placeholders in various orders, placed into a
+// query string, as a value. The expected use case.
+TEST_F(ShareServiceImplUnittest, ReplacePlaceholdersInQuery) {

[Matt Giuca, 2016/12/21 07:13:49]

I think this could be merged into the previous test. It isn't really necessary
to test both with a query and with no other letters (since there is no special
logic for queries).

Just put in the query syntax into the previous tests and you won't need this
one.

[constantina, 2017/01/04 07:01:45]

Not quite sure what you meant, so I took the last test in this function and
included in the previous. Deleted the rest.

+  const std::string title = "My title";
+  const std::string text = "My text";
+  const GURL url("https://www.google.com/");
+
+  // One title placeholder
+  std::string url_template = "?key=%{title}";
+  std::string url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ("?key=My%20title", url_template_filled);
+
+  // One text placeholder
+  url_template = "?key=%{text}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ("?key=My%20text", url_template_filled);
+
+  // One url placeholder
+  url_template = "?key=%{url}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ("?key=https%3A%2F%2Fwww.google.com%2F", url_template_filled);
+
+  // One of each placeholder, in title, text, url order
+  url_template = "?title=%{title}&text=%{text}&url=%{url}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ(
+      "?title=My%20title&text=My%20text&url=https%3A%2F%2Fwww.google.com%2F",
+      url_template_filled);
+
+  // One of each placeholder, in url, text, title order
+  url_template = "?url=%{url}&text=%{text}&title=%{title}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ(
+      "?url=https%3A%2F%2Fwww.google.com%2F&text=My%20text&title=My%20title",
+      url_template_filled);
+
+  // Two of each placeholder, some next to each other, others not
+  url_template =
+      "?title=%{title}&url=%{url}&text=%{text}&text=%{text}&"
+      "title=%{title}&url=%{url}";
+  url_template_filled =
+      ShareServiceImpl::replacePlaceholders(url_template, title, text, url);
+  EXPECT_EQ(
+      "?title=My%20title&url=https%3A%2F%2Fwww.google.com%2F&text=My%20text&"
+      "text=My%20text&title=My%20title&url=https%3A%2F%2Fwww.google.com%2F",
+      url_template_filled);
+}