Bug 154654 - [Web IDL] Add support for [TreatNullAs=EmptyString] and use it
Summary: [Web IDL] Add support for [TreatNullAs=EmptyString] and use it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://heycam.github.io/webidl/#Treat...
Keywords:
Depends on: 154659 154666
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-24 14:24 PST by Chris Dumez
Modified: 2016-02-29 09:28 PST (History)
5 users (show)

See Also:


Attachments
Patch (57.24 KB, patch)
2016-02-27 17:43 PST, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (898.87 KB, application/zip)
2016-02-27 18:32 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (755.51 KB, application/zip)
2016-02-27 18:36 PST, Build Bot
no flags Details
Patch (57.31 KB, patch)
2016-02-27 18:48 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 [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
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 4 Chris Dumez 2016-02-27 17:43:12 PST
Created attachment 272428 [details]
Patch
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] [4]
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 10 Chris Dumez 2016-02-27 18:48:16 PST
Created attachment 272434 [details]
Patch
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] [4]
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.