Implement AssetLinkRetriever for Asset Links in Chrome.

Implement AssetLinkRetriever for Asset Links in Chrome.

The class makes a network request to a digital asset link. The spec is https://github.com/google/digitalassetlinks/blob/master/well-known/details.md

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

[vasilii, 2017-06-16 16:03:36]

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

[commit-bot: I haz the power, 2017-06-16 16:03:55]

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-16 16:11:38]

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

[vasilii, 2017-06-16 16:11:39]

Hi Vaclav,

please review.

[commit-bot: I haz the power, 2017-06-16 16:58:01]

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

[commit-bot: I haz the power, 2017-06-16 16:58:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2017-06-18 16:37:51]

Hi Vasilii,

This LGTM, with some comments and questions.

Cheers,
Vaclav

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

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc:68:
state_ = State::FINISHED;
Why not set the state independently of |error_|? Do we expect that after parsing
the resulting state might end up being different?

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc:70:
// Start parsing here.
nit: This should be a TODO(crbug.com/630555).

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

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.h:26:
class AssetLinkRetriever : public base::RefCounted<AssetLinkRetriever>,
Why does this need to be refcounted? Does net::URLFetcher::Create expect that?

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.h:35:
AssetLinkRetriever(GURL file_url);
Should this be explicit?

[vasilii, 2017-06-19 12:03:30]

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

[commit-bot: I haz the power, 2017-06-19 12:03:40]

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-19 12:20:20]

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

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc:68:
state_ = State::FINISHED;
On 2017/06/18 16:37:51, vabr (Chromium) wrote:
> Why not set the state independently of |error_|? Do we expect that after
parsing
> the resulting state might end up being different?

Because if there is no error then the next state should be PARSING.

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc:70:
// Start parsing here.
On 2017/06/18 16:37:51, vabr (Chromium) wrote:
> nit: This should be a TODO(crbug.com/630555).

Done.

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

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.h:26:
class AssetLinkRetriever : public base::RefCounted<AssetLinkRetriever>,
On 2017/06/18 16:37:51, vabr (Chromium) wrote:
> Why does this need to be refcounted? Does net::URLFetcher::Create expect that?

This is a part of a bigger plan. If two requests require the same file then this
object should be shared between them.
E.g. ru.aliexpress.com/.well-known/assetlinks.json includes
https://m.aliexpress.com/.well-known/assetlinks.json. Once we read it we need to
make sure that https://www.aliexpress.com also doesn't mind to give its
credentials to ru.aliexpress.com. But it includes the same file we just read.

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.h:35:
AssetLinkRetriever(GURL file_url);
On 2017/06/18 16:37:51, vabr (Chromium) wrote:
> Should this be explicit?

Done.

[commit-bot: I haz the power, 2017-06-19 13:07:08]

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

[commit-bot: I haz the power, 2017-06-19 13:07:10]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-06-19 13:07:55]

Still LGTM.

In general, it would be helpful to point out the plans (new PARSING state,
reason for refcounting, ...) somewhere, either in TODOs or ideally in a design
doc linked from the bug.

However, I understand if that's unnecessary overhead, because you are already
writing the follow-ups. In any case, don't get hit by a bus! :)

Vaclav

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

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.cc:68:
state_ = State::FINISHED;
On 2017/06/19 12:20:20, vasilii wrote:
> On 2017/06/18 16:37:51, vabr (Chromium) wrote:
> > Why not set the state independently of |error_|? Do we expect that after
> parsing
> > the resulting state might end up being different?
> 
> Because if there is no error then the next state should be PARSING.

Acknowledged.

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

https://codereview.chromium.org/2944533003/diff/1/components/password_manager...
components/password_manager/core/browser/site_affiliation/asset_link_retriever.h:26:
class AssetLinkRetriever : public base::RefCounted<AssetLinkRetriever>,
On 2017/06/19 12:20:20, vasilii wrote:
> On 2017/06/18 16:37:51, vabr (Chromium) wrote:
> > Why does this need to be refcounted? Does net::URLFetcher::Create expect
that?
> 
> This is a part of a bigger plan. If two requests require the same file then
this
> object should be shared between them.
> E.g. ru.aliexpress.com/.well-known/assetlinks.json includes
> https://m.aliexpress.com/.well-known/assetlinks.json. Once we read it we need
to
> make sure that https://www.aliexpress.com also doesn't mind to give its
> credentials to http://ru.aliexpress.com. But it includes the same file we just
read.

Acknowledged.

[vasilii, 2017-06-19 13:23:04]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2017-06-19 13:23:18]

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-06-19 13:28:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497878584037650,
"parent_rev": "0dcd8551496484cc8fdd1aa2fd42a119b6d4de1a", "commit_rev":
"2e6f9506e592058cb9ea2a7494a6f7dfa0f165a9"}

[commit-bot: I haz the power, 2017-06-19 13:28:11]

Description was changed from

==========
Implement AssetLinkRetriever for Asset Links in Chrome.

The class makes a network request to a digital asset link. The spec is
https://github.com/google/digitalassetlinks/blob/master/well-known/details.md

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

to

==========
Implement AssetLinkRetriever for Asset Links in Chrome.

The class makes a network request to a digital asset link. The spec is
https://github.com/google/digitalassetlinks/blob/master/well-known/details.md

BUG=630555

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

[commit-bot: I haz the power, 2017-06-19 13:28:12]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2e6f9506e592058cb9ea2a7494a6...