Bug 85364 - [EFL] Web Intents code is not compiling
: [EFL] Web Intents code is not compiling
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Chris Dumez
:
Depends on: 85954 86354
Blocks: 75123 86864 86865 86866 86867 86868
  Show dependency treegraph
 
Reported: 2012-05-02 04:09 PDT by Chris Dumez
Modified: 2012-05-18 14:00 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-05-02 04:09:19 PDT
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 Greg Billock 2012-05-07 10:49:37 PDT
What are the gaps for the EFL port?
Comment 2 Chris Dumez 2012-05-07 11:05:03 PDT
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 Chris Dumez 2012-05-18 10:43:30 PDT
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 4 Eric Seidel 2012-05-18 10:51:42 PDT
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.
Comment 5 Adam Barth 2012-05-18 10:52:02 PDT
> 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 Adam Barth 2012-05-18 10:53:03 PDT
> 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 Chris Dumez 2012-05-18 11:01:44 PDT
Comment on attachment 142737 [details]
Patch

Clearing flags until I reupload another version which does not enable Web Intents by default on EFL.
Comment 8 Chris Dumez 2012-05-18 11:21:29 PDT
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".
Comment 9 Greg Billock 2012-05-18 11:26:06 PDT
(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 Adam Barth 2012-05-18 11:42:43 PDT
Thanks for the explanation.
Comment 11 Adam Barth 2012-05-18 11:45:41 PDT
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 12 WebKit Review Bot 2012-05-18 14:00:28 PDT
Comment on attachment 142745 [details]
Patch

Clearing flags on attachment: 142745

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