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

Description Kentaro Hara 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?
Comment 1 Adam Barth 2012-02-08 21:45:43 PST
That's a good plan.
Comment 2 Kentaro Hara 2012-02-08 22:31:18 PST
Created attachment 126243 [details]
Patch
Comment 3 WebKit Review Bot 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.
Comment 4 Kentaro Hara 2012-02-08 22:59:09 PST
Committed r107182: <http://trac.webkit.org/changeset/107182>
Comment 5 Kentaro Hara 2012-02-08 22:59:59 PST
Comment on attachment 126243 [details]
Patch

Committed manually to avoid style check errors in generated code.
Comment 6 Csaba Osztrogon√°c 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?
Comment 7 Kentaro Hara 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.
Comment 8 Kentaro Hara 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>
Comment 9 Kentaro Hara 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=...].
Comment 10 Kentaro Hara 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]
Comment 11 Kentaro Hara 2012-02-09 04:36:13 PST
Created attachment 126279 [details]
Patch
Comment 12 Kentaro Hara 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.
Comment 13 WebKit Review Bot 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.
Comment 14 Adam Barth 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
Comment 15 Adam Barth 2012-02-09 10:45:36 PST
Maybe:

[Optional=CallWithDefalutValue] to [Optional=DefaultIsUndefined]
[Optional=CallWithNullValue] to [Optional=DefaultIsNullString]

?

[Optional=Undefined] is slightly mysterious.
Comment 16 Kentaro Hara 2012-02-09 16:07:05 PST
Created attachment 126397 [details]
Patch
Comment 17 Kentaro Hara 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.
Comment 18 WebKit Review Bot 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.
Comment 19 Kentaro Hara 2012-02-09 16:25:36 PST
Committed r107304: <http://trac.webkit.org/changeset/107304>