Bug 154654 – [Web IDL] Add support for [TreatNullAs=EmptyString] and use it

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

[TreatNullAs=NullString] should be renamed to [TreatNullAs=EmptyString] to match Web IDL:
http://heycam.github.io/webidl/#TreatNullAs

The current WebKit behavior is the same as the IDL one [1], only the naming differs. We should align.

Note that [TreatNullAs=EmptyString] is a standard WebIDL extended attribute and is used in specifications. WebKit normally uses it in cases where the specification has it. However, WebKit also uses [TreatNullAs=NullString, TreatUndefined=NullString] for attributes / parameters that should actually be nullable. This is because support for nullable in our bindings generator is currently weak / incomplete. We should eventually use nullable (e.g. DOMString? value) [2] instead of [TreatNullAs=NullString, TreatUndefined=NullString] for those cases but this is a separate issue. Note that [TreatUndefined=XXX] is no longer part of Web IDL and should be dropped entirely once we better support nullable.

[1] http://heycam.github.io/webidl/#es-DOMString
[2] http://heycam.github.io/webidl/#idl-nullable-type

[Darin Adler]

This doesn’t sound exactly right. Null string and empty string are not the same thing in the WebKit C++ DOM implementation. A function can tell the difference between an empty string and a null using the isNull function. It may be true that in some, even many, DOM function implementations, they are both treated the same. But [TreatNullAs=NullString] could be used to implement a form of "nullable" argument whereas [TreatNullAs=EmptyString] is clearly only useful when we actually want to handle the argument as the empty string.

I think you may be saying that all the places we are currently using [TreatNullAs=NullString], we could use [TreatNullAs=EmptyString] instead, and that may well be true. But it requires examining the functions to be sure.

There’s also an open question of whether the bindings should pass a null or empty WTF::String in these cases. It would be safer to pass an empty string if we really don’t want any chance that the C++ function accidentally handles a null differently than the empty string.

[Darin Adler]

Reading your original comment, it seems you already understood this, but I wanted to state it explicitly.

[Chris Dumez]

(In reply to comment #2)
> Reading your original comment, it seems you already understood this, but I
> wanted to state it explicitly.

I am doing a clean up in dependency bugs first. Once those patches land, there hopefully will not be that many uses of [TreatNullAs=NullString]. I do think we should update the bindings implementation to pass emptyString() in this case instead of a null string. Yes, we'll need to check the implementation to make sure it still does the right thing but there shouldn't be that many uses left anyway.

[Chris Dumez]

Created attachment 272428 [details]
Patch

[WebKit Commit Bot]

Attachment 272428 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1083:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 272428 [details]
Patch

Attachment 272428 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/892935

New failing tests:
fast/dom/DOMURL/set-href-attribute-search.html

[Build Bot]

Created attachment 272432 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272428 [details]
Patch

Attachment 272428 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/892940

New failing tests:
fast/dom/DOMURL/set-href-attribute-search.html

[Build Bot]

Created attachment 272433 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 272434 [details]
Patch

[WebKit Commit Bot]

Attachment 272434 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1083:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 272434 [details]
Patch

I wasn’t able to audit every single case so I am taking your word that there is no web-visible change here. Some seem rather tricky, such as HTMLFrameElement.location.

Once we change things so the bindings aren’t passing null strings, it might be a nice tiny cleanup to remove some various bits of code that check for null string and map it to empty string. But we can really only do that if we are sure no internal WebKit callers are passing null strings either.

[WebKit Commit Bot]

Comment on attachment 272434 [details]
Patch

Clearing flags on attachment: 272434

Committed r197353: <http://trac.webkit.org/changeset/197353>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.