Bug 161295 – Streamline DOMImplementation, and move it to our new DOM exception system

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Streamline DOMImplementation, and move it to our new DOM exception system

[Darin Adler]

Created attachment 287216 [details]
Patch

[Darin Adler]

Comment on attachment 287216 [details]
Patch

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

> Source/WebCore/dom/Exception.h:37
> +    WEBCORE_EXPORT ExceptionCode code() const;

This is inline, so it does not need WEBCORE_EXPORT. I will fix that before landing.

[Darin Adler]

The new way of doing exceptions turned out really simple. Might get more complicated as I make it work for more different function return value types, but I think probably not a lot more!

[Sam Weinig]

(In reply to comment #3)
> The new way of doing exceptions turned out really simple. Might get more
> complicated as I make it work for more different function return value
> types, but I think probably not a lot more!

I love it!

[Darin Adler]

Created attachment 287217 [details]
Patch

[Darin Adler]

New patch updates includes in MIMETypeRegistry.cpp. I made a small mistake that was wrong enough that things were building on my Mac, but not on iOS and maybe not Windows either. Should be fixed now.

[Darin Adler]

(If you want me to stage this and land in separate pieces, I am definitely open to that.)

[Chris Dumez]

Comment on attachment 287217 [details]
Patch

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

> Source/WebCore/dom/Exception.h:35
> +    explicit Exception(ExceptionCode);

Now may be a good time to provide better exception messages. So it'd be good to take a message parameter. Do we ever want the generic exception message?

> Source/WebCore/dom/ExceptionOr.h:41
> +    ReturnType&& returnValue();

Maybe takeReturnValue?

[Chris Dumez]

Comment on attachment 287217 [details]
Patch

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

> Source/WebCore/dom/ExceptionOr.h:53
> +    : m_value(WTFMove(returnValue))

Should this be std::forward() given that ReturnType is a template parameter?

[Chris Dumez]

Comment on attachment 287217 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:796
> +    if (value.hasException()) {

UNLIKELY() ?

> Source/WebCore/bindings/js/JSDOMBinding.h:806
> +    if (value.hasException()) {

UNLIKELY()

> Source/WebCore/dom/Document.cpp:1385
> +void Document::setXMLStandalone(bool standalone, ExceptionCode&)

Seems like we should drop the [SetterRaisesException] for this attribute in Document.idl and drop this last parameter.

>> Source/WebCore/dom/Exception.h:35
>> +    explicit Exception(ExceptionCode);
> 
> Now may be a good time to provide better exception messages. So it'd be good to take a message parameter. Do we ever want the generic exception message?

Also note there is a ExceptionCodeWithMessage type somewhere that was added fairly recently.

[Anders Carlsson]

Comment on attachment 287217 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:71
> +template<typename T> class ExceptionOr;

Can omit "T" here.

> Source/WebCore/dom/DOMImplementation.cpp:217
> +    Document::parseQualifiedName(qualifiedName, prefix, localName, ec);

Is this going to return ExceptionOr at some point too? ExceptionOr<void>?

[Darin Adler]

Comment on attachment 287217 [details]
Patch

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

Just realized that GTK build is failing because I didn’t add support for this to the GObject bindings yet. Darn it.

Windows build failure seems to be because of some kind of conflict or ambiguity between WebCore::Exception and JSC::Exception. I did not realize that JSC had a class named Exception in it, but now I do. I wonder if the Windows error is catching something that clang doesn’t catch, or if it’s a bug in the Windows compiler’s handling of namespaces.

>> Source/WebCore/bindings/js/JSDOMBinding.h:71
>> +template<typename T> class ExceptionOr;
> 
> Can omit "T" here.

Will do.

>> Source/WebCore/bindings/js/JSDOMBinding.h:796
>> +    if (value.hasException()) {
> 
> UNLIKELY() ?

We have not historically done that in our bindings, but I suppose we could. I used to be pretty conservative with that and only use it when I knew it sped something up.

>> Source/WebCore/dom/DOMImplementation.cpp:217
>> +    Document::parseQualifiedName(qualifiedName, prefix, localName, ec);
> 
> Is this going to return ExceptionOr at some point too? ExceptionOr<void>?

Yes. But we might have another name for ExceptionOr<void>. OptionalException is not a great name and Optional<Exception> is not great for template meta-programming.

>> Source/WebCore/dom/Document.cpp:1385
>> +void Document::setXMLStandalone(bool standalone, ExceptionCode&)
> 
> Seems like we should drop the [SetterRaisesException] for this attribute in Document.idl and drop this last parameter.

Sure, I can do that now.

Note that once we finish moving to the new exception system there won’t be any RaisesException extended attributes at all any more, so this would have to get cleaned up as part of that.

>>> Source/WebCore/dom/Exception.h:35
>>> +    explicit Exception(ExceptionCode);
>> 
>> Now may be a good time to provide better exception messages. So it'd be good to take a message parameter. Do we ever want the generic exception message?
> 
> Also note there is a ExceptionCodeWithMessage type somewhere that was added fairly recently.

Yes, the intention of this class is that it can hold more than just an exception code, a message parameter as needed. Specifically, I had planned to figure out exactly how to accomodate ExceptionCodeWithMessage in Exception and ExceptionOr the first time I needed to convert a DOM function that was using it. Actually changing each DOM function to provide a better exception message as I convert it from ExceptionCode to ExceptionOr, though, would change this refactoring into something that changes behavior and so changes test results. I’m not entirely sure it should be done as part of the same pass. But maybe you could convince me.

Another reason I didn’t do this now is that I’m not clear what the form and content of better exception messages should be.

>> Source/WebCore/dom/ExceptionOr.h:41
>> +    ReturnType&& returnValue();
> 
> Maybe takeReturnValue?

I like that idea.

>> Source/WebCore/dom/ExceptionOr.h:53
>> +    : m_value(WTFMove(returnValue))
> 
> Should this be std::forward() given that ReturnType is a template parameter?

I am not sure. For the cases I am currently using both will do the same. I guess we will find out when we use this with a type where it makes a difference? If it needs to be std::forward<ReturnType> here then I am not clear what it will need to be in takeReturnValue.

[Darin Adler]

Comment on attachment 287217 [details]
Patch

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

>>> Source/WebCore/dom/DOMImplementation.cpp:217
>>> +    Document::parseQualifiedName(qualifiedName, prefix, localName, ec);
>> 
>> Is this going to return ExceptionOr at some point too? ExceptionOr<void>?
> 
> Yes. But we might have another name for ExceptionOr<void>. OptionalException is not a great name and Optional<Exception> is not great for template meta-programming.

I’m thinking maybe:

    using PossibleException = ExceptionOr<void>;

But maybe we can come up with something shorter that is still clear. Maybe ExceptionOr<void> is the clearest?

[Chris Dumez]

Comment on attachment 287217 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSDOMBinding.h:796
>>> +    if (value.hasException()) {
>> 
>> UNLIKELY() ?
> 
> We have not historically done that in our bindings, but I suppose we could. I used to be pretty conservative with that and only use it when I knew it sped something up.

This is common pattern in the bindings:
if (UNLIKELY(state->hadException()))\n");

And it seems similar.

[Chris Dumez]

Comment on attachment 287217 [details]
Patch

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

>>>> Source/WebCore/dom/Exception.h:35
>>>> +    explicit Exception(ExceptionCode);
>>> 
>>> Now may be a good time to provide better exception messages. So it'd be good to take a message parameter. Do we ever want the generic exception message?
>> 
>> Also note there is a ExceptionCodeWithMessage type somewhere that was added fairly recently.
> 
> Yes, the intention of this class is that it can hold more than just an exception code, a message parameter as needed. Specifically, I had planned to figure out exactly how to accomodate ExceptionCodeWithMessage in Exception and ExceptionOr the first time I needed to convert a DOM function that was using it. Actually changing each DOM function to provide a better exception message as I convert it from ExceptionCode to ExceptionOr, though, would change this refactoring into something that changes behavior and so changes test results. I’m not entirely sure it should be done as part of the same pass. But maybe you could convince me.
> 
> Another reason I didn’t do this now is that I’m not clear what the form and content of better exception messages should be.

Ok, I guess we can add better exception messages later on and having an Exception class will make this easier. It is true that doing it now will mean rebaselining tests.

[Chris Dumez]

Comment on attachment 287217 [details]
Patch

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

>>> Source/WebCore/dom/Document.cpp:1385
>>> +void Document::setXMLStandalone(bool standalone, ExceptionCode&)
>> 
>> Seems like we should drop the [SetterRaisesException] for this attribute in Document.idl and drop this last parameter.
> 
> Sure, I can do that now.
> 
> Note that once we finish moving to the new exception system there won’t be any RaisesException extended attributes at all any more, so this would have to get cleaned up as part of that.

Oh. So how do you plan on getting rid of [SetterRaisesException]? Will setters return an Optional<Exception> ? And can we detect the return type from the bindings without an extended attribute?

[Darin Adler]

Comment on attachment 287217 [details]
Patch

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

>>>> Source/WebCore/bindings/js/JSDOMBinding.h:796
>>>> +    if (value.hasException()) {
>>> 
>>> UNLIKELY() ?
>> 
>> We have not historically done that in our bindings, but I suppose we could. I used to be pretty conservative with that and only use it when I knew it sped something up.
> 
> This is common pattern in the bindings:
> if (UNLIKELY(state->hadException()))\n");
> 
> And it seems similar.

I agree that it’s similar. It always felt a bit different to me since most of those are truly surprising places to encounter an exception, but on reflection I guess it’s not all that different.

As I alluded to above, we used to be a lot more conservative and used LIKELY and UNLIKELY only when we had real evidence they caused measurable speedups.

>>>> Source/WebCore/dom/Document.cpp:1385
>>>> +void Document::setXMLStandalone(bool standalone, ExceptionCode&)
>>> 
>>> Seems like we should drop the [SetterRaisesException] for this attribute in Document.idl and drop this last parameter.
>> 
>> Sure, I can do that now.
>> 
>> Note that once we finish moving to the new exception system there won’t be any RaisesException extended attributes at all any more, so this would have to get cleaned up as part of that.
> 
> Oh. So how do you plan on getting rid of [SetterRaisesException]? Will setters return an Optional<Exception> ? And can we detect the return type from the bindings without an extended attribute?

(In reply to comment #16)
> So how do you plan on getting rid of [SetterRaisesException]? Will
> setters return an Optional<Exception> ?

Yes, they will return an ExceptionOr<void> just like non-attribute functions that can raise an exception but have no return value. As I mentioned to Anders above we might decide to come up with a different name for that, but I think maybe ExceptionOr<void> might be the clearest name.

> And can we detect the return type
> from the bindings without an extended attribute?

Yes, it is quite likely we can, although not guaranteed.

It was neat that for the simplest return value case that this patch uses, I was able to do this for JavaScript bindings without even changing the code generator. Looks like for the GObject bindings I will have to change it a bit, though. And for functions without a return value I am sure this will include some change to the code generator but probably something small; mostly template programming. It might be possible to treat "no return value" as the type void and make non-return-value functions like setters share patterns more with return-value functions like getters. We never had a good reason to do that before, but I think now we do.

There’s likely more adjustment needed for the various more exotic types of return value, too.

[Darin Adler]

Sam, maybe you can help me think through what to do for the Windows thing. I’d hate to lose the beautiful class name WebCore::Exception, but the easiest fix without deeply understanding the Windows compiler issue is presumably to rename it to something like WebCore::DOMException.

[Darin Adler]

Created attachment 287221 [details]
Patch

[Darin Adler]

This new patch addresses all the review feedback I have received so far and contains an attempt at fixing the build issue on Windows.

However, it does not address the issue of making the gobject bindings compile! Sent mail to webkit-dev about that.

[Darin Adler]

Created attachment 287222 [details]
Patch

[Darin Adler]

Comment on attachment 287222 [details]
Patch

Oops, no point in putting this up formally for review before I resolve the gobject bindings issue.

[Darin Adler]

As I hoped, ready to go except for the gobject bindings issue.

[Darin Adler]

Created attachment 287862 [details]
Patch

[Darin Adler]

OK, rebased and re-uploaded so I can see if this builds on GTK. Probably won't, and so then I will fix that and then put up the final version for review.

[Darin Adler]

Created attachment 287863 [details]
Patch

[Darin Adler]

Created attachment 287864 [details]
Patch

[Darin Adler]

Created attachment 287865 [details]
Patch

[Darin Adler]

Created attachment 287867 [details]
Patch

[Darin Adler]

Comment on attachment 287867 [details]
Patch

OK. Ready to review. Now passes on all the EWS bots since I dealt with the GObject bindings!

[Darin Adler]

Sam, Chris, Anders, you all reviewed an earlier version. Thank you! Nothing major changed since then, although I did address your comments, but I think I need one of you to look over the final working version.

[Ryosuke Niwa]

Comment on attachment 287867 [details]
Patch

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

> Source/WebCore/ChangeLog:4760
>  
> +        * xml/parser/XMLDocumentParserLibxml2.cpp:
> +        (WebCore::XMLDocumentParser::startDocument): Updated since setXMLStandalone
> +        no longer can raise an exception.
> +

What's up with this change log change?

[Darin Adler]

Committed r205411: <http://trac.webkit.org/changeset/205411>