Bug 164516 – Remove LegacyException support from bindings script

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

[Darin Adler]

Now that all uses of ExceptionCode& for bindings have been removed, get rid of LegacyException support from bindings and remove the ExceptionCodePlaceholder.h script as well. There are also some stray places still using ExceptionCode&, but that will be a separate task.

[Darin Adler]

Created attachment 294586 [details]
Patch

[Darin Adler]

Created attachment 294589 [details]
Patch

[Darin Adler]

EWS is passing, so this is ready for review.

[youenn fablet]

Comment on attachment 294589 [details]
Patch

Sounds very good to me to remove all that binding script code!

I am just wondering whether obsolete keywords have been removed everywhere.
It seems that TestInterface.idl would need to be changed for instance.

View in context: https://bugs.webkit.org/attachment.cgi?id=294589&action=review

> Source/WebCore/ChangeLog:11
> +        ExceptionCode&, so to fix that, thuis changes most call sites to use specific create

s/thuis/thus

> Source/WebCore/ChangeLog:41
> +        * bindings/scripts/test/TestImplements.idl: Use non-legacy exceptions.

Should there be also TestInterface.idl in this list?
Might worth rerunning binding tests to ensure everything is fine.

> Source/WebCore/bindings/scripts/test/JS/JSTestNondeterministic.cpp:209
> +    auto& cursor = globalObject->inputCursor();

I guess it is ok like this but we should have globalObject be a ref and not a pointer.
Same for state.

> Source/WebCore/html/FTPDirectoryDocument.cpp:302
> +    auto& document = *this->document();

Should we add an ASSERT(document())?

> Source/WebCore/html/FTPDirectoryDocument.cpp:331
> +    auto& document = *this->document();

Ditto here?

> Source/WebCore/html/MediaDocument.cpp:85
> +    auto& document = *this->document();

Ditto here, or should document() return a reference?

> Source/WebCore/html/PluginDocument.cpp:104
> +    if (auto* loader = document.loader())

Why are we both asserting and doing an if?

[Darin Adler]

Comment on attachment 294589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294589&action=review

Thanks for the review!

>> Source/WebCore/ChangeLog:11
>> +        ExceptionCode&, so to fix that, thuis changes most call sites to use specific create
> 
> s/thuis/thus

Fixed.

>> Source/WebCore/ChangeLog:41
>> +        * bindings/scripts/test/TestImplements.idl: Use non-legacy exceptions.
> 
> Should there be also TestInterface.idl in this list?
> Might worth rerunning binding tests to ensure everything is fine.

Fixed TestInterface.idl and re-ran bindings tests. For some reason the bindings script was not complaining even though that file was still using ConstructorMayThrowLegacyException. After I land I will find out why.

>> Source/WebCore/bindings/scripts/test/JS/JSTestNondeterministic.cpp:209
>> +    auto& cursor = globalObject->inputCursor();
> 
> I guess it is ok like this but we should have globalObject be a ref and not a pointer.
> Same for state.

I got rid of the globalObject local variable since it’s only used in that one place.

>> Source/WebCore/html/FTPDirectoryDocument.cpp:302
>> +    auto& document = *this->document();
> 
> Should we add an ASSERT(document())?

Not really sure. A null pointer quickly results in a typically-easy-to-diagnose crash soon after it’s used, so I’m not sure how critical assertions that pointers are non-null are. We could have one of those assertions every time we dereference a pointer everywhere, like everywhere we use a -> on a pointer. I’m just going to leave this alone for now.

>> Source/WebCore/html/FTPDirectoryDocument.cpp:331
>> +    auto& document = *this->document();
> 
> Ditto here?

Same logic.

>> Source/WebCore/html/MediaDocument.cpp:85
>> +    auto& document = *this->document();
> 
> Ditto here, or should document() return a reference?

It shouldn’t, unless we want to add a function that returns a reference and asserts that the document is non-null for use early in parsing when we know it’s good.

I looked it up and parsers can get into a state where the document is null, but I don’t believe that can happen this early in the parsing process.

Anyway, all that null stuff is not new, and I don’t want to add assertions.

>> Source/WebCore/html/PluginDocument.cpp:104
>> +    if (auto* loader = document.loader())
> 
> Why are we both asserting and doing an if?

This is a relatively common pattern in WebKit.

There are some assertions that we are relatively certain of, and that we want to see in a backtrace or the debugger if they ever happen on a debug build. But that we are not so confident of that we feel comfortable omitting the check from the production build.

If we become even more confident we could remove the if. Or we could find the assertion is incorrect and remove the assertion. But it’s reasonable to have both for now in some places.

[Darin Adler]

Committed r208658: <http://trac.webkit.org/changeset/208658>