> This happens in chrome UI layer (not manifest parsing) so we're missing it for the case of webapks.

It seems to be working for WebAPKs. I just checked with Twitter Lite (which has a relative url_template) and it works. But maybe it's (erroneously) being resolved against the origin, rather than against the manifest URL?

In that case, the correct fix is to match ShareServiceImpl on desktop, and perform the resolution at launch time, rather than trying to resolve the URL at manifest parse time.

=== Explanation for why we can't resolve URLs at manifest parse time ===

Ultimately, it isn't really possible to resolve it at Manifest parse time, due to the fact that a url_template is not a URL (and therefore the concept of "parsing" it using the URL parser, or "resolving" it against another URL, is meaningless). (Specifically, it's an illegal URL because it contains "{}" characters, which is why we store it in a std::string, not a GURL, and why in the spec it's always referred to as a string, not a URL.)

Maybe GURL can store those characters, I don't know, but it would probably be unwise to rely on it since it's not legal in the URL syntax.

See the spec draft:
https://wicg.github.io/web-share-target/

Note that the "replace placeholders" algorithm takes a string (not a URL) and returns a string (not a URL), then that string is run through the URL Parser. The spec only considers it as a URL once placeholders are substituted at launch time. For now, I think the implementation should match the spec, so it should treat the url_template as an opaque string (not performing any operations on it) until launch time.

There are (silly) cases where resolving before substitution would actually result in the wrong answer, such as a template "{text}"; whether it's absolute or relative depends on whether text starts with a slash. We probably want to prohibit this type of template in the manifest, but for now, we don't.

I am grappling with this issue in the spec right now (since even if we don't resolve it at parse time, we'd still like to validate at parse time):

https://github.com/WICG/web-share-target/issues/25

In the spec, I worked around this by substituting an empty string at parse time (to validate), but then I leave it unresolved until launch time:
https://s3.amazonaws.com/pr-preview/mgiuca/web-share-target/pull/28.html#dfn-post-processing-the-share_target-member

Ultimately, we might want to define a "resolve the share_target URL against the manifest URL" algorithm in the spec, but for now, the implementation should match the spec.

It's working for webapks because we handled it at parse time, relative to manifest url: https://critique.corp.google.com/#review/176782907

Hmm, so the client and server are inconsistent in how they deal with share_target? (Client substitutes at launch time; server at parse time?)

What happens in the cases where this makes a difference? For example, this manifest:

  "share_target": {"url_template": "{text}"}

I think the server should be changed to match the spec (i.e., treat url_template as an opaque string, not a weird not-quite-a-valid-URL that gets resolved).

If we end up forcing the substitution parameters to be always in the query, it should be possible to make the template url absolute in manifest_parser.cc
More specifically:
- I think that splitting a URL in [query+fragment, everything_else] is simple (look for the first "?" or "#")
- It should be possible to resolve the scheme+authority+path against the manifest URL and append the unresolved query+fragment

For the sake of clarity:
- I am not suggesting making content::ShareTarget::url_template a GURL (mgiuca@ convinced me that this is suboptimal)
- I am suggesting that users of content::ShareTarget::url_template should assume that the URL is absolute

The reason that I am suggesting doing this (ugly) partial resolution is that in order to do the resolution on the client after filling in the template, I would need to add
WebappInfo#unresolvedUri()
WebappInfo#resolvedUri()

WebappInfo#create() is called prior to native being loaded (and we should continue to do so because it is needed for showing the splash screen)
WebappInfo#resolvedUri() must be called after native is loaded because we need native to resolve the URL.

android.net.Uri cannot resolve uris
java.net.URI can resolve uris but throws an exception when it tries to parse https://www.google.ca/%%330/ which gurl.cc accepts

What does everyone think?

One other concern is that if WebAPK validation fails, we need to resolve the URL in WebappLauncherActivity.java at which point native is not loaded

#5 OK, well as the spec currently stands, it is not really possible to resolve server-side. If we decide to change the spec to disallow templates in the path, we can do it as you say. But I would not like to block that in the code until we decide to change the spec.

If you like, you can resolve it on the client side and just file a bug that it isn't quite spec-compliant in the edge case where ".." or other tricky things appear in the string. And then we can resolve that as part of the spec discussion.

I will start a TAG review soon.

I am going to wait till the spec issues are resolved. Doing the resolution in Java is suboptimal because java.net.URI does not handle all of the URLs that GURL does

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20f515ccc63480fe265b1560b80b701d43fe006b

commit 20f515ccc63480fe265b1560b80b701d43fe006b
Author: Peter Kotwicz <pkotwicz@chromium.org>
Date: Thu Mar 08 01:15:02 2018

[WebShare] Only parse placeholders in the query and fragment part 2

This CL refactors ReplacePlaceholders() to make it clearer that only
placeholders in the url_template query and fragment are replaced.

BUG= 694380 ,  788191 

Change-Id: I62672ae9e8386ef53ff532cf20a0d312fcba600f
Reviewed-on: https://chromium-review.googlesource.com/925648
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541673}
[modify] https://crrev.com/20f515ccc63480fe265b1560b80b701d43fe006b/chrome/browser/webshare/share_service_impl.cc
[modify] https://crrev.com/20f515ccc63480fe265b1560b80b701d43fe006b/chrome/browser/webshare/share_service_impl.h
[modify] https://crrev.com/20f515ccc63480fe265b1560b80b701d43fe006b/chrome/browser/webshare/share_service_impl_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89d5601fd746b9db3f4847aae24aa09b65b48caf

commit 89d5601fd746b9db3f4847aae24aa09b65b48caf
Author: Peter Kotwicz <pkotwicz@chromium.org>
Date: Fri Mar 16 18:15:21 2018

[WebShare] Check share target url_template syntax at parse time

Invalid share targets will now be ignored, rather than showing up in the
picker and being rejected at launch time. This brings the implementation
in line with the spec: https://wicg.github.io/web-share-target/

This CL:
- Makes manifest_parser.cc check the share target url_template placeholder
  syntax.
- Refactors ReplacePlaceholders() so that it does no string copying when
 called with url_template_filled==nullptr

BUG= 694380 ,  788191 

Change-Id: Icd2a5af5e3100fd93a3fa3cd9ba00a23905355b7
Reviewed-on: https://chromium-review.googlesource.com/927581
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543763}
[modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/chrome/browser/webshare/share_service_impl.cc
[modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/common/manifest_share_target_util_unittest.cc
[modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/public/common/manifest_share_target_util.cc
[modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/public/common/manifest_share_target_util.h
[modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/renderer/manifest/manifest_parser.cc
[modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/renderer/manifest/manifest_parser_unittest.cc