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..01ba0908c7a12e46ce6fefe0f0cb36306fdc6e59 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,131 @@ void ShareServiceImpl::Create(blink::mojom::ShareServiceRequest request) {
                           std::move(request));
 }
 
+// static
+int ShareServiceImpl::ReplacePlaceholders(const std::string& url_template,
+                                          const std::string& title,
+                                          const std::string& text,
+                                          const GURL& share_url,
+                                          std::string* url_template_filled) {
+  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);
+
+  constexpr char kTitlePlaceholder[] = "%{title}";

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

It would be much nicer I think if we could avoid having the "%{" and "}" here
(and in the map). Can we just make it so these constants are "title" rather than
"%{title}"?

[constantina, 2017/01/09 02:12:43]

Done.

+  constexpr char kTextPlaceholder[] = "%{text}";
+  constexpr char kUrlPlaceholder[] = "%{url}";
+
+  std::vector<base::StringPiece> placeholders = {
+      kTitlePlaceholder, kTextPlaceholder, kUrlPlaceholder};
+
+  std::vector<base::StringPiece> share_data = {title_escaped, text_escaped,
+                                               share_url_escaped};
+
+  std::map<base::StringPiece, base::StringPiece> placeholder_to_data;
+  for (size_t i = 0; i < placeholders.size(); ++i) {

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

I think this would read much clearer if it was just explicit rather than
building all these data structures first:

placeholder_to_data[kTitlePlaceholder] = net::EscapeQueryParamValue(title,
false);
placeholder_to_data[kTextPlaceholder] = net::EscapeQueryParamValue(text, false);
placeholder_to_data[kUrlPlaceholder] =
net::EscapeQueryParamValue(share_url.spec(), false);

Then should be able to delete all the stuff above.

Failing that, I would rather those vectors be a vector of pairs rather than two
separate vectors.

[constantina, 2017/01/09 02:12:42]

Done.

+    placeholder_to_data[placeholders[i]] = share_data[i];
+  }
+
+  // Find open ("%{") and close ("}") identifiers, and record indices.
+  // If two opens occur before a close, or a close occurs with no preceding
+  // open, return an error code.

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

I think we could change this rule to *only* allow identifier characters (ASCII
letters, numbers and maybe '-' and/or '_') inside these braces. There's no
future in which we want to allow all kinds of random characters in there.

That might simplify this algorithm a bit. Instead of explicitly erroring if we
see a "%{", we error if we see any non-identifier character at all (essentially
|last_saw_open| changes to a different mode where we are only looking for
identifiers or '}').

[constantina, 2017/01/09 02:12:43]

Done. Not sure if this is the way you wanted it implemented.

+  std::vector<int> placeholder_open_indices;

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

Can we make this a std::vector<std::pair<int, int>> instead of two separate
vectors?

I'm not really a fan of having two vectors that *must* be the same length and
making an assumption about that. It would make the code below a bit neater.

Failing that, add a DCHECK below when you loop over these vectors to check that
they are the same length.

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

std::vector<base::StringPiece> is a nice way to model string segments within a
larger string.

[Matt Giuca, 2017/01/05 05:00:13]

I agree, that sounds good, as long as you can figure out which indices of the
original string those pieces point to.

[constantina, 2017/01/09 02:12:42]

Reimplemented algo to allow for this.

+  std::vector<int> placeholder_close_indices;
+  bool last_saw_open = false;
+  for (size_t i = 0; i < url_template.size(); ++i) {
+    if (url_template[i] == '%' && i < url_template.size() &&

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

i + 1 < url_template.size()
  ^^^

[constantina, 2017/01/09 02:12:42]

Done.

+        url_template[i + 1] == '{') {
+      if (last_saw_open) {
+        *url_template_filled = url_template;

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

Why populate |url_template_filled| at all on error? Conventionally you just
leave outputs unchanged if there is an error.

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

Don't change |*url_template_filled| on failure.

[constantina, 2017/01/09 02:12:42]

Done.

+        return 1;

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

Please comment each error case why it is an error.

e.g.:
// Nested template parameters are not allowed.

[constantina, 2017/01/09 02:12:43]

Done.

+      }
+      last_saw_open = true;
+      placeholder_open_indices.push_back(i);
+    } else if (url_template[i] == '}') {
+      if (!last_saw_open) {

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

We could just not make this an error. It isn't necessarily bad to have a '}'
character outside of a template parameter sequence. It would simplify the
algorithm and your description of it if we just don't make this an error.

On the flip side, '}' is an illegal character in URL syntax, so it should never
appear outside of the template parameters. However, it isn't our job to police
illegal characters (or else we should error on '<' and '>' too, for example). So
I'd say just remove this rule.

[constantina, 2017/01/09 02:12:43]

Done.

+        *url_template_filled = url_template;
+        return 1;
+      }
+      last_saw_open = false;
+      placeholder_close_indices.push_back(i);
+    }
+  }
+  if (last_saw_open) {
+    *url_template_filled = url_template;
+    return 1;

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

Comment why this is an error.

[constantina, 2017/01/09 02:12:42]

Done.

+  }
+
+  // Reserve memory for filled template, assuming all placeholders are replaced

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

This function is getting pretty big. Maybe break off a separate function to
calculate the size of the allocation, and maybe break some other things out into
functions.

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

Use the real sizes.

[Matt Giuca, 2017/01/05 05:00:13]

Do you mean actually calculate the final size before building the string (rather
than just doing the max as is done here)? It seems like more work than it's
worth?

[Sam McNally, 2017/01/05 05:41:49]

Yes. The mapping from placeholder to value can be done in the initial pass
through the template URL.

[constantina, 2017/01/09 02:12:42]

Incorporated as part of the new implementation.

+  // with the largest share data field.
+  int placeholder_char_count = 0;

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

I would suggest not to worry about this. You don't need to allocate the exact
amount of memory required. Subtracting the typically ~20 bytes of placeholder
characters isn't really worth this complexity.

[constantina, 2017/01/09 02:12:43]

New implementation means this is easily accounted for, with no added code
complexity.

+  for (size_t i = 0; i < placeholder_open_indices.size(); ++i) {
+    placeholder_char_count +=
+        placeholder_close_indices[i] - placeholder_open_indices[i] + 1;
+  }
+  size_t largest_share_datum = 0;

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

Comment here to explain why you're doing this.

[constantina, 2017/01/09 02:12:43]

Gone with new impl.

+  for (base::StringPiece datum : share_data) {
+    if (datum.size() >= largest_share_datum) {
+      largest_share_datum = datum.size();
+    }
+  }
+  *url_template_filled = "";

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

->clear();

[constantina, 2017/01/09 02:12:42]

Done.

+  url_template_filled->reserve(url_template.size() - placeholder_char_count +
+                               largest_share_datum *
+                                   placeholder_open_indices.size());
+
+  // Look at each placeholder, and check if it is valid.

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

What does "valid" mean? You mean one of the recognised strings? Maybe choose a
different word.

[constantina, 2017/01/09 02:12:42]

Gone.

+  // If it is, replace the placeholder with its corresponding share datum.
+  // Otherwise, replace it with an empty string.
+  int start_index_to_copy = 0;
+  for (size_t i = 0; i < placeholder_open_indices.size(); ++i) {
+    int placeholder_open = placeholder_open_indices[i];
+    int placeholder_close = placeholder_close_indices[i];
+
+    *url_template_filled += url_template.substr(
+        start_index_to_copy, placeholder_open - start_index_to_copy);
+
+    base::StringPiece placeholder = url_template.substr(

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

This is dangling unless |url_template| is a StringPiece.

[constantina, 2017/01/09 02:12:43]

Gone.

+        placeholder_open, placeholder_close + 1 - placeholder_open);
+
+    std::map<base::StringPiece, base::StringPiece>::iterator it =
+        placeholder_to_data.find(placeholder);
+    if (it != placeholder_to_data.end()) {
+      it->second.AppendToString(url_template_filled);
+    }
+    start_index_to_copy = placeholder_close + 1;
+  }
+  *url_template_filled += url_template.substr(start_index_to_copy);
+  return 0;
+}
+
+void ShareServiceImpl::OpenTargetURL(const 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"));
+  bool error_occured = false;
+
+  const char kUrlBase[] = "https://wicg.github.io/web-share-target/";
+  std::string url_template =

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

constexpr char kUrlTemplate[] =

[constantina, 2017/01/09 02:12:42]

Done.

[Matt Giuca, 2017/01/09 03:56:54]

You still need to rename to kUrlTemplate.

[constantina, 2017/01/09 23:51:03]

Done.

+      "demos/sharetarget.html?title=%{title}&text=%{text}&url=%{url}";
+
+  std::string url_template_filled;
+  int error = ReplacePlaceholders(url_template, title, text, share_url,
+                                  &url_template_filled);
+  error_occured |= error;

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

No point having error_occurred if it only gets set once (this |= pattern might
be useful if you have a number of things that can go wrong but for now it is
unnecessary).

If you change the result of ReplacePlaceholders to bool (as I suggested above)
this will be easier -- just declare and set error_occurred in this one line.

[constantina, 2017/01/09 02:12:43]

Done.

+
+  GURL target_url(kUrlBase + url_template_filled);
+  OpenTargetURL(target_url);

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

Why open the URL if substitution fails?

[constantina, 2017/01/09 02:12:42]

Done.

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