[Optional=CallWithDefalutValue] and [Optional=CallWithNullValue] are confusing. We should rename them. - [Optional=CallWithDefalutValue] indicates that a missing value is treated as if the JavaScript undefined is passed. - [Optional=CallWithNullValue] indicates that a missing value is treated as the WebKit null value (i.e. JSValue() or v8::Local<v8::Value>()). Actually, the difference between [Optional=CallWithDefalutValue] and [Optional=CallWithNullValue] will appear only when the type of the value is DOMString. In case of [Optional=CallWithDefalutValue], a missing value is converted to the string "undefined". On the other hand, in case of [Optional=CallWithNullValue], a missing value is converted to the WebKit null string. My suggestion is - Rename [Optional=CallWithDefalutValue] to [Optional=TreatAsUndefined] - Remove [Optional=CallWithNullValue]. Instead, we can use [Optional=TreatAsUndefined, TreatUndefinedAs=NullString] Any better idea?
That's a good plan.
Created attachment 126243 [details] Patch
Attachment 126243 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:189: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:190: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:191: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 141 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r107182: <http://trac.webkit.org/changeset/107182>
Comment on attachment 126243 [details] Patch Committed manually to avoid style check errors in generated code.
(In reply to comment #4) > Committed r107182: <http://trac.webkit.org/changeset/107182> Reopen, because it broke many tests on Qt: http://build.webkit.org/results/Qt%20Linux%20Release/r107183%20%2843082%29/results.html Maybe on other platforms too, I didn't check them. Could you fix this regression?
(In reply to comment #6) > (In reply to comment #4) > > Committed r107182: <http://trac.webkit.org/changeset/107182> > > Reopen, because it broke many tests on Qt: > http://build.webkit.org/results/Qt%20Linux%20Release/r107183%20%2843082%29/results.html > > Maybe on other platforms too, I didn't check them. Could you fix this regression? Fixed in r107191. Thanks.
Reverted r107182 for reason: Layout tests of JSC-related port are crashing Committed r107204: <http://trac.webkit.org/changeset/107204>
I rolled out the patch, as I couldn't fix the failure. (In reply to comment #0) > - Rename [Optional=CallWithDefalutValue] to [Optional=TreatAsUndefined] BTW, this should be renamed to [Optional=Undefined]. [Optional=TreatAsUndefined] is confusing with [TreatUndefinedAs=...].
(In reply to comment #0) > - Rename [Optional=CallWithDefalutValue] to [Optional=Undefined] > - Remove [Optional=CallWithNullValue]. Instead, we can use [Optional=Undefined, TreatUndefinedAs=NullString] I found that this renaming is wrong (this is the cause of build failures). If [Optional=Undefined, TreatUndefinedAs=NullString] is specified on a parameter, WebCore cannot distinguish the following two cases: - the parameter is the JavaScript undefined - the parameter is missing In other words, we cannot separately specify the behavior when the parameter is the JavaScript undefined and the behavior when the parameter is missing. Thus, my revised suggestion is - Rename [Optional=CallWithDefalutValue] to [Optional=Undefined] - Rename [Optional=CallWithNullValue] to [Optional=NullString]
Created attachment 126279 [details] Patch
Adam: Review? I confirmed that the uploaded patch passes layout tests which had been crashing in the previous patch.
Attachment 126279 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:189: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:190: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:191: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 127 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 126279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126279&action=review > Source/WebCore/ChangeLog:22 > + - Rename [Optional=CallWithDefalutValue] to [Optional=Undefined]. CallWithDefalutValue => CallWithDefaultValue
Maybe: [Optional=CallWithDefalutValue] to [Optional=DefaultIsUndefined] [Optional=CallWithNullValue] to [Optional=DefaultIsNullString] ? [Optional=Undefined] is slightly mysterious.
Created attachment 126397 [details] Patch
(In reply to comment #15) > Maybe: > > [Optional=CallWithDefalutValue] to [Optional=DefaultIsUndefined] > [Optional=CallWithNullValue] to [Optional=DefaultIsNullString] Renamed as above.
Attachment 126397 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:189: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:190: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:191: The parameter name "str" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 141 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r107304: <http://trac.webkit.org/changeset/107304>