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

chrome/browser/webshare/share_service_impl.h

Index: chrome/browser/webshare/share_service_impl.h
diff --git a/chrome/browser/webshare/share_service_impl.h b/chrome/browser/webshare/share_service_impl.h
index 83de10f9511ff36394283b8f200f331ae0dad769..ad08806bd39f0952809ca0889337e371ba57063c 100644
--- a/chrome/browser/webshare/share_service_impl.h
+++ b/chrome/browser/webshare/share_service_impl.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/gtest_prod_util.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "third_party/WebKit/public/platform/modules/webshare/webshare.mojom.h"
 
@@ -22,10 +23,23 @@ class ShareServiceImpl : public blink::mojom::ShareService {
 
   void Share(const std::string& title,

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

Add a comment:
// ShareService overrides:

(By convention, all overrides from a certain base class should have a similar
comment at the top of the block.)

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

Done. The following example I found had the full namespace, so i did that:
https://cs.chromium.org/chromium/src/services/ui/ws/gpu_host.cc?l=55&dr=C

              const std::string& text,
-             const GURL& url,
+             const GURL& share_url,

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

Why not just |url|? 

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

The implementation references lots of different types of urls: url_template,
url_template_filled, url_base, target_url. I suppose those are unambiguously
named, and the only url left is the one being shared, so I can change it to just
be url. I think shared_url is even better than share_url, but not sure about
past tense conventions. What do you think?

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

Right, makes sense. But you should not change the names of parameters when
overriding a method (that is valid, but confusing).

Feel free to disambiguate this in all of your internal methods here (like
replacePlaceholders) but don't change this name here (or in the .cc file for
this method).
I agree shared_url is better.

              const ShareCallback& callback) override;
 
+  virtual void openTargetURL(GURL target_url);

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

nit: Names should start with a capital letter.

This should be protected (because it is only used by the subclass). Document
what it does, and mention that it is virtual for testing.

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

Done.

+
  private:
+  FRIEND_TEST_ALL_PREFIXES(ShareServiceImplUnittest, ReplacePlaceholders);

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

👍

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

👍

+  FRIEND_TEST_ALL_PREFIXES(ShareServiceImplUnittest,
+                           ReplacePlaceholdersInQuery);
+
+  // Replaces all instances of "%{title}", "%{text}", and "%{url}" (quotes for
+  // clarity) in |url_template| with |title|, |text|, and |url| respectively.
+  static std::string replacePlaceholders(const std::string url_template,

[Sam McNally, 2016/12/22 07:03:50]

ReplacePlaceholders
base::StringPiece

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

Changed Capitalisation. Did you want me to change url_template, or the returned
value to base::StringPiece? Or both? All the parameters?

+                                         const std::string title,
+                                         const std::string text,
+                                         const GURL& share_url);
+
   DISALLOW_COPY_AND_ASSIGN(ShareServiceImpl);
 };