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

chrome/browser/webshare/share_service_impl.cc

Index: chrome/browser/webshare/share_service_impl.cc
diff --git a/chrome/browser/webshare/share_service_impl.cc b/chrome/browser/webshare/share_service_impl.cc
index 9f8bb0b690e52854f780fdcc5373e8efd5062a3b..580f166c6ca08a7fa26dd5bc8a118f089c5c1588 100644
--- a/chrome/browser/webshare/share_service_impl.cc
+++ b/chrome/browser/webshare/share_service_impl.cc
@@ -4,7 +4,17 @@
 
 #include "chrome/browser/webshare/share_service_impl.h"
 
+#include <algorithm>
+#include <functional>
+#include <utility>
+
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
+#include "net/base/escape.h"
 
 // static
 void ShareServiceImpl::Create(blink::mojom::ShareServiceRequest request) {
@@ -12,11 +22,88 @@ void ShareServiceImpl::Create(blink::mojom::ShareServiceRequest request) {
                           std::move(request));
 }
 
+// static
+std::string ShareServiceImpl::replacePlaceholders(

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

As Sam suggested, you should just search (in one pass) for all "%{" sequences
and mark them (regardless of what they contain). This allows you to properly
deal with non-standard fields (which should become empty, for future
compatibility) and avoid multiple passes over the string.

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

Done.

+    const std::string url_template,
+    const std::string title,
+    const std::string text,
+    const GURL& share_url) {
+  std::string title_escaped = net::EscapeQueryParamValue(title, false);
+  std::string text_escaped = net::EscapeQueryParamValue(text, false);
+  std::string share_url_escaped =
+      net::EscapeQueryParamValue(share_url.spec(), false);
+
+  std::string title_placeholder = "%{title}";

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

Nit: kTitlePlaceholder, kTextPlaceholder and kUrlPlaceholder.

(Even when local variables, still follow this convention.)

Also you should not have const std::strings; instead the convention is:

const char kTitlePlaceholder[] = "%{title}";

Rationale is that this can be allocated into static memory rather than being
dynamically created every time the function runs.

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

Better yet, use constexpr char kTitlePlaceholder[].

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

Done.

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

Done.

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

Sam: Is your advice just to always use constexpr in place of const if the RHS is
a compile-time constant?

[Sam McNally, 2017/01/05 04:08:37]

Yes, if constexpr is supported for the type.

+  std::string text_placeholder = "%{text}";
+  std::string url_placeholder = "%{url}";
+
+  std::vector<std::string> placeholders;
+  placeholders.push_back(title_placeholder);
+  placeholders.push_back(text_placeholder);
+  placeholders.push_back(url_placeholder);
+
+  std::string url_template_filled = url_template;
+
+  // Fill placeholder_positions with pair of position of ith placeholder in
+  // |placeholders|, and ith placeholder in |placeholders|.
+  std::vector<std::pair<int, std::string>> placeholder_positions;

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

Prefer base::StringPiece when you can refer to part of an existing string.

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

Done. Also changed |placeholders|.

+  for (std::string placeholder : placeholders) {

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

const std::string&

or const auto&

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

Done.

+    std::size_t found = url_template_filled.find(placeholder);
+    while (found != std::string::npos) {
+      placeholder_positions.push_back(
+          std::pair<int, std::string>(found, placeholder));
+      found = url_template_filled.find(placeholder, found + 1);
+    }
+  }
+
+  // Sort placeholders by their index in the url template, in descending order.
+  // Processing in descending order means that when the latter placeholders are
+  // replaced, the positions of the earlier placeholders are not affected.
+  std::sort(placeholder_positions.begin(), placeholder_positions.end());
+  std::reverse(placeholder_positions.begin(), placeholder_positions.end());
+
+  for (std::pair<int, std::string> placeholder : placeholder_positions) {

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

For the second pass, you can avoid doing replacements on the string (in reverse)
by simply building a brand new string from scratch, copying over the
non-replaced segments of the old string into the new one.

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

Done.

+    int found = placeholder.first;
+    // Replace the placeholder with its corresponding share datum.
+    if (placeholder.second == title_placeholder) {
+      url_template_filled.replace(found, placeholder.second.size(),
+                                  title_escaped);
+    } else if (placeholder.second == text_placeholder) {
+      url_template_filled.replace(found, placeholder.second.size(),
+                                  text_escaped);
+    } else if (placeholder.second == url_placeholder) {
+      url_template_filled.replace(found, placeholder.second.size(),
+                                  share_url_escaped);
+    }
+  }
+  return url_template_filled;
+}
+
+void ShareServiceImpl::openTargetURL(GURL target_url) {
+  Browser* browser = BrowserList::GetInstance()->GetLastActive();
+  chrome::AddTabAt(browser, target_url,
+                   browser->tab_strip_model()->active_index() + 1, true);
+}
+
 void ShareServiceImpl::Share(const std::string& title,
                              const std::string& text,
-                             const GURL& url,
+                             const GURL& share_url,
                              const ShareCallback& callback) {
-  // TODO(constantina): Implement Web Share Target here.
-  NOTIMPLEMENTED();
-  callback.Run(base::Optional<std::string>("Not implemented: navigator.share"));
+  std::string url_base = "https://wicg.github.io/web-share-target/";

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

const char kUrlBase[]

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

Done.

+  std::string url_template =
+      "demos/sharetarget.html?title=%{title}&text=%{text}&url=%{url}";
+
+  std::string url_template_filled =
+      replacePlaceholders(url_template, title, text, share_url);
+
+  GURL target_url(url_base + url_template_filled);
+  openTargetURL(target_url);
+
+  // TODO(constantina): Check for errors.

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

You should do this in this CL. If you don't have any errors, this logic should
be simplified so it doesn't check at all.

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

Done.

+  bool error_occured = false;
+  if (error_occured) {
+    callback.Run(base::Optional<std::string>("Error"));
+  } else {
+    callback.Run(base::Optional<std::string>());
+  }
 }