Bug 161295

Summary: Streamline DOMImplementation, and move it to our new DOM exception system
Product: WebKit Reporter: Darin Adler <darin>
Component: BindingsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, kling, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch rniwa: review+, rniwa: commit-queue-

Description Darin Adler 2016-08-27 16:10:17 PDT
Streamline DOMImplementation, and move it to our new DOM exception system
Comment 1 Darin Adler 2016-08-27 16:39:23 PDT
Created attachment 287216 [details]
Patch
Comment 2 Darin Adler 2016-08-27 16:41:28 PDT
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.
Comment 3 Darin Adler 2016-08-27 16:48:46 PDT
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!
Comment 4 Sam Weinig 2016-08-27 16:56:23 PDT
(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!
Comment 5 Darin Adler 2016-08-27 16:58:20 PDT
Created attachment 287217 [details]
Patch
Comment 6 Darin Adler 2016-08-27 16:59:23 PDT
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.
Comment 7 Darin Adler 2016-08-27 17:00:11 PDT
(If you want me to stage this and land in separate pieces, I am definitely open to that.)
Comment 8 Chris Dumez 2016-08-27 17:10:34 PDT
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?
Comment 9 Chris Dumez 2016-08-27 17:13:26 PDT
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?
Comment 10 Chris Dumez 2016-08-27 17:23:29 PDT
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.
Comment 11 Anders Carlsson 2016-08-27 17:34:59 PDT
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>?
Comment 12 Darin Adler 2016-08-27 17:56:34 PDT
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.
Comment 13 Darin Adler 2016-08-27 17:58:31 PDT
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?
Comment 14 Chris Dumez 2016-08-27 18:08:10 PDT
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.
Comment 15 Chris Dumez 2016-08-27 18:10:13 PDT
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.
Comment 16 Chris Dumez 2016-08-27 18:12:42 PDT
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?
Comment 17 Darin Adler 2016-08-27 22:03:12 PDT
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.
Comment 18 Darin Adler 2016-08-27 22:04:39 PDT
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.
Comment 19 Darin Adler 2016-08-27 23:20:59 PDT
Created attachment 287221 [details]
Patch
Comment 20 Darin Adler 2016-08-27 23:22:03 PDT
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.
Comment 21 Darin Adler 2016-08-28 00:26:44 PDT
Created attachment 287222 [details]
Patch
Comment 22 Darin Adler 2016-08-28 00:27:39 PDT
Comment on attachment 287222 [details]
Patch

Oops, no point in putting this up formally for review before I resolve the gobject bindings issue.
Comment 23 Darin Adler 2016-08-28 02:20:46 PDT
As I hoped, ready to go except for the gobject bindings issue.
Comment 24 Darin Adler 2016-09-03 08:14:34 PDT
Created attachment 287862 [details]
Patch
Comment 25 Darin Adler 2016-09-03 08:15:22 PDT
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.
Comment 26 Darin Adler 2016-09-03 08:35:57 PDT
Created attachment 287863 [details]
Patch
Comment 27 Darin Adler 2016-09-03 10:41:14 PDT
Created attachment 287864 [details]
Patch
Comment 28 Darin Adler 2016-09-03 12:12:56 PDT
Created attachment 287865 [details]
Patch
Comment 29 Darin Adler 2016-09-03 13:01:12 PDT
Created attachment 287867 [details]
Patch
Comment 30 Darin Adler 2016-09-03 14:07:07 PDT
Comment on attachment 287867 [details]
Patch

OK. Ready to review. Now passes on all the EWS bots since I dealt with the GObject bindings!
Comment 31 Darin Adler 2016-09-03 14:23:13 PDT
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.
Comment 32 Ryosuke Niwa 2016-09-03 16:21:28 PDT
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?
Comment 33 Darin Adler 2016-09-03 16:35:57 PDT
Committed r205411: <http://trac.webkit.org/changeset/205411>