|Summary:||[Web IDL] Add support for [TreatNullAs=EmptyString] and use it|
|Product:||WebKit||Reporter:||Chris Dumez <cdumez>|
|Component:||Bindings||Assignee:||Chris Dumez <cdumez>|
|Severity:||Normal||CC:||buildbot, commit-queue, darin, rniwa, sam|
|Version:||WebKit Nightly Build|
|Bug Depends on:||154659, 154666|
Description Chris Dumez 2016-02-24 14:24:14 PST
[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 , 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)  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.  http://heycam.github.io/webidl/#es-DOMString  http://heycam.github.io/webidl/#idl-nullable-type
Comment 1 Darin Adler 2016-02-25 09:03:28 PST
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.
Comment 2 Darin Adler 2016-02-25 09:04:20 PST
Reading your original comment, it seems you already understood this, but I wanted to state it explicitly.
Comment 3 Chris Dumez 2016-02-25 09:11:16 PST
(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.
Comment 5 WebKit Commit Bot 2016-02-27 17:44:33 PST
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]  Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Build Bot 2016-02-27 18:32:15 PST
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
Comment 7 Build Bot 2016-02-27 18:32:18 PST
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
Comment 8 Build Bot 2016-02-27 18:36:17 PST
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
Comment 9 Build Bot 2016-02-27 18:36:21 PST
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
Comment 11 WebKit Commit Bot 2016-02-27 18:49:32 PST
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]  Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Darin Adler 2016-02-29 08:39:11 PST
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.
Comment 13 WebKit Commit Bot 2016-02-29 09:28:07 PST
Comment on attachment 272434 [details] Patch Clearing flags on attachment: 272434 Committed r197353: <http://trac.webkit.org/changeset/197353>
Comment 14 WebKit Commit Bot 2016-02-29 09:28:11 PST
All reviewed patches have been landed. Closing bug.