Bug 85364 - [EFL] Web Intents code is not compiling
: [EFL] Web Intents code is not compiling
Status: RESOLVED FIXED
: WebKit
WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 85954 86354
: 75123 86864 86865 86866 86867 86868
  Show dependency treegraph
 
Reported: 2012-05-02 04:09 PST by
Modified: 2012-05-18 14:00 PST (History)


Attachments
Patch (11.36 KB, patch)
2012-05-18 10:43 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.43 KB, patch)
2012-05-18 11:21 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-02 04:09:19 PST
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.
------- Comment #1 From 2012-05-07 10:49:37 PST -------
What are the gaps for the EFL port?
------- Comment #2 From 2012-05-07 11:05:03 PST -------
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.
------- Comment #3 From 2012-05-18 10:43:30 PST -------
Created an attachment (id=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 #4 From 2012-05-18 10:51:42 PST -------
(From update of attachment 142737 [details])
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.
------- Comment #5 From 2012-05-18 10:52:02 PST -------
> 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?
------- Comment #6 From 2012-05-18 10:53:03 PST -------
> 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 #7 From 2012-05-18 11:01:44 PST -------
(From update of attachment 142737 [details])
Clearing flags until I reupload another version which does not enable Web Intents by default on EFL.
------- Comment #8 From 2012-05-18 11:21:29 PST -------
Created an attachment (id=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".
------- Comment #9 From 2012-05-18 11:26:06 PST -------
(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.
------- Comment #10 From 2012-05-18 11:42:43 PST -------
Thanks for the explanation.
------- Comment #11 From 2012-05-18 11:45:41 PST -------
(From update of attachment 142745 [details])
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 #12 From 2012-05-18 14:00:28 PST -------
(From update of attachment 142745 [details])
Clearing flags on attachment: 142745

Committed r117624: <http://trac.webkit.org/changeset/117624>
------- Comment #13 From 2012-05-18 14:00:38 PST -------
All reviewed patches have been landed.  Closing bug.