Bug 85364 – [EFL] Web Intents code is not compiling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

Web Intents are used by more and more APIs but it is currently only enabled in Chromium port. I think we should work on getting this supported in EFL port as well as it seems like an important feature.

[Greg Billock]

What are the gaps for the EFL port?

[Chris Dumez]

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.

[Chris Dumez]

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?

[Eric Seidel (no email)]

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.

[Adam Barth]

> 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?

[Adam Barth]

> 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.

[Chris Dumez]

Comment on attachment 142737 [details]
Patch

Clearing flags until I reupload another version which does not enable Web Intents by default on EFL.

[Chris Dumez]

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".

[Greg Billock]

(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.

[Adam Barth]

Thanks for the explanation.

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 142745 [details]
Patch

Clearing flags on attachment: 142745

Committed r117624: <http://trac.webkit.org/changeset/117624>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.