Bug 78200 – Rename [Optional=CallWithDefalutValue] and [Optional=CallWithNullValue]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

[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?

[Adam Barth]

That's a good plan.

[Kentaro Hara]

Created attachment 126243 [details]
Patch

[WebKit Review Bot]

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]

Committed r107182: <http://trac.webkit.org/changeset/107182>

[Kentaro Hara]

Comment on attachment 126243 [details]
Patch

Committed manually to avoid style check errors in generated code.

[Csaba Osztrogonác]

(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]

(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]

Reverted r107182 for reason:

Layout tests of JSC-related port are crashing

Committed r107204: <http://trac.webkit.org/changeset/107204>

[Kentaro Hara]

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]

(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]

Created attachment 126279 [details]
Patch

[Kentaro Hara]

Adam: Review? I confirmed that the uploaded patch passes layout tests which had been crashing in the previous patch.

[WebKit Review Bot]

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 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]

Maybe:

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

?

[Optional=Undefined] is slightly mysterious.

[Kentaro Hara]

Created attachment 126397 [details]
Patch

[Kentaro Hara]

(In reply to comment #15)
> Maybe:
> 
> [Optional=CallWithDefalutValue] to [Optional=DefaultIsUndefined]
> [Optional=CallWithNullValue] to [Optional=DefaultIsNullString]

Renamed as above.

[WebKit Review Bot]

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]

Committed r107304: <http://trac.webkit.org/changeset/107304>