Store target app name in Web Share prefs, and add extra logic.

Store target app name in Web Share prefs, and remove external logic.

UpdateShareTargetInPrefs now stores the name field of the supplied
target's manifest in the Web Share target prefs. For the given share
target, adds the name to the dictionary associated with the target.

UpdateShareTargetInPrefs also handles the logic about web apps with
manifests that don't have the share_target field, entirely internally.

BUG=668389
Review-Url: https://codereview.chromium.org/2679523002
Cr-Commit-Position: refs/heads/master@{#448568}
Committed: https://chromium.googlesource.com/chromium/src/+/2883697c7814c4fca4675bd071161498b3b6aa27

[constantina, 2017-02-06 00:16:13]

Description was changed from

==========
Store target app name in UpdateShareTargetInPrefs, and add extra logic.

UpdateShareTargetInPrefs stores the name field of the supplied target's
manifest.

UpdateShareTargetInPrefs now takes an entire manifest object, instead of
just the optional url_template. This allows UpdateShareTargetInPrefs to
handle the logic about web apps with manifests that don't have the
share_target field.

BUG=668389
==========

to

==========
Store target app name in UpdateShareTargetInPrefs, and add extra logic.

UpdateShareTargetInPrefs also stores the name field of the supplied
target's manifest in prefs. (See documentation of
UpdateShareTargetInPrefs.)

UpdateShareTargetInPrefs now takes an entire manifest object, instead of
just the optional url_template. This allows UpdateShareTargetInPrefs to
handle the logic about web apps with manifests that don't have the
share_target field.

BUG=668389
==========

[constantina, 2017-02-06 00:16:13]

constantina@google.com changed reviewers:
+ benwells@chromium.org, mgiuca@chromium.org

[constantina, 2017-02-06 00:19:31]

PTAL

[Matt Giuca, 2017-02-06 00:40:12]

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
CL description:

This is a bit confusing because it talks about the specific changes to the code
the CL is making (which doesn't make sense unless you know what these functions
already do). Rewrite it to be about what the change is really doing at a high
level.

That is: it's now going to store the app name from the manifest in the web share
target prefs. The refactor of UpdateShareTargetInPrefs is incidental and doesn't
really need to be mentioned (but could be mentioned in a little paragraph by
itself; doesn't need this much detail).

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:588: if
(!ChromeOriginTrialPolicy().IsFeatureDisabled("WebShare")) {
Drop the braces (for consistency with surrounding code).

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:589:
UpdateShareTargetInPrefs(manifest_url, manifest, profile_->GetPrefs());
Nice refactor.

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
File chrome/browser/webshare/share_target_pref_helper.cc (right):

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper.cc:23:
share_target_dict->RemoveWithoutPathExpansion(manifest_url.spec(), NULL);
s/NULL/nullptr

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right):

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:43: std::string
url_template = "share/?title={title}";
Just make this a base::string16 and do the conversion here rather than later.

Also you could use ASCIIToUTF16 here since it's a hard-coded constant.

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:48:
base::Optional<content::Manifest::ShareTarget>(share_target);
There is no name set, or tested, in any of these manifests.

Maybe add a name to one (but not both) of these manifests, then test that the
name is correctly set on the first one's prefs, and absent on the second one's
prefs. Don't bother to add names to the remaining tests.

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:62:
EXPECT_EQ(url_template, url_template_in_dict);
Good catch.

[constantina, 2017-02-07 04:44:41]

Description was changed from

==========
Store target app name in UpdateShareTargetInPrefs, and add extra logic.

UpdateShareTargetInPrefs also stores the name field of the supplied
target's manifest in prefs. (See documentation of
UpdateShareTargetInPrefs.)

UpdateShareTargetInPrefs now takes an entire manifest object, instead of
just the optional url_template. This allows UpdateShareTargetInPrefs to
handle the logic about web apps with manifests that don't have the
share_target field.

BUG=668389
==========

to

==========
Store target app name in Web Share prefs, and remove external logic.

UpdateShareTargetInPrefs now stores the name field of the supplied
target's manifest in the Web Share target prefs. For the given share
target, adds the name to the dictionary associated with the target.

UpdateShareTargetInPrefs also handles the logic about web apps with
manifests that don't have the share_target field, entirely internally.

BUG=668389
==========

[constantina, 2017-02-07 05:16:25]

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2017/02/06 00:40:11, Matt Giuca wrote:
> CL description:
> 
> This is a bit confusing because it talks about the specific changes to the
code
> the CL is making (which doesn't make sense unless you know what these
functions
> already do). Rewrite it to be about what the change is really doing at a high
> level.
> 
> That is: it's now going to store the app name from the manifest in the web
share
> target prefs. The refactor of UpdateShareTargetInPrefs is incidental and
doesn't
> really need to be mentioned (but could be mentioned in a little paragraph by
> itself; doesn't need this much detail).

Done.

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:588: if
(!ChromeOriginTrialPolicy().IsFeatureDisabled("WebShare")) {
On 2017/02/06 00:40:11, Matt Giuca wrote:
> Drop the braces (for consistency with surrounding code).

Done.

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:589:
UpdateShareTargetInPrefs(manifest_url, manifest, profile_->GetPrefs());
On 2017/02/06 00:40:11, Matt Giuca wrote:
> Nice refactor.

Ty!

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
File chrome/browser/webshare/share_target_pref_helper.cc (right):

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper.cc:23:
share_target_dict->RemoveWithoutPathExpansion(manifest_url.spec(), NULL);
On 2017/02/06 00:40:11, Matt Giuca wrote:
> s/NULL/nullptr

Done.

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right):

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:43: std::string
url_template = "share/?title={title}";
On 2017/02/06 00:40:12, Matt Giuca wrote:
> Just make this a base::string16 and do the conversion here rather than later.
> 
> Also you could use ASCIIToUTF16 here since it's a hard-coded constant.

If I do that, I have to convert back for the comparison to the
url_template_in_dict.

Used ASCIIToUTF16.

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:48:
base::Optional<content::Manifest::ShareTarget>(share_target);
On 2017/02/06 00:40:12, Matt Giuca wrote:
> There is no name set, or tested, in any of these manifests.
> 
> Maybe add a name to one (but not both) of these manifests, then test that the
> name is correctly set on the first one's prefs, and absent on the second one's
> prefs. Don't bother to add names to the remaining tests.

Done.

https://codereview.chromium.org/2679523002/diff/20001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:62:
EXPECT_EQ(url_template, url_template_in_dict);
On 2017/02/06 00:40:11, Matt Giuca wrote:
> Good catch.

Ty

[Matt Giuca, 2017-02-07 05:19:36]

https://codereview.chromium.org/2679523002/diff/60001/chrome/browser/webshare...
File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right):

https://codereview.chromium.org/2679523002/diff/60001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:63:
EXPECT_EQ(url_template, url_template_in_dict);
Get kNameKey and verify that it is... I guess EXPECT_FALSE on GetString? (Check
that it is not stored.)

[constantina, 2017-02-07 05:28:12]

https://codereview.chromium.org/2679523002/diff/60001/chrome/browser/webshare...
File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right):

https://codereview.chromium.org/2679523002/diff/60001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:63:
EXPECT_EQ(url_template, url_template_in_dict);
On 2017/02/07 05:19:36, Matt Giuca wrote:
> Get kNameKey and verify that it is... I guess EXPECT_FALSE on GetString?
(Check
> that it is not stored.)

I already expect that the dictionary has a size of 1. Is that sufficient? Since
I then lookup a key in the dict, and if that is successful, it must be the one
element in the dictionary.

[Matt Giuca, 2017-02-07 05:29:24]

lgtm

https://codereview.chromium.org/2679523002/diff/60001/chrome/browser/webshare...
File chrome/browser/webshare/share_target_pref_helper_unittest.cc (right):

https://codereview.chromium.org/2679523002/diff/60001/chrome/browser/webshare...
chrome/browser/webshare/share_target_pref_helper_unittest.cc:63:
EXPECT_EQ(url_template, url_template_in_dict);
On 2017/02/07 05:28:12, constantina wrote:
> On 2017/02/07 05:19:36, Matt Giuca wrote:
> > Get kNameKey and verify that it is... I guess EXPECT_FALSE on GetString?
> (Check
> > that it is not stored.)
> 
> I already expect that the dictionary has a size of 1. Is that sufficient?
Since
> I then lookup a key in the dict, and if that is successful, it must be the one
> element in the dictionary.

OK.

[benwells, 2017-02-07 06:51:06]

bookmark_app_helper lgtm

[constantina, 2017-02-07 07:22:50]

The CQ bit was checked by constantina@google.com

[commit-bot: I haz the power, 2017-02-07 07:23:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 08:04:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486452170608900,
"parent_rev": "9e1e3f73fc41ca1f8a5d42b3a418c286c2e39fac", "commit_rev":
"2883697c7814c4fca4675bd071161498b3b6aa27"}

[commit-bot: I haz the power, 2017-02-07 08:04:41]

Description was changed from

==========
Store target app name in Web Share prefs, and remove external logic.

UpdateShareTargetInPrefs now stores the name field of the supplied
target's manifest in the Web Share target prefs. For the given share
target, adds the name to the dictionary associated with the target.

UpdateShareTargetInPrefs also handles the logic about web apps with
manifests that don't have the share_target field, entirely internally.

BUG=668389
==========

to

==========
Store target app name in Web Share prefs, and remove external logic.

UpdateShareTargetInPrefs now stores the name field of the supplied
target's manifest in the Web Share target prefs. For the given share
target, adds the name to the dictionary associated with the target.

UpdateShareTargetInPrefs also handles the logic about web apps with
manifests that don't have the share_target field, entirely internally.

BUG=668389

Review-Url: https://codereview.chromium.org/2679523002
Cr-Commit-Position: refs/heads/master@{#448568}
Committed:
https://chromium.googlesource.com/chromium/src/+/2883697c7814c4fca4675bd07116...
==========

[commit-bot: I haz the power, 2017-02-07 08:04:42]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2883697c7814c4fca4675bd07116...