Add a pref name for share targets, and store their manifest data.

chrome/browser/webshare/share_target_pref_helper.cc

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

[Matt Giuca, 2017/01/23 03:00:57]

Copyright 2017.

[constantina, 2017/01/24 02:03:42]

OMG. Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+

[Matt Giuca, 2017/01/23 03:00:57]

You need a .h file for this file.

Also you need to add both this and the header file to the BUILD file for
chrome/browser. And #include the header file from its .cc file.

[constantina, 2017/01/24 02:03:42]

Done.

+#include <sstream>
+
+#include "base/strings/string16.h"
+#include "base/values.h"
+#include "chrome/common/pref_names.h"
+#include "components/prefs/pref_service.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "content/public/common/manifest.h"
+
+namespace {

[Matt Giuca, 2017/01/23 03:00:57]

This can't be in an anonymous namespace, since it will be used by other modules.

(It just happens to work because you are directly #including the .cc file, but
you aren't allowed do that.)

[constantina, 2017/01/24 02:03:43]

Done. Used 'webshare'.

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

We haven't used the "webshare" namespace anywhere else. I think just leave this
bare for consistency. (i.e., ShareServiceImpl is not in any namespace.)

[constantina, 2017/01/24 23:39:04]

Done.

+
+// Adds the Web Share target |share_target_origin| with template |url_template|
+// to |pref_service| under kWebShareVisitedTargets. If |url_template| is null,
+// this function does nothing.

[Matt Giuca, 2017/01/23 03:00:58]

Obsolete comment? It removes the target from the pref.

[constantina, 2017/01/24 02:03:42]

Yeah, obsolete. Removed.

+void AddShareTargetToPrefs(std::string share_target_origin,

[Matt Giuca, 2017/01/23 03:00:57]

StringPiece?

[constantina, 2017/01/24 02:03:42]

Done.

+                           base::NullableString16 share_target_url_template,

[Matt Giuca, 2017/01/23 03:00:57]

Hmm, I think it would be more natural to accept a std::string (or rather a
base::StringPiece) here.

I think the NullableString16 is something sammc@ is trying to remove from
Manifest -- it is a weird anomaly which we're trying to get rid of AFAIK. It's
better for this interface to distance itself from the implementation details of
Manifest, and it's best for interfaces to take string, not string16, if they
need a string internally.

Do the 16-to-8-bit conversion in the calling code.

But you do care about optionality. Can this be a
base::Optional<base::StringPiece>?

[constantina, 2017/01/24 02:03:42]

I do care about optionality, because we want to allow empty url templates, and
want to distinguish between empty and non-existent. Changed to Optional.

+                           PrefService* pref_service) {
+  DictionaryPrefUpdate update(pref_service, prefs::kWebShareVisitedTargets);
+  base::DictionaryValue* share_target_dict = update.Get();
+
+  if (share_target_url_template.is_null()) {

[Matt Giuca, 2017/01/23 03:00:57]

nit: && rather than two if statements.

[constantina, 2017/01/24 02:03:42]

There are supposed to be two distinct cases here, and the return occurs in both;
do you want me to unnest them?
'''
if (share_target_url_template.is_null() &&
share_target_dict->HasKey(share_target_origin)) {
  share_target_dict->RemoveWithoutPathExpansion(share_target_origin, NULL);
  return;
}

if (share_target_url_template.is_null()) return;
'''

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

I see, I misread it.

No, don't do the same check twice. Instead, just add a blank line between that
inner if statement and the return, so it's a bit easier to follow.

[constantina, 2017/01/24 23:39:04]

Done.

+    if (share_target_dict->HasKey(share_target_origin))
+      share_target_dict->RemoveWithoutPathExpansion(share_target_origin, NULL);
+    return;
+  }
+
+  constexpr char kUrlTemplateKey[] = "url_template";
+
+  std::unique_ptr<base::DictionaryValue> origin_dict(new base::DictionaryValue);
+
+  std::stringstream ss;
+  ss << share_target_url_template.string();
+  std::string url_template;
+  ss >> url_template;

[Matt Giuca, 2017/01/23 03:00:57]

What's with the stringstream? Are you just doing this to convert the string16 to
a string?

Use UTF16ToUTF8 in utf_string_conversions.h.

[constantina, 2017/01/24 02:03:43]

Done.

+  origin_dict->SetStringWithoutPathExpansion(kUrlTemplateKey, url_template);
+
+  share_target_dict->SetWithoutPathExpansion(share_target_origin,

[Matt Giuca, 2017/01/23 03:58:56]

As discussed offline, this should actually be keyed to the full (absolute)
manifest URL, not just the origin (because there could be two separate manifests
with target apps on the same domain).

[constantina, 2017/01/24 02:03:43]

Done.

+                                             std::move(origin_dict));
+}
+
+}  // namespace