Bug 78200

Summary: Rename [Optional=CallWithDefalutValue] and [Optional=CallWithNullValue]
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, japhet, macpherson, menard, ojan, ossy, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77393    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Kentaro Hara
Reported 2012-02-08 21:36:39 PST
[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?
Attachments
Patch (302.65 KB, patch)
2012-02-08 22:31 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (226.05 KB, patch)
2012-02-09 04:36 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (298.99 KB, patch)
2012-02-09 16:07 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-02-08 21:45:43 PST
That's a good plan.
Kentaro Hara
Comment 2 2012-02-08 22:31:18 PST
Created attachment 126243 [details] Patch
WebKit Review Bot
Comment 3 2012-02-08 22:34:00 PST
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.
Kentaro Hara
Comment 4 2012-02-08 22:59:09 PST
Committed r107182: <http://trac.webkit.org/changeset/107182>
Kentaro Hara
Comment 5 2012-02-08 22:59:59 PST
Comment on attachment 126243 [details] Patch Committed manually to avoid style check errors in generated code.
Csaba Osztrogonác
Comment 6 2012-02-09 00:34:54 PST
(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?
Kentaro Hara
Comment 7 2012-02-09 01:01:38 PST
(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.
Kentaro Hara
Comment 8 2012-02-09 02:01:21 PST
Reverted r107182 for reason: Layout tests of JSC-related port are crashing Committed r107204: <http://trac.webkit.org/changeset/107204>
Kentaro Hara
Comment 9 2012-02-09 02:40:48 PST
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=...].
Kentaro Hara
Comment 10 2012-02-09 04:18:35 PST
(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]
Kentaro Hara
Comment 11 2012-02-09 04:36:13 PST
Created attachment 126279 [details] Patch
Kentaro Hara
Comment 12 2012-02-09 04:38:16 PST
Adam: Review? I confirmed that the uploaded patch passes layout tests which had been crashing in the previous patch.
WebKit Review Bot
Comment 13 2012-02-09 04:39:11 PST
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.
Adam Barth
Comment 14 2012-02-09 10:44:18 PST
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
Adam Barth
Comment 15 2012-02-09 10:45:36 PST
Maybe: [Optional=CallWithDefalutValue] to [Optional=DefaultIsUndefined] [Optional=CallWithNullValue] to [Optional=DefaultIsNullString] ? [Optional=Undefined] is slightly mysterious.
Kentaro Hara
Comment 16 2012-02-09 16:07:05 PST
Created attachment 126397 [details] Patch
Kentaro Hara
Comment 17 2012-02-09 16:07:38 PST
(In reply to comment #15) > Maybe: > > [Optional=CallWithDefalutValue] to [Optional=DefaultIsUndefined] > [Optional=CallWithNullValue] to [Optional=DefaultIsNullString] Renamed as above.
WebKit Review Bot
Comment 18 2012-02-09 16:09:34 PST
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.
Kentaro Hara
Comment 19 2012-02-09 16:25:36 PST
Committed r107304: <http://trac.webkit.org/changeset/107304>
Note You need to log in before you can comment on or make changes to this bug.