Summary: | Web Intents code only supports V8 | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, d-r, gbillock, gustavo, gyuyoung.kim, haraken, japhet, lucas.de.marchi, philn, rakuco, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||
Bug Blocks: | 85364, 86868 | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2012-05-08 22:27:53 PDT
Created attachment 140921 [details]
Patch
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.
Created attachment 140924 [details]
Patch
Fix Changelog.
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. Created attachment 141156 [details]
Patch
Take Adam Barth's feedback into consideration.
Comment on attachment 141156 [details] Patch Attachment 141156 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12663309 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. Created attachment 141217 [details]
Patch
Add exception checks.
Created attachment 141218 [details]
stdout output from run-bindings-tests script
Created attachment 141219 [details]
Diff generated by run-bindings-tests script
Comment on attachment 141217 [details]
Patch
Thanks!
Comment on attachment 141217 [details] Patch Attachment 141217 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12664421 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 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. 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.
Is Dictionary.cpp in JSBindingsAllInOne.cpp ? Created attachment 141235 [details]
Patch
Add Dictionary.cpp to JSBindingsAllInOne.cpp to hopefully fix linking on Windows (Thanks Adam).
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.
Created attachment 141239 [details]
Patch
Fix style issue.
Comment on attachment 141239 [details] Patch Attachment 141239 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12671090 Created attachment 141248 [details]
Patch
Add JSValue.cpp to JSBindingsAllInOne.cpp
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 on attachment 141248 [details] Patch Attachment 141248 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12668139 Created attachment 141269 [details]
Patch
Try to add JSScriptValue.cpp to JSBindingsAllInOne.cpp instead.
Created attachment 141279 [details]
Patch
Add reviewer name to changelogs.
Comment on attachment 141279 [details] Patch Attachment 141279 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12668177 Created attachment 141340 [details]
Patch
Attempt to fix build on Windows.
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.
Created attachment 141341 [details]
Patch
Comment on attachment 141341 [details] Patch Attachment 141341 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12664600 Created attachment 141349 [details]
Patch
Another attempt to fix build on Windows.
Comment on attachment 141349 [details]
Patch
All green, finally. Adding cq? flag, could someone please commit?
Comment on attachment 141349 [details] Patch Clearing flags on attachment: 141349 Committed r116763: <http://trac.webkit.org/changeset/116763> All reviewed patches have been landed. Closing bug. 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.) |