Bug 85954 - Web Intents code only supports V8
Summary: Web Intents code only supports V8
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 85364 86868
  Show dependency treegraph
 
Reported: 2012-05-08 22:27 PDT by Chris Dumez
Modified: 2012-05-18 10:18 PDT (History)
12 users (show)

See Also:


Attachments
Patch (10.45 KB, patch)
2012-05-09 05:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.48 KB, patch)
2012-05-09 05:21 PDT, Chris Dumez
abarth: review-
Details | Formatted Diff | Diff
Patch (14.95 KB, patch)
2012-05-10 06:08 PDT, Chris Dumez
abarth: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (15.05 KB, patch)
2012-05-10 11:54 PDT, Chris Dumez
abarth: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
stdout output from run-bindings-tests script (3.04 KB, text/plain)
2012-05-10 11:55 PDT, Chris Dumez
no flags Details
Diff generated by run-bindings-tests script (4.00 KB, patch)
2012-05-10 11:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.05 KB, patch)
2012-05-10 12:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.62 KB, patch)
2012-05-10 13:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.58 KB, patch)
2012-05-10 13:38 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (15.86 KB, patch)
2012-05-10 14:12 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (15.83 KB, patch)
2012-05-10 14:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.82 KB, patch)
2012-05-10 15:33 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (16.34 KB, patch)
2012-05-11 00:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (16.34 KB, patch)
2012-05-11 00:24 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (16.11 KB, patch)
2012-05-11 01:14 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-08 22:27:53 PDT
The current Web Intents code in WebCore only compiles with V8, not with JSC.
Support for JSC is needed so that Web Intents can be enabled for other ports such as EFL.

This will requires minor changes to the JSC bindings generator as well (similar to the changes made to the V8 bindings generator when adding support for intents).

I'll upload a patch for this soon.
Comment 1 Chris Dumez 2012-05-09 05:14:45 PDT
Created attachment 140921 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-09 05:17:20 PDT
Attachment 140921 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/CMakeLists.tx..." exit_code: 1
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Dumez 2012-05-09 05:21:10 PDT
Created attachment 140924 [details]
Patch

Fix Changelog.
Comment 4 Adam Barth 2012-05-09 09:03:19 PDT
Comment on attachment 140924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140924&action=review

> Source/WebCore/Modules/intents/Intent.cpp:41
> +static void getJSObjectPropertiesAsStringHashMap(ScriptState* scriptState, JSC::JSObject* object, WTF::HashMap<String, String>& map)

This isn't correct.  JSC-specific code should be in WebCore/bindings/js

> Source/WebCore/Modules/intents/Intent.cpp:135
> +#if USE(V8)
>      Dictionary extrasDictionary;
>      if (options.get("extras", extrasDictionary))
>          extrasDictionary.getOwnPropertiesAsStringHashMap(extras);
> +#else
> +    ScriptValue extraValue;
> +    if (options.get("extras", extraValue))
> +        if (extraValue.isObject())
> +            getJSObjectPropertiesAsStringHashMap(scriptState, extraValue.jsValue().toObject(scriptState), extras);
> +#endif

All these ifdefs are wrong.  The correct approach is to implement any missing script abstractions for JSC.
Comment 5 Chris Dumez 2012-05-10 06:08:27 PDT
Created attachment 141156 [details]
Patch

Take Adam Barth's feedback into consideration.
Comment 6 Build Bot 2012-05-10 06:36:26 PDT
Comment on attachment 141156 [details]
Patch

Attachment 141156 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12663309
Comment 7 Adam Barth 2012-05-10 10:47:32 PDT
Comment on attachment 141156 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141156&action=review

This patch is much better!  There are just a couple minor things below.

> Source/WebCore/bindings/js/Dictionary.cpp:70
> +            JSValue value = object->get(exec, *it);

What if this throws an exception?  It's possible that we want to ignore exceptions.  What does JSDictionary do with exceptions like these?

> Source/WebCore/bindings/js/Dictionary.cpp:72
> +                map.set(stringKey, String(value.toUString(exec).impl()));

What if this throws an exception?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2839
> +sub GetNativeTypeForCallbacks

Can you run run-bindings-tests --reset-results to update the references files we have checked in?  That will help us understand the effects of changing this file.
Comment 8 Chris Dumez 2012-05-10 11:54:45 PDT
Created attachment 141217 [details]
Patch

Add exception checks.
Comment 9 Chris Dumez 2012-05-10 11:55:32 PDT
Created attachment 141218 [details]
stdout output from run-bindings-tests script
Comment 10 Chris Dumez 2012-05-10 11:56:19 PDT
Created attachment 141219 [details]
Diff generated by run-bindings-tests script
Comment 11 Adam Barth 2012-05-10 12:11:58 PDT
Comment on attachment 141217 [details]
Patch

Thanks!
Comment 12 Build Bot 2012-05-10 12:36:00 PDT
Comment on attachment 141217 [details]
Patch

Attachment 141217 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12664421
Comment 13 Chris Dumez 2012-05-10 12:40:42 PDT
Any idea of what I need to do for ews-win? It apparently gets a linking error:

12>   Creating library C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\lib\WebKit.lib and object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\lib\WebKit.exp
12>WebCore.lib(JSBindingsAllInOne.obj) : error LNK2001: unresolved external symbol "public: __thiscall WebCore::Dictionary::Dictionary(class JSC::ExecState *,class JSC::JSValue)" (??0Dictionary@WebCore@@QAE@PAVExecState@JSC@@VJSValue@3@@Z)
12>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals

However, this does not make much sense to me. The Dictionary constructor in question was not modified. I merely added a new constructor.
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-05-10 12:48:47 PDT
Comment on attachment 141217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141217&action=review

> Source/cmake/WebKitFeatures.cmake:92
> +    WEBKIT_OPTION_DEFINE(ENABLE_WEB_INTENTS "Toggle Web Intents support", OFF)

Extra comma here.
Comment 15 Chris Dumez 2012-05-10 12:54:50 PDT
Created attachment 141232 [details]
Patch

Removed extra comma in CMake file. Thanks rakuco.
I still have no clue on our to make the ews-win happy though.
Comment 16 Adam Barth 2012-05-10 13:07:28 PDT
Is Dictionary.cpp in JSBindingsAllInOne.cpp ?
Comment 17 Chris Dumez 2012-05-10 13:18:13 PDT
Created attachment 141235 [details]
Patch

Add Dictionary.cpp to JSBindingsAllInOne.cpp to hopefully fix linking on Windows (Thanks Adam).
Comment 18 WebKit Review Bot 2012-05-10 13:21:47 PDT
Attachment 141235 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/CMakeLists.tx..." exit_code: 1
Source/WebCore/bindings/js/JSBindingsAllInOne.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Chris Dumez 2012-05-10 13:38:29 PDT
Created attachment 141239 [details]
Patch

Fix style issue.
Comment 20 Build Bot 2012-05-10 14:07:52 PDT
Comment on attachment 141239 [details]
Patch

Attachment 141239 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12671090
Comment 21 Chris Dumez 2012-05-10 14:12:16 PDT
Created attachment 141248 [details]
Patch

Add JSValue.cpp to JSBindingsAllInOne.cpp
Comment 22 Adam Barth 2012-05-10 14:34:04 PDT
Comment on attachment 141248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141248&action=review

> Source/WebCore/bindings/js/JSBindingsAllInOne.cpp:156
> +#include "JSValue.cpp"

This doesn't seem right.  JSValue is in the JavaScriptCore project, right?
Comment 23 Build Bot 2012-05-10 14:44:41 PDT
Comment on attachment 141248 [details]
Patch

Attachment 141248 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12668139
Comment 24 Chris Dumez 2012-05-10 14:49:15 PDT
Created attachment 141269 [details]
Patch

Try to add JSScriptValue.cpp to JSBindingsAllInOne.cpp instead.
Comment 25 Chris Dumez 2012-05-10 15:33:23 PDT
Created attachment 141279 [details]
Patch

Add reviewer name to changelogs.
Comment 26 Build Bot 2012-05-10 15:53:23 PDT
Comment on attachment 141279 [details]
Patch

Attachment 141279 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12668177
Comment 27 Chris Dumez 2012-05-11 00:20:15 PDT
Created attachment 141340 [details]
Patch

Attempt to fix build on Windows.
Comment 28 WebKit Review Bot 2012-05-11 00:23:09 PDT
Attachment 141340 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/API/JS..." exit_code: 1
Source/JavaScriptCore/API/JSValueRef.h:66:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Chris Dumez 2012-05-11 00:24:51 PDT
Created attachment 141341 [details]
Patch
Comment 30 Build Bot 2012-05-11 01:01:13 PDT
Comment on attachment 141341 [details]
Patch

Attachment 141341 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12664600
Comment 31 Chris Dumez 2012-05-11 01:14:28 PDT
Created attachment 141349 [details]
Patch

Another attempt to fix build on Windows.
Comment 32 Chris Dumez 2012-05-11 06:31:37 PDT
Comment on attachment 141349 [details]
Patch

All green, finally. Adding cq? flag, could someone please commit?
Comment 33 WebKit Review Bot 2012-05-11 06:48:32 PDT
Comment on attachment 141349 [details]
Patch

Clearing flags on attachment: 141349

Committed r116763: <http://trac.webkit.org/changeset/116763>
Comment 34 WebKit Review Bot 2012-05-11 06:48:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Adam Barth 2012-05-11 09:46:56 PDT
Comment on attachment 141349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141349&action=review

> Source/JavaScriptCore/API/JSValueRef.h:66
> -JS_EXPORT JSType JSValueGetType(JSContextRef ctx, JSValueRef value);
> +JS_EXPORT JSType JSValueGetType(JSContextRef ctx, JSValueRef);

This change probably wasn't necessary.  Generally we try to be careful when changing API headers.  (Although this change does look harmless.)