Bug 85364

What are the gaps for the EFL port?

For starters, the Web Intents code in WebCore does not compile with SFX (only works with V8) so this would have to be fixed. Then, it is pretty much the same work as for Chromium: - we need EFL wrappers for Intent, IntentRequest and IntentServiceInfo (ref: Bugs 73036 & 69870). - we need some code for dispatchIntent() in FrameLoaderClientEfl (ref: Bug 76014) Hopefully that's it.

Created attachment 142737 [details] Patch What's in the patch: - Add missing implementation for JSIntentConstructor::constructJSIntent() in JS bindings. - Fix compilation errors in EFL intents code (missing header includes) - Enable Web Intents by default on EFL port - Fix crash in ewk_intent_request_new() due to using a PassRefPtr after it becomes null. - Unskip webintents/web-intents-api.html Important note: -The following asserts fail in webintents/web-intents-obj-constructor.html even though the constructor is fully implemented: * shouldNotThrow("new WebKitIntent({'action':'a','type':'b','service':'url://a url'})"); * shouldNotThrow("new WebKitIntent({'action':'a','type':'b','service':'htttp://a url'})"); My code is throwing a SYNTAX_ERR because KURL(KURL(), 'url://a url').isValid() returns false. I'm guessing the Chromium port is using a different KURL implementation which behaves slightly differently for this case? Any advice on how to proceed?

Comment on attachment 142737 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142737&action=review > Source/WebCore/bindings/js/JSIntentConstructor.cpp:36 > +EncodedJSValue JSC_HOST_CALL JSIntentConstructor::constructJSIntent(ExecState* exec) I'm surprised we can't autogenerate more of this.

> My code is throwing a SYNTAX_ERR because KURL(KURL(), 'url://a url').isValid() returns false. I'm guessing the Chromium port is using a different KURL implementation which behaves slightly differently for this case? Any advice on how to proceed? We probably shouldn't be calling KURL(KURL(), 'url://a url'). Instead, we should be using ScriptExecutionContext::completeURL to take the string and produce a KURL. I wouldn't worry too much about those particular test cases. They're not that important (although getting exactly the right behavior would be ideal). Maybe a good path forward is to land this patch without enabling the feature and then work on that issue separately when enabling the feature?

> I'm surprised we can't autogenerate more of this. Yeah, I'm usually a stickler for making folks autogenerate their bindings. The V8 version is custom too. We should look at why it needs to be custom. I don't think that should block this patch, however, given that the V8 version is custom too.

Comment on attachment 142737 [details] Patch Clearing flags until I reupload another version which does not enable Web Intents by default on EFL.

Created attachment 142745 [details] Patch Take feedback into consideration: - Do not enable WEB_INTENTS by default on EFL port yet. Opened Bug 86873 to address the constructor issues with urls such as "url://a url".

(In reply to comment #6) > > I'm surprised we can't autogenerate more of this. > > Yeah, I'm usually a stickler for making folks autogenerate their bindings. The V8 version is custom too. We should look at why it needs to be custom. > > I don't think that should block this patch, however, given that the V8 version is custom too. Some of that discussion happened while you were away, Adam. We hope to move away from the custom code. The trade-off is that there's quite a bit of V8-specific machinery in handling overloaded constructors. There's no support for overloaded constructors in code-gen, and adding it is pretty tough. We want to move to only having the object literal constructor, so we think the code gen is temporary and can be dropped relatively soon.

Thanks for the explanation.

Comment on attachment 142745 [details] Patch This looks fine. As discussed above, I'd prefer if the bindings were autogenerated, but we can solve that problem for JSC at the same time we solve it for V8.

Comment on attachment 142745 [details] Patch Clearing flags on attachment: 142745 Committed r117624: <http://trac.webkit.org/changeset/117624>

All reviewed patches have been landed. Closing bug.