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

chrome/browser/webshare/share_service_impl.h

 // Copyright 2016 The Chromium Authors. All rights reserved.

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

CL Description:

1. The title is misleading since this doesn't properly implement WS (since the
user can't choose).
2. Wrap to 72 chars.

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

Done.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_WEBSHARE_SHARE_SERVICE_IMPL_H_
 #define CHROME_BROWSER_WEBSHARE_SHARE_SERVICE_IMPL_H_
 
 #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"
 
 class GURL;
 
 // Desktop implementation of the ShareService Mojo service.
 class ShareServiceImpl : public blink::mojom::ShareService {
  public:
   ShareServiceImpl() = default;
   ~ShareServiceImpl() override = default;
 
   static void Create(mojo::InterfaceRequest<ShareService> request);
 
   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);
 };
 
 #endif  // CHROME_BROWSER_WEBSHARE_SHARE_SERVICE_IMPL_H_