Bug 85954

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