Read share targets from prefstore, and filter by engagement.

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 94f7b90bbac82a5778364337fa43c69e64bff108..ba766f56e3f1dba1dd9ade4459bd35bd098f22e7 100644
--- a/chrome/browser/webshare/share_service_impl.cc
+++ b/chrome/browser/webshare/share_service_impl.cc
@@ -10,12 +10,15 @@
 
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/engagement/site_engagement_service.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_dialogs.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 "chrome/common/pref_names.h"
+#include "components/prefs/pref_service.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "net/base/escape.h"
 
@@ -125,22 +128,71 @@ void ShareServiceImpl::ShowPickerDialog(
 #endif
 }
 
+Browser* ShareServiceImpl::GetBrowser() {
+  return BrowserList::GetInstance()->GetLastActive();
+}
+
 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);
+  chrome::AddTabAt(GetBrowser(), target_url,

[Matt Giuca, 2017/02/01 07:30:20]

You should still assign this to a Browser* local variable, to avoid calling
GetLastActive() twice.

[constantina, 2017/02/02 00:43:47]

Done.

+                   GetBrowser()->tab_strip_model()->active_index() + 1, true);
 #endif
 }
 
+std::string ShareServiceImpl::GetTargetTemplate(std::string target_url) {
+  const base::DictionaryValue* share_target_dict =
+      GetPrefService()->GetDictionary(prefs::kWebShareVisitedTargets);
+
+  const base::DictionaryValue* share_target_info_dict = nullptr;
+  share_target_dict->GetDictionaryWithoutPathExpansion(target_url,
+                                                       &share_target_info_dict);
+
+  std::string url_template;
+  share_target_info_dict->GetString("url_template", &url_template);
+  return url_template;
+}
+
+PrefService* ShareServiceImpl::GetPrefService() {
+  return GetBrowser()->profile()->GetPrefs();
+}
+
+blink::mojom::EngagementLevel ShareServiceImpl::GetEngagementLevel(GURL url) {
+  SiteEngagementService* site_engagement_service =
+      SiteEngagementService::Get(GetBrowser()->profile());
+  return site_engagement_service->GetEngagementLevel(url);
+}
+
+// static
+std::vector<std::string>
+ShareServiceImpl::GetTargetsWithSufficientEngagement() {
+  const base::DictionaryValue* dict =
+      GetPrefService()->GetDictionary(prefs::kWebShareVisitedTargets);
+
+  std::vector<std::string> most_engaged_targets;

[Matt Giuca, 2017/02/01 07:30:20]

Not really the right name (it implies that you'll pick the top N targets,
whereas you actually pick all targets with sufficient engagement.

sufficiently_engaged_targets?

[constantina, 2017/02/02 00:43:47]

Done.

+
+  for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) {

[Matt Giuca, 2017/02/01 07:30:20]

auto?

Also, what a strange iterator protocol!

[constantina, 2017/02/02 00:43:48]

Compiler cries.

Ikr!

[Matt Giuca, 2017/02/02 02:57:10]

Oh, right, you can't call auto because you're actually calling the Iterator
constructor, as opposed to some kind of begin() method on the dict itself. (And
this is the only way.) That's... pretty junk.

Acknowledged.

+    GURL manifest_url(it.key());

[Matt Giuca, 2017/02/01 07:30:20]

This can probably be a const GURL& yes?

[constantina, 2017/02/02 00:43:47]

Done. Just const, no "&". The key is a string, not a GURL, so I think that's why
"&" won't work.

[Matt Giuca, 2017/02/02 02:57:10]

Oh OK, then could just remove the const as well (I see much less value in having
a const value rather than a const ref). But you can also keep const as well; up
to you.

[constantina, 2017/02/02 05:57:07]

Removed

+    if (GetEngagementLevel(manifest_url) >=
+        blink::mojom::EngagementLevel::LOW) {

[Matt Giuca, 2017/02/01 07:30:20]

Can this be a constant (e.g., kMinimumEngagementLevel) at the top of the file in
an anonymous namespace?

(This is kind of my opinion; others would probably tell you to keep it here, but
I like for arbitrary tunable constants to be in a more visible spot.)

[constantina, 2017/02/02 00:43:47]

I moved it to the top of the function.

[Matt Giuca, 2017/02/02 02:57:10]

Acknowledged.

+      most_engaged_targets.push_back(it.key());
+    }
+  }
+
+  return most_engaged_targets;
+}
+
 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 kUrlBase[] = "https://wicg.github.io/web-share-target/demos/";
-  std::vector<base::string16> targets{base::ASCIIToUTF16(kUrlBase)};
+  std::vector<std::string> most_engaged_targets =

[Matt Giuca, 2017/02/01 07:30:20]

Same name as previous function.

[constantina, 2017/02/02 00:43:47]

Done.

+      GetTargetsWithSufficientEngagement();
+
+  std::vector<base::string16> targets;
+  for (const std::string& target : most_engaged_targets) {
+    targets.push_back(base::ASCIIToUTF16(target));

[Matt Giuca, 2017/02/01 07:30:20]

UTF8ToUTF16

You should never use ASCIIToUTF16 unless it is a hard-coded ASCII literal. (And
generally never use UTF16ToASCII, always UTF16ToUTF8.) This will fail (via
DCHECK -- i.e., in release simply misbehave) if there are UTF-8 characters in
it.

[constantina, 2017/02/02 00:43:47]

Done.

+  }
 
   ShowPickerDialog(targets, base::Bind(&ShareServiceImpl::OnPickerClosed,
                                        base::Unretained(this), title, text,
@@ -157,25 +209,29 @@ void ShareServiceImpl::OnPickerClosed(const std::string& title,
     return;
   }
 
-  constexpr char kUrlTemplate[] =
-      "sharetarget.html?title={title}&text={text}&url={url}";
+  std::string chosen_target = base::UTF16ToASCII(result.value());

[Matt Giuca, 2017/02/01 07:30:20]

UTF16ToUTF8

[constantina, 2017/02/02 00:43:48]

Done.

 
+  std::string url_template = GetTargetTemplate(chosen_target);

[Matt Giuca, 2017/02/01 07:30:20]

I'm a big fan of this approach versus what we discussed the other day: to use
the manifest URL as the unique identifier of a share target everywhere (up to
and including OnPickerClosed) and then doing a lookup here to get the URL
template.

[constantina, 2017/02/02 00:43:47]

:D It has turned out to be quite neat; I like that it doesn't give unnecessary
info to the picker.

   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"));
+  if (!ReplacePlaceholders(url_template, title, text, share_url,
+                          &url_template_filled)) {
+    callback.Run(
+        base::Optional<std::string>("Error: placeholders in share target's url "

[Matt Giuca, 2017/02/01 07:30:20]

Is there a reason to change this string?

[constantina, 2017/02/02 00:43:48]

No, I just lost it in a rebase, wrote something similar for temporary testing
purposes, and forgot to check what it actually used to be.

Changed back.

+                                    "template could not be replaced."));
     return;
   }
 
-  std::string url_base = base::UTF16ToASCII(result.value());
-  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."));
+  base::StringPiece url_base(

[Matt Giuca, 2017/02/01 07:30:20]

// The template is relative to the manifest URL (minus the filename).
Concatenate to make an absolute URL.

[constantina, 2017/02/02 00:43:47]

Done.

+      chosen_target.data(),
+      chosen_target.size() - GURL(chosen_target).ExtractFileName().size());
+  const GURL target(url_base.as_string() + url_template_filled);
+  if (!target.is_valid()) {
+    callback.Run(
+        base::Optional<std::string>("Error: chosen share target's url template "
+                                    "does not result in a valid url."));
     return;
   }
-  OpenTargetURL(target_url);
+  OpenTargetURL(target);
 
   callback.Run(base::nullopt);
 }