Bug 78210

Created attachment 127292 [details] Patch.

Comment on attachment 127292 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=127292&action=review > Source/WebCore/ChangeLog:7 > + Please describe what this patch is doing. > Source/WebCore/bindings/js/JSDOMBinding.h:360 > + result.append(static_cast<T>(indexedValue.toNumber(exec))); I am afraid that this conversion rule is wrong with the Web IDL spec: http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long It might be difficult to make such a template<class T> for all types due to complicated conversion rules in the spec. For now, maybe we can write jsUnsignedLongArrayToVector() for unsigned long[] only. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:281 > + } elsif ($type eq "unsigned long[]") { We want to avoid such hard-coding, but hard-coding might be a modest solution in this case. OK. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2899 > + return "jsNumberArrayToVector<unsigned long>(exec, $value)"; I cannot find any "jsNumberArrayToVector()" in your run-bindings-tests results... > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:45 > +#include "JSunsigned long[].h" This is wrong. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:55 > +#include "unsigned long[].h" This is wrong. > Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:72 > + putDirect(exec->globalData(), exec->propertyNames().length, jsNumber(2), ReadOnly | DontDelete | DontEnum); This is not the change caused by your patch. Maybe you need to rebaseline run-bindings-tests results before making your patch, if the results are not correct on trunk. (Sometimes people forget to update run-bindings-tests results.)

Comment on attachment 127292 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=127292&action=review Thank you for your comments. >> Source/WebCore/ChangeLog:7 >> + > > Please describe what this patch is doing. OK. >> Source/WebCore/bindings/js/JSDOMBinding.h:360 >> + result.append(static_cast<T>(indexedValue.toNumber(exec))55); > > I am afraid that this conversion rule is wrong with the Web IDL spec: http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long > > It might be difficult to make such a template<class T> for all types due to complicated conversion rules in the spec. For now, maybe we can write jsUnsignedLongArrayToVector() for unsigned long[] only. Sure - I will change that way. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:281 >> + } elsif ($type eq "unsigned long[]") { > > We want to avoid such hard-coding, but hard-coding might be a modest solution in this case. OK. I think so. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2899 >> + return "jsNumberArrayToVector<unsigned long>(exec, $value)"; > > I cannot find any "jsNumberArrayToVector()" in your run-bindings-tests results... I got it. It will be added. >> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:45 >> +#include "JSunsigned long[].h" > > This is wrong. Yes. It will be fixed. >> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:55 >> +#include "unsigned long[].h" > > This is wrong. Yes. It will be fixed. >> Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:72 >> + putDirect(exec->globalData(), exec->propertyNames().length, jsNumber(2), ReadOnly | DontDelete | DontEnum); > > This is not the change caused by your patch. Maybe you need to rebaseline run-bindings-tests results before making your patch, if the results are not correct on trunk. (Sometimes people forget to update run-bindings-tests results.) You are right. I will remove them.

Created attachment 127325 [details] Fixed patch.

Comment on attachment 127325 [details] Fixed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=127325&action=review > Source/WebCore/bindings/js/JSDOMBinding.h:350 > + if (!object) "if (exec->hadException())" might be better. If "!object" is true, then "exec->hadException()" is true. > Source/WebCore/bindings/js/JSDOMBinding.h:354 > + JSC::JSValue indexedValue; Nit: You can move this declaration into the loop. > Source/WebCore/bindings/js/JSDOMBinding.h:357 > + if (indexedValue == JSC::JSValue(JSC::JSValue::JSUndefined) || !indexedValue.isNumber()) Maybe "if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isUInt32())"? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2899 > + return "jsUnsignedLongArrayToVector<unsigned long>(exec, $value)"; This should be "jsUnsignedLongArrayToVector(exec, $value)", since the function is not a template. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2144 > + Vector<unsigned long> unsignedLongArray(jsUnsignedLongArrayToVector<unsigned long>(exec, MAYBE_MISSING_PARAMETER(exec, 0, DefaultIsUndefined))); Ditto.

Comment on attachment 127325 [details] Fixed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=127325&action=review >> Source/WebCore/bindings/js/JSDOMBinding.h:350 >> + if (!object) > > "if (exec->hadException())" might be better. If "!object" is true, then "exec->hadException()" is true. OK. You are right. >> Source/WebCore/bindings/js/JSDOMBinding.h:354 >> + JSC::JSValue indexedValue; > > Nit: You can move this declaration into the loop. OK. >> Source/WebCore/bindings/js/JSDOMBinding.h:357 >> + if (indexedValue == JSC::JSValue(JSC::JSValue::JSUndefined) || !indexedValue.isNumber()) > > Maybe "if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isUInt32())"? I will change this as follows: "if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isNumber())". It returns false when indexedValue goes beyond 2147483647. Please refer to the comments from Gavin Barraclough (https://bugs.webkit.org/show_bug.cgi?id=77317) >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2899 >> + return "jsUnsignedLongArrayToVector<unsigned long>(exec, $value)"; > > This should be "jsUnsignedLongArrayToVector(exec, $value)", since the function is not a template. Yes. It will be fixed. >> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2144 >> + Vector<unsigned long> unsignedLongArray(jsUnsignedLongArrayToVector<unsigned long>(exec, MAYBE_MISSING_PARAMETER(exec, 0, DefaultIsUndefined))); > > Ditto. Ditto.

Created attachment 127352 [details] Fixed patch.

Comment on attachment 127352 [details] Fixed patch. OK!

Comment on attachment 127352 [details] Fixed patch. Rejecting attachment 127352 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11535167

Comment on attachment 127352 [details] Fixed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=127352&action=review > Source/WebCore/ChangeLog:6 > + reviewed by nobody (oops!). Maybe you manually edited this line. It should be exactly "Reviewed by NOBODY (OOPS!)". Anyway, would you please rebase the patch (just in case), and upload it with correct ChangeLogs with "--no-review --no-obsolete" option?

Comment on attachment 127352 [details] Fixed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=127352&action=review >> Source/WebCore/ChangeLog:6 >> + reviewed by nobody (oops!). > > Maybe you manually edited this line. It should be exactly "Reviewed by NOBODY (OOPS!)". > > Anyway, would you please rebase the patch (just in case), and upload it with correct ChangeLogs with "--no-review --no-obsolete" option? OK. Thank you for your review. Mr. Kentaro Hara.:)

Created attachment 127355 [details] Fixed patch for WebCore/ChangeLog

The commit-queue encountered the following flaky tests while processing attachment 127355 [details]: perf/object-keys.html bug 63769 (author: ojan@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 127355 [details] Fixed patch for WebCore/ChangeLog Clearing flags on attachment: 127355 Committed r107926: <http://trac.webkit.org/changeset/107926>

All reviewed patches have been landed. Closing bug.