Add share_target field to Manifest.

Add share_target field to Manifest.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.

For use in: codereview.chromium.org/2639463002/
BUG=668389
Review-Url: https://codereview.chromium.org/2637003002
Cr-Commit-Position: refs/heads/master@{#445322}
Committed: https://chromium.googlesource.com/chromium/src/+/6e068f8014fdb411b63432c38aeb71fee20c7d18

[constantina, 2017-01-16 23:25:53]

Description was changed from

==========
Add share_url_template to Manifest, and store sites with this field.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.
Also adds a dictionary pref name, to store sites with manifests that
contain the share_url_template field; the key is the site origin, and
value is the value of share_url_template in the site's manifest.
Reads manifests after they have been parsed, for the share_url_template,
and stores the site and value under the pref name added.

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

to

==========
Add share_url_template to Manifest, and store sites with this field.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.

Also adds a dictionary pref name, to store sites with manifests that
contain the share_url_template field; the key is the site origin, and
value is the value of share_url_template in the site's manifest.

Reads manifests after they have been parsed, for the share_url_template,
and stores the site and value under the pref name added.

Follow-up CL will read the pref store for the pref name added, and read
share targets in chrome/browser/webshare/share_service_impl.cc.

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

[constantina, 2017-01-17 00:46:30]

Description was changed from

==========
Add share_url_template to Manifest, and store sites with this field.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.

Also adds a dictionary pref name, to store sites with manifests that
contain the share_url_template field; the key is the site origin, and
value is the value of share_url_template in the site's manifest.

Reads manifests after they have been parsed, for the share_url_template,
and stores the site and value under the pref name added.

Follow-up CL will read the pref store for the pref name added, and read
share targets in chrome/browser/webshare/share_service_impl.cc.

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

to

==========
Add share_url_template field to Manifest.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.

For use in: codereview.chromium.org/2639463002/
BUG=668389
==========

[constantina, 2017-01-18 06:18:26]

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

[constantina, 2017-01-18 06:18:26]

nasko@ for content/public, and security

mlamouri@ for content/renderer/manifest.

[Matt Giuca, 2017-01-18 07:51:32]

non-OWNER lgtm pending some nits.

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
File content/public/common/manifest.cc (right):

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
content/public/common/manifest.cc:27: Manifest::ShareTarget::ShareTarget() { }
Has this been git cl formatted? I think there's supposed to be no space in
between these.

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
content/public/common/manifest.h:61: // Structure representing how a Share
Target handles an incoming share.
nit: "Web Share target" (lowercase 't') to ensure this is associated with Web
Share.

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
content/public/common/manifest.h:68: // The url template that contains
placeholders to be replaced with shared
nit: URL.

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
content/renderer/manifest/manifest_parser_unittest.cc:1026: // Contains
share_target and url_template, and url_template is valid template.
What if it's an invalid template? Is it expected to parse correctly despite
being invalid, or not?

Either way, add a test for this case, and demonstrate that it behaves according
to your expectations.

[constantina, 2017-01-18 07:52:18]

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
content/renderer/manifest/manifest_parser.h:150: //
https://github.com/WICG/web-share-target/blob/master/docs/interface.md
Documentation doesn't reference dictionary structure for the field; pull request
to update it in progress.

[constantina, 2017-01-18 23:28:10]

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
File content/public/common/manifest.cc (right):

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
content/public/common/manifest.cc:27: Manifest::ShareTarget::ShareTarget() { }
On 2017/01/18 07:51:32, Matt Giuca wrote:
> Has this been git cl formatted? I think there's supposed to be no space in
> between these.

Done. There were inconsistencies, so I made all empty methods follow the first
formatting, which happened to have a space. Git cl formatted, and also corrected
the first.

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
content/public/common/manifest.h:61: // Structure representing how a Share
Target handles an incoming share.
On 2017/01/18 07:51:32, Matt Giuca wrote:
> nit: "Web Share target" (lowercase 't') to ensure this is associated with Web
> Share.

Done.

https://codereview.chromium.org/2637003002/diff/140001/content/public/common/...
content/public/common/manifest.h:68: // The url template that contains
placeholders to be replaced with shared
On 2017/01/18 07:51:32, Matt Giuca wrote:
> nit: URL.

Done.

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
content/renderer/manifest/manifest_parser_unittest.cc:1026: // Contains
share_target and url_template, and url_template is valid template.
On 2017/01/18 07:51:32, Matt Giuca wrote:
> What if it's an invalid template? Is it expected to parse correctly despite
> being invalid, or not?
> 
> Either way, add a test for this case, and demonstrate that it behaves
according
> to your expectations.

Test added. We will let it parse correctly, at least for now. Since we already
handle invalid templates later, it won't be a problem, and we avoid the extra
complexity during parsing. But we can consider changing it later.

[nasko, 2017-01-18 23:39:13]

content/ LGTM with a couple of nits.

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
content/renderer/manifest/manifest_parser.h:153: base::NullableString16
ParseShareTargetURLTemplate(
nit: Acronyms (such as URL) need to only upper case the first letter -
ParseShareTargetUrlTemplate.

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
content/renderer/manifest/manifest_parser.h:158: // Returns the parsed Share
Target. The returned Share Target is null if
nit: "target" as commented in another file with similar text.

[constantina, 2017-01-19 00:07:32]

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
content/renderer/manifest/manifest_parser.h:153: base::NullableString16
ParseShareTargetURLTemplate(
On 2017/01/18 23:39:13, nasko wrote:
> nit: Acronyms (such as URL) need to only upper case the first letter -
> ParseShareTargetUrlTemplate.

Capitalised every letter for consistency with the rest of the file.

https://codereview.chromium.org/2637003002/diff/140001/content/renderer/manif...
content/renderer/manifest/manifest_parser.h:158: // Returns the parsed Share
Target. The returned Share Target is null if
On 2017/01/18 23:39:13, nasko wrote:
> nit: "target" as commented in another file with similar text.

Done.

[constantina, 2017-01-20 05:02:47]

The CQ bit was checked by constantina@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 05:03:04]

Dry run: 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-01-20 05:55:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 05:55:56]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2017-01-23 00:18:06]

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.h:122: // Null if parsing failed or the field was
not present.
Document that this field is non-standard and part of a Chrome experiment
(https://github.com/WICG/web-share-target/blob/master/docs/interface.md).

[mlamouri (slow - plz ping), 2017-01-23 00:22:26]

Thank you for the change Connie! and sorry for the delay :(

lgtm with comments applied.

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
File content/public/common/manifest.cc (right):

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.cc:17: Manifest::Icon::Icon() {}
 = default;

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.cc:21: Manifest::Icon::~Icon() {}
ditto... and in many places below

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.h:122: // Null if parsing failed or the field was
not present.
Could you say that this should not be exposed to web contents at the moment
because it's not part of the spec and add a TODO on you and mgiuca?

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.h:123: ShareTarget share_target;
Would it make sense to use base::optional<> here? I think it would be clearer
for the consumer of the API that there is no share_target instead of using
`is_null`.

https://codereview.chromium.org/2637003002/diff/200001/content/renderer/manif...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/2637003002/diff/200001/content/renderer/manif...
content/renderer/manifest/manifest_parser_unittest.cc:1009:
ASSERT_TRUE(manifest.IsEmpty());
Why ASSERT?

https://codereview.chromium.org/2637003002/diff/200001/content/renderer/manif...
content/renderer/manifest/manifest_parser_unittest.cc:1020:
ASSERT_TRUE(manifest.IsEmpty());
ditto

https://codereview.chromium.org/2637003002/diff/200001/content/renderer/manif...
content/renderer/manifest/manifest_parser_unittest.cc:1047: }
Shouldn't you test `url_template` parsing?

[constantina, 2017-01-23 02:51:36]

> Thank you for the change Connie! and sorry for the delay :(

No worries, it's all good!

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
File content/public/common/manifest.cc (right):

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.cc:17: Manifest::Icon::Icon() {}
On 2017/01/23 00:22:26, mlamouri wrote:
>  = default;

Done.

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.cc:21: Manifest::Icon::~Icon() {}
On 2017/01/23 00:22:26, mlamouri wrote:
> ditto... and in many places below

Done.

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.h:122: // Null if parsing failed or the field was
not present.
On 2017/01/23 00:18:06, Matt Giuca wrote:
> Document that this field is non-standard and part of a Chrome experiment
> (https://github.com/WICG/web-share-target/blob/master/docs/interface.md).

Done.

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.h:122: // Null if parsing failed or the field was
not present.
On 2017/01/23 00:22:26, mlamouri wrote:
> Could you say that this should not be exposed to web contents at the moment
> because it's not part of the spec and add a TODO on you and mgiuca?

Done.

https://codereview.chromium.org/2637003002/diff/200001/content/public/common/...
content/public/common/manifest.h:123: ShareTarget share_target;
On 2017/01/23 00:22:26, mlamouri wrote:
> Would it make sense to use base::optional<> here? I think it would be clearer
> for the consumer of the API that there is no share_target instead of using
> `is_null`.

Yes, it would. Done.

https://codereview.chromium.org/2637003002/diff/200001/content/renderer/manif...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/2637003002/diff/200001/content/renderer/manif...
content/renderer/manifest/manifest_parser_unittest.cc:1009:
ASSERT_TRUE(manifest.IsEmpty());
On 2017/01/23 00:22:26, mlamouri wrote:
> Why ASSERT?

It shouldn't be ASSERT; changed to EXPECT.

https://codereview.chromium.org/2637003002/diff/200001/content/renderer/manif...
content/renderer/manifest/manifest_parser_unittest.cc:1020:
ASSERT_TRUE(manifest.IsEmpty());
On 2017/01/23 00:22:26, mlamouri wrote:
> ditto

Done.

https://codereview.chromium.org/2637003002/diff/200001/content/renderer/manif...
content/renderer/manifest/manifest_parser_unittest.cc:1047: }
On 2017/01/23 00:22:26, mlamouri wrote:
> Shouldn't you test `url_template` parsing?

Done. Separated out the existing test cases that were specific to url_template.

[constantina, 2017-01-23 03:13:17]

The CQ bit was checked by constantina@google.com

[constantina, 2017-01-23 03:13:17]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org,
nasko@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2637003002/#ps220001
(title: "Changed share_target type to base::Optional, and fixed tests, according
to feedback.")

[commit-bot: I haz the power, 2017-01-23 03:13:32]

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-01-23 04:51:34]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1485141197373130,
"parent_rev": "9aad73908ef510572cfe4504c36c2ece69438d1e", "commit_rev":
"6e068f8014fdb411b63432c38aeb71fee20c7d18"}

[commit-bot: I haz the power, 2017-01-23 04:52:49]

Description was changed from

==========
Add share_url_template field to Manifest.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.

For use in: codereview.chromium.org/2639463002/
BUG=668389
==========

to

==========
Add share_url_template field to Manifest.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.

For use in: codereview.chromium.org/2639463002/
BUG=668389

Review-Url: https://codereview.chromium.org/2637003002
Cr-Commit-Position: refs/heads/master@{#445322}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e068f8014fdb411b63432c38aeb...
==========

[commit-bot: I haz the power, 2017-01-23 04:52:50]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/6e068f8014fdb411b63432c38aeb...

[constantina, 2017-02-08 23:48:46]

Description was changed from

==========
Add share_url_template field to Manifest.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.

For use in: codereview.chromium.org/2639463002/
BUG=668389

Review-Url: https://codereview.chromium.org/2637003002
Cr-Commit-Position: refs/heads/master@{#445322}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e068f8014fdb411b63432c38aeb...
==========

to

==========
Add share_target field to Manifest.

Adds the share_url_template field to the Manifest object, for use with
the Web Share Target API.

For use in: codereview.chromium.org/2639463002/
BUG=668389

Review-Url: https://codereview.chromium.org/2637003002
Cr-Commit-Position: refs/heads/master@{#445322}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e068f8014fdb411b63432c38aeb...
==========