86873 2012-05-18 11:11:35 -0700 Intent::create() throws is service is "url://a url" 2012-05-22 10:24:33 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 86868 1 cdumez cdumez abarth gbillock gyuyoung.kim lucas.de.marchi webkit.review.bot oldest_to_newest 627827 0 cdumez 2012-05-18 11:11:35 -0700 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. 629246 1 cdumez 2012-05-21 10:37:37 -0700 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? 629256 2 abarth 2012-05-21 10:48:33 -0700 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. 629261 3 cdumez 2012-05-21 10:55:58 -0700 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? 629268 4 143061 cdumez 2012-05-21 10:57:16 -0700 Created attachment 143061 Patch proposal 629269 5 143061 abarth 2012-05-21 10:58:42 -0700 Comment on attachment 143061 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. 629271 6 abarth 2012-05-21 11:00:14 -0700 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. 629276 7 gbillock 2012-05-21 11:04:50 -0700 (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.) 629290 8 abarth 2012-05-21 11:20:30 -0700 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. 630265 9 143280 cdumez 2012-05-22 05:08:14 -0700 Created attachment 143280 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() 630487 10 143280 abarth 2012-05-22 10:10:02 -0700 Comment on attachment 143280 Patch Thanks. 630514 11 143280 webkit.review.bot 2012-05-22 10:24:28 -0700 Comment on attachment 143280 Patch Clearing flags on attachment: 143280 Committed r117994: <http://trac.webkit.org/changeset/117994> 630515 12 webkit.review.bot 2012-05-22 10:24:33 -0700 All reviewed patches have been landed. Closing bug. 143061 2012-05-21 10:57:16 -0700 2012-05-22 05:08:14 -0700 Patch proposal 86873_encode_url.patch text/plain 1494 cdumez ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA1ODZkOTc1Li5iZWMxOTQyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTkg QEAKIDIwMTItMDUtMjEgIENocmlzdG9waGUgRHVtZXogIDxjaHJpc3RvcGhlLmR1bWV6QGludGVs LmNvbT4KIAorICAgICAgICBJbnRlbnQ6OmNyZWF0ZSgpIHRocm93cyBpcyBzZXJ2aWNlIGlzICJ1 cmw6Ly9hIHVybCIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTg2ODczCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgQ2FsbCBlbmNvZGVXaXRoVVJMRXNjYXBlU2VxdWVuY2VzKCkgb24gaW50ZW50IHNlcnZpY2Ug VVJMIHNvIHRoYXQKKyAgICAgICAgVVJMcyBzdWNoIGFzICJ1cmw6Ly9hIHVybCIgYXJlIHJlY29n bml6ZWQgYXMgdmFsaWQsIGFzIGV4cGVjdGVkCisgICAgICAgIGJ5IHdlYmludGVudHMvd2ViLWlu dGVudHMtb2JqLWNvbnN0cnVjdG9yLmh0bWwgdGVzdCBjYXNlLgorCisgICAgICAgICogTW9kdWxl cy9pbnRlbnRzL0ludGVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpJbnRlbnQ6OmNyZWF0ZSk6 CisKKzIwMTItMDUtMjEgIENocmlzdG9waGUgRHVtZXogIDxjaHJpc3RvcGhlLmR1bWV6QGludGVs LmNvbT4KKwogICAgICAgICBBZGQgc3VwcG9ydCBmb3IgTWVzc2FnZVBvcnRBcnJheSB0eXBlIHRv IEpTQwogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODQw OTMKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9pbnRlbnRzL0ludGVudC5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL2ludGVudHMvSW50ZW50LmNwcAppbmRleCAxNTcy OTdiLi5lM2EyMTBkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL2ludGVudHMv SW50ZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL2ludGVudHMvSW50ZW50LmNw cApAQCAtNzUsNyArNzUsNyBAQCBQYXNzUmVmUHRyPEludGVudD4gSW50ZW50OjpjcmVhdGUoU2Ny aXB0U3RhdGUqIHNjcmlwdFN0YXRlLCBjb25zdCBEaWN0aW9uYXJ5JiBvcAogICAgIFN0cmluZyBz ZXJ2aWNlOwogICAgIEtVUkwgc2VydmljZVVybDsKICAgICBpZiAob3B0aW9ucy5nZXQoInNlcnZp Y2UiLCBzZXJ2aWNlKSAmJiAhc2VydmljZS5pc0VtcHR5KCkpIHsKLSAgICAgIHNlcnZpY2VVcmwg PSBLVVJMKEtVUkwoKSwgc2VydmljZSk7CisgICAgICBzZXJ2aWNlVXJsID0gS1VSTChLVVJMKCks IGVuY29kZVdpdGhVUkxFc2NhcGVTZXF1ZW5jZXMoc2VydmljZSkpOwogICAgICAgaWYgKCFzZXJ2 aWNlVXJsLmlzVmFsaWQoKSkgewogICAgICAgICAgIGVjID0gU1lOVEFYX0VSUjsKICAgICAgICAg ICByZXR1cm4gMDsK 143280 2012-05-22 05:08:14 -0700 2012-05-22 10:24:28 -0700 Patch 86873_web_intents_test.patch text/plain 3107 cdumez ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBiZjI0NjQyLi40NGJhMGNjIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTItMDUt MjIgIENocmlzdG9waGUgRHVtZXogIDxjaHJpc3RvcGhlLmR1bWV6QGludGVsLmNvbT4KKworICAg ICAgICBJbnRlbnQ6OmNyZWF0ZSgpIHRocm93cyBpcyBzZXJ2aWNlIGlzICJ1cmw6Ly9hIHVybCIK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg2ODczCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUmVtb3ZlIGNo ZWNrcyBpbiB3ZWJpbnRlbnRzL3dlYi1pbnRlbnRzLW9iai1jb25zdHJ1Y3Rvci5odG1sIHdoaWNo IGFyZQorICAgICAgICB2YWxpZCBmb3IgR29vZ2xlIFVSTCBidXQgZmFpbCB3aXRoIEtVUkwuCisK KyAgICAgICAgKiB3ZWJpbnRlbnRzL3dlYi1pbnRlbnRzLW9iai1jb25zdHJ1Y3Rvci1leHBlY3Rl ZC50eHQ6CisgICAgICAgICogd2ViaW50ZW50cy93ZWItaW50ZW50cy1vYmotY29uc3RydWN0b3Iu aHRtbDoKKwogMjAxMi0wNS0yMiAgRG9taW5payBSw7Z0dHNjaGVzICA8ZG9taW5pay5yb3R0c2No ZXNAaW50ZWwuY29tPgogCiAgICAgICAgIFtFRkxdW0RSVF0gU3RydWN0dXJlIHRlc3RfZXhwZWN0 YXRpb25zLnR4dCBpbnRvIG1lYW5pbmdmdWwgc2VjdGlvbnMKZGlmZiAtLWdpdCBhL0xheW91dFRl c3RzL3dlYmludGVudHMvd2ViLWludGVudHMtb2JqLWNvbnN0cnVjdG9yLWV4cGVjdGVkLnR4dCBi L0xheW91dFRlc3RzL3dlYmludGVudHMvd2ViLWludGVudHMtb2JqLWNvbnN0cnVjdG9yLWV4cGVj dGVkLnR4dAppbmRleCBhZDkxNzY4Li4xNDRiOWJmIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy93 ZWJpbnRlbnRzL3dlYi1pbnRlbnRzLW9iai1jb25zdHJ1Y3Rvci1leHBlY3RlZC50eHQKKysrIGIv TGF5b3V0VGVzdHMvd2ViaW50ZW50cy93ZWItaW50ZW50cy1vYmotY29uc3RydWN0b3ItZXhwZWN0 ZWQudHh0CkBAIC0zMiw4ICszMiw2IEBAIFBBU1MgbmV3IFdlYktpdEludGVudCh7J2FjdGlvbic6 J2EnLCd0eXBlJzonYicsJ3NlcnZpY2UnOicnfSkgZGlkIG5vdCB0aHJvdyBleGNlCiBQQVNTIG5l dyBXZWJLaXRJbnRlbnQoeydhY3Rpb24nOidhJywndHlwZSc6J2InLCdzZXJ2aWNlJzpudWxsfSkg dGhyZXcgZXhjZXB0aW9uIEVycm9yOiBTWU5UQVhfRVJSOiBET00gRXhjZXB0aW9uIDEyLgogUEFT UyBuZXcgV2ViS2l0SW50ZW50KHsnYWN0aW9uJzonYScsJ3R5cGUnOidiJywnc2VydmljZSc6dW5k ZWZpbmVkfSkgdGhyZXcgZXhjZXB0aW9uIEVycm9yOiBTWU5UQVhfRVJSOiBET00gRXhjZXB0aW9u IDEyLgogUEFTUyBuZXcgV2ViS2l0SW50ZW50KHsnYWN0aW9uJzonYScsJ3R5cGUnOidiJywnc2Vy dmljZSc6J25vdCBhIHVybCd9KSB0aHJldyBleGNlcHRpb24gRXJyb3I6IFNZTlRBWF9FUlI6IERP TSBFeGNlcHRpb24gMTIuCi1QQVNTIG5ldyBXZWJLaXRJbnRlbnQoeydhY3Rpb24nOidhJywndHlw ZSc6J2InLCdzZXJ2aWNlJzondXJsOi8vYSB1cmwnfSkgZGlkIG5vdCB0aHJvdyBleGNlcHRpb24u Ci1QQVNTIG5ldyBXZWJLaXRJbnRlbnQoeydhY3Rpb24nOidhJywndHlwZSc6J2InLCdzZXJ2aWNl JzonaHR0dHA6Ly9hIHVybCd9KSBkaWQgbm90IHRocm93IGV4Y2VwdGlvbi4KIFBBU1MgbmV3IFdl YktpdEludGVudCh7J2FjdGlvbic6J2EnLCd0eXBlJzonYicsJ2RhdGEnOndpbmRvd30pIHRocmV3 IGV4Y2VwdGlvbiBFcnJvcjogREFUQV9DTE9ORV9FUlI6IERPTSBFeGNlcHRpb24gMjUuCiBQQVNT IG5ldyBXZWJLaXRJbnRlbnQoeydhY3Rpb24nOidhJywndHlwZSc6J2InLCdleHRyYXMnOm51bGx9 KSBkaWQgbm90IHRocm93IGV4Y2VwdGlvbi4KIFBBU1MgbmV3IFdlYktpdEludGVudCh7J2FjdGlv bic6J2EnLCd0eXBlJzonYicsJ2V4dHJhcyc6dW5kZWZpbmVkfSkgZGlkIG5vdCB0aHJvdyBleGNl cHRpb24uCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy93ZWJpbnRlbnRzL3dlYi1pbnRlbnRzLW9i ai1jb25zdHJ1Y3Rvci5odG1sIGIvTGF5b3V0VGVzdHMvd2ViaW50ZW50cy93ZWItaW50ZW50cy1v YmotY29uc3RydWN0b3IuaHRtbAppbmRleCBhMzU2ZTNkLi42MjUwZjgyIDEwMDY0NAotLS0gYS9M YXlvdXRUZXN0cy93ZWJpbnRlbnRzL3dlYi1pbnRlbnRzLW9iai1jb25zdHJ1Y3Rvci5odG1sCisr KyBiL0xheW91dFRlc3RzL3dlYmludGVudHMvd2ViLWludGVudHMtb2JqLWNvbnN0cnVjdG9yLmh0 bWwKQEAgLTQ0LDggKzQ0LDYgQEAKICAgICAgICAgc2hvdWxkVGhyb3coIm5ldyBXZWJLaXRJbnRl bnQoeydhY3Rpb24nOidhJywndHlwZSc6J2InLCdzZXJ2aWNlJzpudWxsfSkiLCAiJ0Vycm9yOiBT WU5UQVhfRVJSOiBET00gRXhjZXB0aW9uIDEyJyIpOwogICAgICAgICBzaG91bGRUaHJvdygibmV3 IFdlYktpdEludGVudCh7J2FjdGlvbic6J2EnLCd0eXBlJzonYicsJ3NlcnZpY2UnOnVuZGVmaW5l ZH0pIiwgIidFcnJvcjogU1lOVEFYX0VSUjogRE9NIEV4Y2VwdGlvbiAxMiciKTsKICAgICAgICAg c2hvdWxkVGhyb3coIm5ldyBXZWJLaXRJbnRlbnQoeydhY3Rpb24nOidhJywndHlwZSc6J2InLCdz ZXJ2aWNlJzonbm90IGEgdXJsJ30pIiwgIidFcnJvcjogU1lOVEFYX0VSUjogRE9NIEV4Y2VwdGlv biAxMiciKTsKLSAgICAgICAgc2hvdWxkTm90VGhyb3coIm5ldyBXZWJLaXRJbnRlbnQoeydhY3Rp b24nOidhJywndHlwZSc6J2InLCdzZXJ2aWNlJzondXJsOi8vYSB1cmwnfSkiKTsKLSAgICAgICAg c2hvdWxkTm90VGhyb3coIm5ldyBXZWJLaXRJbnRlbnQoeydhY3Rpb24nOidhJywndHlwZSc6J2In LCdzZXJ2aWNlJzonaHR0dHA6Ly9hIHVybCd9KSIpOwogCiAgICAgICAgIC8vIHxkYXRhfCBtdXN0 IGJlIGNsb25lYWJsZS4KICAgICAgICAgc2hvdWxkVGhyb3coIm5ldyBXZWJLaXRJbnRlbnQoeydh Y3Rpb24nOidhJywndHlwZSc6J2InLCdkYXRhJzp3aW5kb3d9KSIsICInRXJyb3I6IERBVEFfQ0xP TkVfRVJSOiBET00gRXhjZXB0aW9uIDI1JyIpOwo=