Bug 85954 – Web Intents code only supports V8

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

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 140921 [details]
Patch

[WebKit Review Bot]

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.

[Chris Dumez]

Created attachment 140924 [details]
Patch

Fix Changelog.

[Adam Barth]

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.

[Chris Dumez]

Created attachment 141156 [details]
Patch

Take Adam Barth's feedback into consideration.

[Build Bot]

Comment on attachment 141156 [details]
Patch

Attachment 141156 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12663309

[Adam Barth]

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.

[Chris Dumez]

Created attachment 141217 [details]
Patch

Add exception checks.

[Chris Dumez]

Created attachment 141218 [details]
stdout output from run-bindings-tests script

[Chris Dumez]

Created attachment 141219 [details]
Diff generated by run-bindings-tests script

[Adam Barth]

Comment on attachment 141217 [details]
Patch

Thanks!

[Build Bot]

Comment on attachment 141217 [details]
Patch

Attachment 141217 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12664421

[Chris Dumez]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Chris Dumez]

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.

[Adam Barth]

Is Dictionary.cpp in JSBindingsAllInOne.cpp ?

[Chris Dumez]

Created attachment 141235 [details]
Patch

Add Dictionary.cpp to JSBindingsAllInOne.cpp to hopefully fix linking on Windows (Thanks Adam).

[WebKit Review Bot]

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.

[Chris Dumez]

Created attachment 141239 [details]
Patch

Fix style issue.

[Build Bot]

Comment on attachment 141239 [details]
Patch

Attachment 141239 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12671090

[Chris Dumez]

Created attachment 141248 [details]
Patch

Add JSValue.cpp to JSBindingsAllInOne.cpp

[Adam Barth]

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?

[Build Bot]

Comment on attachment 141248 [details]
Patch

Attachment 141248 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12668139

[Chris Dumez]

Created attachment 141269 [details]
Patch

Try to add JSScriptValue.cpp to JSBindingsAllInOne.cpp instead.

[Chris Dumez]

Created attachment 141279 [details]
Patch

Add reviewer name to changelogs.

[Build Bot]

Comment on attachment 141279 [details]
Patch

Attachment 141279 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12668177

[Chris Dumez]

Created attachment 141340 [details]
Patch

Attempt to fix build on Windows.

[WebKit Review Bot]

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.

[Chris Dumez]

Created attachment 141341 [details]
Patch

[Build Bot]

Comment on attachment 141341 [details]
Patch

Attachment 141341 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12664600

[Chris Dumez]

Created attachment 141349 [details]
Patch

Another attempt to fix build on Windows.

[Chris Dumez]

Comment on attachment 141349 [details]
Patch

All green, finally. Adding cq? flag, could someone please commit?

[WebKit Review Bot]

Comment on attachment 141349 [details]
Patch

Clearing flags on attachment: 141349

Committed r116763: <http://trac.webkit.org/changeset/116763>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

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