Introduce a parse for digital asset links.

Introduce a parse for digital asset links.

The spec is https://github.com/google/digitalassetlinks/blob/master/well-known/details.md

BUG=630555
Review-Url: https://codereview.chromium.org/2967503003
Cr-Commit-Position: refs/heads/master@{#484025}
Committed: https://chromium.googlesource.com/chromium/src/+/2befc6f4110c0146686f35638e6e890cc1486d7e

[vasilii, 2017-06-30 17:28:55]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 17:29:16]

Dry run: CQ is trying da patch.

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

[vasilii, 2017-06-30 17:31:21]

vasilii@chromium.org changed reviewers:
+ vabr@chromium.org

[vasilii, 2017-06-30 17:31:22]

Hi Vaclav,

please review

[commit-bot: I haz the power, 2017-06-30 18:21:50]

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

[commit-bot: I haz the power, 2017-06-30 18:21:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vabr (Chromium), 2017-07-01 12:21:42]

LGTM, nice!
Cheers,
Vaclav

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
File
components/password_manager/core/browser/site_affiliation/asset_link_data.cc
(right):

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_data.cc:25:
static void RegisterJSONConverter(
I was about to complain that this method seems to do more than the style guide
allows for structs [1]. I wanted to suggest that since it is static and all in
the struct is public, we could move it outside of the struct.

But then I saw this is how even base code is written, so I take my complaint
back.

[1] https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
File components/password_manager/core/browser/site_affiliation/asset_link_data.h
(right):

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_data.h:26:
AssetLinkData(AssetLinkData&& other) noexcept;
For my education, what is the effect of noexcept here?

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
File
components/password_manager/core/browser/site_affiliation/asset_link_data_unittest.cc
(right):

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_data_unittest.cc:149:
EXPECT_THAT(data.targets(), ElementsAre(GURL("https://www.google.com"),
Do the elements need to end up in that order?

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
File
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc
(right):

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc:79:
std::unique_ptr<AssetLinkData> data = base::MakeUnique<AssetLinkData>();
nit: Use auto to avoid duplicating the type name.
auto data = base::MakeUnique<AssetLinkData>()

[vasilii, 2017-07-03 09:31:00]

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
File
components/password_manager/core/browser/site_affiliation/asset_link_data.cc
(right):

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_data.cc:25:
static void RegisterJSONConverter(
On 2017/07/01 12:21:42, vabr (Chromium) wrote:
> I was about to complain that this method seems to do more than the style guide
> allows for structs [1]. I wanted to suggest that since it is static and all in
> the struct is public, we could move it outside of the struct.
> 
> But then I saw this is how even base code is written, so I take my complaint
> back.
> 
> [1] https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

Nothing would work because base::JSONValueConverter needs exactly that format. I
copied it from the examples for the class.

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
File components/password_manager/core/browser/site_affiliation/asset_link_data.h
(right):

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_data.h:26:
AssetLinkData(AssetLinkData&& other) noexcept;
On 2017/07/01 12:21:42, vabr (Chromium) wrote:
> For my education, what is the effect of noexcept here?

Can be faster in containers and algorithms. See
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2009/n2855.html#problem
Imagine you need to rellocate a vector. If the copy constructor may throw then
we can't use it, we should use the copy constructor. On the other hand, if the
move constructor never throws then it will be faster to use it.

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
File
components/password_manager/core/browser/site_affiliation/asset_link_data_unittest.cc
(right):

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_data_unittest.cc:149:
EXPECT_THAT(data.targets(), ElementsAre(GURL("https://www.google.com"),
On 2017/07/01 12:21:42, vabr (Chromium) wrote:
> Do the elements need to end up in that order?

Good point.

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
File
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc
(right):

https://codereview.chromium.org/2967503003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc:79:
std::unique_ptr<AssetLinkData> data = base::MakeUnique<AssetLinkData>();
On 2017/07/01 12:21:42, vabr (Chromium) wrote:
> nit: Use auto to avoid duplicating the type name.
> auto data = base::MakeUnique<AssetLinkData>()

Done.

[vasilii, 2017-07-03 09:31:27]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2017-07-03 09:31:27]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2967503003/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2017-07-03 09:31:36]

CQ is trying da patch.

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

[vabr (Chromium), 2017-07-03 09:33:00]

Thanks for the answers and for explaining the noexcept. (Still LGTM of course.)

[commit-bot: I haz the power, 2017-07-03 10:13:21]

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

[commit-bot: I haz the power, 2017-07-03 10:13:22]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vasilii, 2017-07-03 16:52:57]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2017-07-03 16:52:58]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2967503003/#ps40001
(title: "Fix compilation")

[commit-bot: I haz the power, 2017-07-03 16:53:11]

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-07-03 18:16:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499100777248530,
"parent_rev": "838e38955e7ae018e1bffa5fc1af39ad545cbbe9", "commit_rev":
"2befc6f4110c0146686f35638e6e890cc1486d7e"}

[commit-bot: I haz the power, 2017-07-03 18:17:01]

Description was changed from

==========
Introduce a parse for digital asset links.

The spec is
https://github.com/google/digitalassetlinks/blob/master/well-known/details.md

BUG=630555
==========

to

==========
Introduce a parse for digital asset links.

The spec is
https://github.com/google/digitalassetlinks/blob/master/well-known/details.md

BUG=630555

Review-Url: https://codereview.chromium.org/2967503003
Cr-Commit-Position: refs/heads/master@{#484025}
Committed:
https://chromium.googlesource.com/chromium/src/+/2befc6f4110c0146686f35638e6e...
==========

[commit-bot: I haz the power, 2017-07-03 18:17:02]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2befc6f4110c0146686f35638e6e...

[battre, 2017-07-03 20:29:13]

battre@chromium.org changed reviewers:
+ battre@chromium.org

[battre, 2017-07-03 20:29:14]

Are you sure it is ok to do the parsing of untrusted JSON in the browser process
instead of a sandboxed worker process?

[vasilii, 2017-07-04 11:55:46]

On 2017/07/03 20:29:14, battre wrote:
> Are you sure it is ok to do the parsing of untrusted JSON in the browser
process
> instead of a sandboxed worker process?

We do it for Cloud Print for example
(https://cs.chromium.org/chromium/src/chrome/browser/printing/cloud_print/priv...)
Or we parse OpenSearch descriptions which are just XMLs referenced by random
sites
(https://cs.chromium.org/chromium/src/components/search_engines/template_url_f...)
I can check with the security team of course if you are not convinced.

[battre, 2017-07-04 15:14:44]

On 2017/07/04 11:55:46, vasilii wrote:
> On 2017/07/03 20:29:14, battre wrote:
> > Are you sure it is ok to do the parsing of untrusted JSON in the browser
> process
> > instead of a sandboxed worker process?
> 
> We do it for Cloud Print for example
>
(https://cs.chromium.org/chromium/src/chrome/browser/printing/cloud_print/priv...)
> Or we parse OpenSearch descriptions which are just XMLs referenced by random
> sites
>
(https://cs.chromium.org/chromium/src/components/search_engines/template_url_f...)
> I can check with the security team of course if you are not convinced.

I think it would be great to quickly ping them and ask them whether in the light
of the precedents you found, this is expected.

Thanks,
Dominic