Intent::create() throws is service is "url://a url"

[Chris Dumez]

webintents/web-intents-obj-constructor.html is not passing on EFL port because of the following checks:
  shouldNotThrow("new WebKitIntent({'action':'a','type':'b','service':'url://a url'})");
  shouldNotThrow("new WebKitIntent({'action':'a','type':'b','service':'htttp://a url'})");

The reason for that is that KURL(KURL(), "url://a url").isValid() returns false, which causes the implementation to throw an unexpected SYNTAX_ERR.

In Intent.cpp, we should probably use ScriptExecutionContext::completeURL to take the string and produce a KURL.

[Chris Dumez]

Apparently, the URLs are recognized as invalid because of the white space in them. The following assertions are passing:
  shouldNotThrow("new WebKitIntent({'action':'a','type':'b','service':'url://a%20url'})");
  shouldNotThrow("new WebKitIntent({'action':'a','type':'b','service':'htttp://a%20url'})");

I'm not sure how to proceed at this point. Can the service URL really contain the white space?

[Adam Barth]

These tests are really just testing differences in URL parsing behavior.  We already have tests for these issues in LayoutTests/fast/url.  We probably don't need to test them in these intent tests as well.

We should test the "invalid URL case," but we should do that with a URL that's invalid both for KURL and GURL.

[Chris Dumez]

I'm not sure I understand, the 2 checks in questions seem to be for valid URL case since they don't expect the constructor to throw.

If I understand correctly, the issue is that 'url://a url' is valid for GURL but not for KURL. A (tested) solution for this would be to do:

     KURL serviceUrl;
     if (options.get("service", service) && !service.isEmpty()) {
-      serviceUrl = KURL(KURL(), service);
+      serviceUrl = KURL(KURL(), encodeWithURLEscapeSequences(service));
       if (!serviceUrl.isValid()) {
           ec = SYNTAX_ERR;
           return 0;

Calling encodeWithURLEscapeSequences() on the string before passing it to KURL constructor will make it behave as expected by the test. Is this an appropriate solution or should we somehow change the test case instead?

[Chris Dumez]

Created attachment 143061 [details]
Patch proposal

[Adam Barth]

Comment on attachment 143061 [details]
Patch proposal

This patch is not correct.  I'd recommend just removing these test cases.  They're testing URL parsing behavior, which we already test in LayoutTests/fast/url.

[Adam Barth]

To give an example of why this patch is wrong, consider the following URL:

http://www.example.com/t%41co

After the patch, the % will be encoded rather than being an escape character.

[Greg Billock]

(In reply to comment #3)
> I'm not sure I understand, the 2 checks in questions seem to be for valid URL case since they don't expect the constructor to throw.
> 
> If I understand correctly, the issue is that 'url://a url' is valid for GURL but not for KURL. A (tested) solution for this would be to do:
> 
>      KURL serviceUrl;
>      if (options.get("service", service) && !service.isEmpty()) {
> -      serviceUrl = KURL(KURL(), service);
> +      serviceUrl = KURL(KURL(), encodeWithURLEscapeSequences(service));
>        if (!serviceUrl.isValid()) {
>            ec = SYNTAX_ERR;
>            return 0;
> 
> Calling encodeWithURLEscapeSequences() on the string before passing it to KURL constructor will make it behave as expected by the test. Is this an appropriate solution or should we somehow change the test case instead?

I think within WebCore, we should limit the error case to where KURL produces an invalid parse. Being stricter than that is liable to have side effects. The reason for using this constructor instead of completeURL is that the spec wants these urls to be absolute.

(Whether that's a good restriction, or whether allowing relative URLs and then using completeURL() makes more sense, is a good question, but we should discuss that on public-web-intents.)

[Adam Barth]

For reference, it's extremely rare to not resolve strings relative to some base URL.  I'm sure we do it somewhere, but I can't think of any cases off hand.

[Chris Dumez]

Created attachment 143280 [details]
Patch

Removing the 2 checks in question, as suggested.

Note that the test cannot will unskipped on EFL port yet because of the following remaining issues:
  * Bug 87118 - [JSC] SerializedScriptValue.create() succeeds even if port cannot be added to transfer
  * Bug 86864 - [EFL] EFL's DRT needs to print information about received Web Intents
  * Bug 86841 - [EFL] EFL's Ewk_Intent does not expose WebCore::Intent::messagePorts()

[Adam Barth]

Comment on attachment 143280 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 143280 [details]
Patch

Clearing flags on attachment: 143280

Committed r117994: <http://trac.webkit.org/changeset/117994>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.