Picker takes a vector of pairs, and passes back the user chosen target.

chrome/browser/webshare/share_service_impl.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/webshare/share_service_impl.h"
 
 #include <algorithm>
 #include <functional>
 #include <utility>
 
@@ skipping 97 matching lines @@
   }
   split_template.push_back(url_template.substr(
       start_index_to_copy, url_template.size() - start_index_to_copy));
 
   *url_template_filled = JoinString(split_template);
   return true;
 }
 
 void ShareServiceImpl::ShowPickerDialog(
     const std::vector<base::string16>& targets,
-    const base::Callback<void(SharePickerResult)>& callback) {
+    const base::Callback<void(base::Optional<base::string16>)>& callback) {
   // TODO(mgiuca): Get the browser window as |parent_window|.
 #if defined(OS_LINUX) || defined(OS_WIN)
   chrome::ShowWebShareTargetPickerDialog(nullptr /* parent_window */, targets,
                                          callback);
 #else
-  callback.Run(SharePickerResult::CANCEL);
+  callback.Run(base::nullopt);
 #endif
 }
 
 void ShareServiceImpl::OpenTargetURL(const GURL& target_url) {
 // TODO(constantina): Prevent this code from being run/compiled in android.
 #if defined(OS_LINUX) || defined(OS_WIN)
   Browser* browser = BrowserList::GetInstance()->GetLastActive();
   chrome::AddTabAt(browser, target_url,
                    browser->tab_strip_model()->active_index() + 1, true);
 #endif
 }
 
 void ShareServiceImpl::Share(const std::string& title,
                              const std::string& text,
                              const GURL& share_url,
                              const ShareCallback& callback) {
   // TODO(constantina): Replace hard-coded name with the registered target list.
-  constexpr char kTargetName[] = "Web Share Target Test App";
-  std::vector<base::string16> targets{base::ASCIIToUTF16(kTargetName)};
+  constexpr char kUrlBase[] = "https://wicg.github.io/web-share-target/demos/";

[Matt Giuca, 2017/02/01 07:04:33]

This makes it ugly: you are no longer displaying the title of the app, but
rather the URL template.

You will need either another CL (or just do it here, up to you) where you pass
in BOTH a display name for use in the table, and a URL string to be returned. If
you want to do it in a follow-up CL, add a TODO here (replace the above TODO
since it's a bit out of date).

[constantina, 2017/02/01 23:24:21]

Done.

+  std::vector<base::string16> targets{base::ASCIIToUTF16(kUrlBase)};

[Matt Giuca, 2017/02/01 07:04:33]

Also I'm confused what the actual URL is that you're passing into (and getting
back from) the dialog. The documentation (in 3 places) says "manifest URL", but
this is the base URL. I think previously we discussed this being the full
template with placeholders -- NOT the manifest URL or the base URL.

(See my comment below.)

Update the documentation to be consistent.

[Matt Giuca, 2017/02/01 07:25:23]

Had an offline chat: the summary is that this will all be sorted out by
https://codereview.chromium.org/2664033002/ (where it *will* use the manifest
URL to identify targets everywhere, which I think is nice).

In the meantime, just change this CL to be consistent with that eventual plan,
even though it does mean hard-coding the manifest URL here, and then
(temporarily) ignoring the result of the dialog in OnPickerClosed and
hard-coding the URL template.

[constantina, 2017/02/01 23:24:21]

Done.

 
   ShowPickerDialog(targets, base::Bind(&ShareServiceImpl::OnPickerClosed,
                                        base::Unretained(this), title, text,
                                        share_url, callback));
 }
 
 void ShareServiceImpl::OnPickerClosed(const std::string& title,
                                       const std::string& text,
                                       const GURL& share_url,
                                       const ShareCallback& callback,
-                                      SharePickerResult result) {
-  if (result == SharePickerResult::CANCEL) {
+                                      base::Optional<base::string16> result) {
+  if (!result.has_value()) {
     callback.Run(base::Optional<std::string>("Share was cancelled"));
     return;
   }
 
-  // TODO(constantina): replace hard-coded URL with one from user-chosen site.
-  constexpr char kUrlBase[] = "https://wicg.github.io/web-share-target/";
   constexpr char kUrlTemplate[] =
-      "demos/sharetarget.html?title={title}&text={text}&url={url}";
+      "sharetarget.html?title={title}&text={text}&url={url}";

[Matt Giuca, 2017/02/01 07:04:33]

This template part of the URL is still hard-coded here. That should be going
away with this CL: you will keep the hard-coded string in Share(), for the time
being, because we do not yet have data to populate the dialog with. But you
should not have anything hard-coded in here because you should be getting back
whatever site the user chose.

In other words, if you need to hard-code anything here, it suggests the data
model is wrong, because it won't be able to vary with the user's choice.

I think the template should be concatenated onto the base URL before putting it
into the vector, and then when the user makes a choice, you get that absolute
template URL back as a result.

[constantina, 2017/02/01 23:24:21]

Done as per your last comment.

 
   std::string url_template_filled;
   if (!ReplacePlaceholders(kUrlTemplate, title, text, share_url,
                            &url_template_filled)) {
     callback.Run(base::Optional<std::string>(
         "Error: unable to replace placeholders in url template"));
     return;
   }
 
-  GURL target_url(kUrlBase + url_template_filled);
+  std::string url_base = base::UTF16ToASCII(result.value());

[Matt Giuca, 2017/02/01 07:17:23]

UTF16ToUTF8

Otherwise this will fail if it contains non-ASCII characters. OK, it's a URL so
"shouldn't" contain non-ASCII characters but I think they can do.

[constantina, 2017/02/01 23:24:21]

Done.

+  GURL target_url(url_base + url_template_filled);
   if (!target_url.is_valid()) {
     callback.Run(base::Optional<std::string>(
         "Error: url of share target is not a valid url."));
     return;
   }
   OpenTargetURL(target_url);
 
   callback.Run(base::nullopt);
 }