c.f. corresponding V8 bug: https://bugs.webkit.org/show_bug.cgi?id=109899 Currently: - MAYBE_MISSING_PARAMETER(exec, index, DefaultIsUndefined) returns exec->argument(index). - MAYBE_MISSING_PARAMETER(exec, index, DefaultIsNullString) returns JSValue() if exec->argument(index) is missing (i.e. the length of the argument is less than index). It returns exec->argument(index) otherwise. No one other than CodeGeneratorJS.pm uses MAYBE_MISSING_PARAMETER(exec, index, DefaultIsUndefined). Instead, we simply use exec->argument(index). We should remove the redundant usage in CodeGeneratorJS.pm too. The long-name macro has been making generated code less readable. In addition, we can rename MAYBE_MISSING_PARAMETER() to MissingIsNullString().
Created attachment 188494 [details] Patch
Comment on attachment 188494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188494&action=review > Source/WebCore/bindings/js/JSDOMBinding.h:62 > +#define MissingIsNullString(exec, index) (((index) >= (exec)->argumentCount()) ? (JSValue()) : ((exec)->argument(index))) Same comment. This should probably be an inline function. If it can't be an inline function, we should keep the ALL_CAPS name.
Comment on attachment 188494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188494&action=review >> Source/WebCore/bindings/js/JSDOMBinding.h:62 >> +#define MissingIsNullString(exec, index) (((index) >= (exec)->argumentCount()) ? (JSValue()) : ((exec)->argument(index))) > > Same comment. This should probably be an inline function. If it can't be an inline function, we should keep the ALL_CAPS name. Further, both the names MAYBE_MISSING_PARAMETER and MissingIsNullString are examples of the kind of confusing names we try to avoid. A function should be named after its return value. And even though it’s used to implement DefaultIsNullString, this does not return a string, so it’s really confusing to mention string in its name. ARGUMENT_OR_NULL (or ArgumentOrNull if it can be an inline function) would be one possible name. Not great, but better than the two we used in the past.
Created attachment 189002 [details] patch for landing
(In reply to comment #3) > (From update of attachment 188494 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188494&action=review > > >> Source/WebCore/bindings/js/JSDOMBinding.h:62 > >> +#define MissingIsNullString(exec, index) (((index) >= (exec)->argumentCount()) ? (JSValue()) : ((exec)->argument(index))) > > > > Same comment. This should probably be an inline function. If it can't be an inline function, we should keep the ALL_CAPS name. > > Further, both the names MAYBE_MISSING_PARAMETER and MissingIsNullString are examples of the kind of confusing names we try to avoid. A function should be named after its return value. And even though it’s used to implement DefaultIsNullString, this does not return a string, so it’s really confusing to mention string in its name. > > ARGUMENT_OR_NULL (or ArgumentOrNull if it can be an inline function) would be one possible name. Not great, but better than the two we used in the past. Thanks, changed it to an inline function and renamed it to argumentOrNull().
Comment on attachment 189002 [details] patch for landing Clearing flags on attachment: 189002 Committed r143304: <http://trac.webkit.org/changeset/143304>
All reviewed patches have been landed. Closing bug.
This patch breaks our SH4 build slave: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/20416
(In reply to comment #8) > This patch breaks our SH4 build slave: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/20416 Sorry, looking.
Fixed the build error in r143306.
(In reply to comment #10) > Fixed the build error in r143306. Thanks a lot !
Landed another build fix in r143309.
Comment on attachment 189002 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=189002&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2758 > + push(@$outputArray, " " . GetNativeTypeFromSignature($parameter) . " $name(" . JSValueToNative($parameter, $optional eq "DefaultIsNullString" ? "argumentOrNull(exec, $argsIndex)" : "exec->argument($argsIndex)") . ");\n"); It caused the following warning: Use of uninitialized value $optional in string eq at /home/oszi/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm line 2758. Could you check and fix it, please?
(In reply to comment #13) > (From update of attachment 189002 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189002&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2758 > > + push(@$outputArray, " " . GetNativeTypeFromSignature($parameter) . " $name(" . JSValueToNative($parameter, $optional eq "DefaultIsNullString" ? "argumentOrNull(exec, $argsIndex)" : "exec->argument($argsIndex)") . ");\n"); > > It caused the following warning: > Use of uninitialized value $optional in string eq at /home/oszi/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm line 2758. > > Could you check and fix it, please? See the Qt bot for details: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/57635/steps/compile-webkit/logs/stdio
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 189002 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=189002&action=review > > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2758 > > > + push(@$outputArray, " " . GetNativeTypeFromSignature($parameter) . " $name(" . JSValueToNative($parameter, $optional eq "DefaultIsNullString" ? "argumentOrNull(exec, $argsIndex)" : "exec->argument($argsIndex)") . ");\n"); > > > > It caused the following warning: > > Use of uninitialized value $optional in string eq at /home/oszi/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm line 2758. > > > > Could you check and fix it, please? > > See the Qt bot for details: > http://build.webkit.org/builders/Qt%20Linux%20Release/builds/57635/steps/compile-webkit/logs/stdio Fixed by https://trac.webkit.org/changeset/143308
Additionally it broke the bindings tests on all bot. Could you fix it?
(In reply to comment #16) > Additionally it broke the bindings tests on all bot. Could you fix it? ping for fix?
(In reply to comment #17) > (In reply to comment #16) > > Additionally it broke the bindings tests on all bot. Could you fix it? > > ping for fix? Fixed in r143374. Sorry for late.