Bug 180019 – [WebIDL] Remove the need to specify [MayThrowException]

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebIDL] Remove the need to specify [MayThrowException] for operations

[Sam Weinig]

Created attachment 327587 [details]
Patch

[EWS Watchlist]

Attachment 327587 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMOperation.h:69:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/JSDOMOperation.h:75:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 192 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 327590 [details]
Patch

[EWS Watchlist]

Attachment 327590 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMOperation.h:69:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/JSDOMOperation.h:75:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 186 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 327593 [details]
Patch

[EWS Watchlist]

Attachment 327593 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMOperation.h:69:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/JSDOMOperation.h:75:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 174 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 327594 [details]
Patch

[EWS Watchlist]

Attachment 327594 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMOperation.h:69:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/JSDOMOperation.h:75:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 174 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Shvayka]

*** Bug 177453 has been marked as a duplicate of this bug. ***

[Sam Weinig]

I have a change to remove all the remaining uses of [MayThrowException], so am going to broaden this a bit.

[Sam Weinig]

Created attachment 423828 [details]
Patch

[Sam Weinig]

It's a big one, but it finally does away with [MayThrowException].

[Sam Weinig]

And a huge amount of it is just test changes.

[Sam Weinig]

Created attachment 423830 [details]
Just the code changes (no IDL or test result changes)

[Sam Weinig]

Added a version with just the code changes to make it a bit easier to look at.

[Sam Weinig]

Created attachment 423898 [details]
Patch

[Chris Dumez]

Comment on attachment 423898 [details]
Patch

r=me if the bots are happy. Very nice.

[Darin Adler]

Comment on attachment 423898 [details]
Patch

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

Reading the patch now, spotted a couple errors, but still reading through the rest

> Source/WebCore/ChangeLog:14
> +        compile. To work around this, toJS<>() can now take a lambda as it's value, and

it's -> its

> Source/WebCore/ChangeLog:93
> +        required as we actually look at the return type via deduction and need it match.

need it match -> need it to match

[Darin Adler]

Comment on attachment 423898 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMExceptionHandling.h:101
>          return;

Why not take out the return if we are using else?

[Darin Adler]

Comment on attachment 423898 [details]
Patch

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

Not that we needed to, but if we wanted a smaller patch could have done "allow but ignore MayThrowException" and then did the "remove and forbid MayThrowException" in a follow up.

> Source/WebCore/dom/Element.h:212
> +    // Bindings variants of the above two functions that have the correct return type.

Not 100% sure we need the comment. The names seem self-explanatory and while it’s hard to notice the only difference is the return type, the repercussions of not noticing are very small.

> Source/WebCore/dom/Element.h:214
> +    inline void removeAttributeForBindings(const AtomString& qualifiedName) { removeAttribute(qualifiedName); }
> +    inline void removeAttributeNSForBindings(const AtomString& namespaceURI, const AtomString& localName) { removeAttributeNS(namespaceURI, localName); }

The "inline" here can be omitted and has no effect.

[Darin Adler]

OK, done with my review pass.

[Sam Weinig]

Created attachment 423956 [details]
Patch

[Sam Weinig]

*** Bug 177453 has been marked as a duplicate of this bug. ***

[EWS]

Committed r274832: <https://commits.webkit.org/r274832>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423956 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75717440>

[Don Olmstead]

After this patch I see the following error in a non-unified build.

WebCore/DerivedSources/JSWebGLLoseContext.cpp:146:57: error: use of undeclared identifier 'IDLUndefined'
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.loseContext(); })));
                                                        ^
WebCore/DerivedSources/JSWebGLLoseContext.cpp:161:71: warning: expression result unused [-Wunused-value]
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
                                                                      ^~~~~~~~~~~~~~~~~~~~
JavaScriptCore/PrivateHeaders\JavaScriptCore/ExceptionScope.h:113:16: note: expanded from macro 'RELEASE_AND_RETURN'
        return expression__; \
               ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:161:93: warning: expression result unused [-Wunused-value]
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
                                                                                            ^~~~~~~~~~
JavaScriptCore/PrivateHeaders\JavaScriptCore/ExceptionScope.h:113:16: note: expanded from macro 'RELEASE_AND_RETURN'
        return expression__; \
               ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:161:57: error: use of undeclared identifier 'IDLUndefined'
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));

[Darin Adler]

(In reply to Don Olmstead from comment #26)
> After this patch I see the following error in a non-unified build.

The fix for this is to add the necessary includes. It looks like the AddToIncludesForIDLType function is missing code for the case of "undefined". I think it might be something like this:

    if ($type->name eq "undefined") {
        AddToIncludes("IDLTypes.h", $includesRef, $conditional);
        return;
    }

[Darin Adler]

Can someone who is doing a non-unified build (Don?) try the above addition to CodeGeneratorJS.pm with my suggestion above to see if it works?

[Don Olmstead]

(In reply to Darin Adler from comment #28)
> Can someone who is doing a non-unified build (Don?) try the above addition
> to CodeGeneratorJS.pm with my suggestion above to see if it works?

(In reply to Darin Adler from comment #27)
> (In reply to Don Olmstead from comment #26)
> > After this patch I see the following error in a non-unified build.
> 
> The fix for this is to add the necessary includes. It looks like the
> AddToIncludesForIDLType function is missing code for the case of
> "undefined". I think it might be something like this:
> 
>     if ($type->name eq "undefined") {
>         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
>         return;
>     }

I can give that a shot. Might not get to it until tomorrow though.

Thanks for the hint! Always appreciated!

[Chris Dumez]

(In reply to Don Olmstead from comment #29)
> (In reply to Darin Adler from comment #28)
> > Can someone who is doing a non-unified build (Don?) try the above addition
> > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> 
> (In reply to Darin Adler from comment #27)
> > (In reply to Don Olmstead from comment #26)
> > > After this patch I see the following error in a non-unified build.
> > 
> > The fix for this is to add the necessary includes. It looks like the
> > AddToIncludesForIDLType function is missing code for the case of
> > "undefined". I think it might be something like this:
> > 
> >     if ($type->name eq "undefined") {
> >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> >         return;
> >     }
> 
> I can give that a shot. Might not get to it until tomorrow though.
> 
> Thanks for the hint! Always appreciated!

Follow-up build fix landed in <https://commits.webkit.org/r274906>.

[Sam Weinig]

(In reply to Chris Dumez from comment #30)
> (In reply to Don Olmstead from comment #29)
> > (In reply to Darin Adler from comment #28)
> > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > 
> > (In reply to Darin Adler from comment #27)
> > > (In reply to Don Olmstead from comment #26)
> > > > After this patch I see the following error in a non-unified build.
> > > 
> > > The fix for this is to add the necessary includes. It looks like the
> > > AddToIncludesForIDLType function is missing code for the case of
> > > "undefined". I think it might be something like this:
> > > 
> > >     if ($type->name eq "undefined") {
> > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > >         return;
> > >     }
> > 
> > I can give that a shot. Might not get to it until tomorrow though.
> > 
> > Thanks for the hint! Always appreciated!
> 
> Follow-up build fix landed in <https://commits.webkit.org/r274906>.

Did that cause no bindings test changes?

[Chris Dumez]

(In reply to Sam Weinig from comment #31)
> (In reply to Chris Dumez from comment #30)
> > (In reply to Don Olmstead from comment #29)
> > > (In reply to Darin Adler from comment #28)
> > > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > > 
> > > (In reply to Darin Adler from comment #27)
> > > > (In reply to Don Olmstead from comment #26)
> > > > > After this patch I see the following error in a non-unified build.
> > > > 
> > > > The fix for this is to add the necessary includes. It looks like the
> > > > AddToIncludesForIDLType function is missing code for the case of
> > > > "undefined". I think it might be something like this:
> > > > 
> > > >     if ($type->name eq "undefined") {
> > > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > > >         return;
> > > >     }
> > > 
> > > I can give that a shot. Might not get to it until tomorrow though.
> > > 
> > > Thanks for the hint! Always appreciated!
> > 
> > Follow-up build fix landed in <https://commits.webkit.org/r274906>.
> 
> Did that cause no bindings test changes?

It did, sorry. I updated them at <https://commits.webkit.org/r274909>.

BTW, I don't know if I am going crazy or if something is wrong with my machine but generating the JS bindings seems to be crazy slow. I wonder if we regressed speed somewhat recently.

[Sam Weinig]

(In reply to Chris Dumez from comment #32)
> (In reply to Sam Weinig from comment #31)
> > (In reply to Chris Dumez from comment #30)
> > > (In reply to Don Olmstead from comment #29)
> > > > (In reply to Darin Adler from comment #28)
> > > > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > > > 
> > > > (In reply to Darin Adler from comment #27)
> > > > > (In reply to Don Olmstead from comment #26)
> > > > > > After this patch I see the following error in a non-unified build.
> > > > > 
> > > > > The fix for this is to add the necessary includes. It looks like the
> > > > > AddToIncludesForIDLType function is missing code for the case of
> > > > > "undefined". I think it might be something like this:
> > > > > 
> > > > >     if ($type->name eq "undefined") {
> > > > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > > > >         return;
> > > > >     }
> > > > 
> > > > I can give that a shot. Might not get to it until tomorrow though.
> > > > 
> > > > Thanks for the hint! Always appreciated!
> > > 
> > > Follow-up build fix landed in <https://commits.webkit.org/r274906>.
> > 
> > Did that cause no bindings test changes?
> 
> It did, sorry. I updated them at <https://commits.webkit.org/r274909>.
> 
> BTW, I don't know if I am going crazy or if something is wrong with my
> machine but generating the JS bindings seems to be crazy slow. I wonder if
> we regressed speed somewhat recently.

Can you quantify it anyway? As one data point, I haven't seen any bugs from ap about a compile time slowdown (which I usually do when I regress it).

[Chris Dumez]

(In reply to Chris Dumez from comment #32)
> (In reply to Sam Weinig from comment #31)
> > (In reply to Chris Dumez from comment #30)
> > > (In reply to Don Olmstead from comment #29)
> > > > (In reply to Darin Adler from comment #28)
> > > > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > > > 
> > > > (In reply to Darin Adler from comment #27)
> > > > > (In reply to Don Olmstead from comment #26)
> > > > > > After this patch I see the following error in a non-unified build.
> > > > > 
> > > > > The fix for this is to add the necessary includes. It looks like the
> > > > > AddToIncludesForIDLType function is missing code for the case of
> > > > > "undefined". I think it might be something like this:
> > > > > 
> > > > >     if ($type->name eq "undefined") {
> > > > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > > > >         return;
> > > > >     }
> > > > 
> > > > I can give that a shot. Might not get to it until tomorrow though.
> > > > 
> > > > Thanks for the hint! Always appreciated!
> > > 
> > > Follow-up build fix landed in <https://commits.webkit.org/r274906>.
> > 
> > Did that cause no bindings test changes?
> 
> It did, sorry. I updated them at <https://commits.webkit.org/r274909>.
> 
> BTW, I don't know if I am going crazy or if something is wrong with my
> machine but generating the JS bindings seems to be crazy slow. I wonder if
> we regressed speed somewhat recently.

r274832:
real	1m9.199s
user	0m29.724s
sys	0m38.310s

r273000:
real	1m6.759s
user	0m29.013s
sys	0m36.827s

Seems slow to regenerate bindings tests results on an iMac Pro but at least it's not a recent regression :)

[Don Olmstead]

(In reply to Chris Dumez from comment #30)
> (In reply to Don Olmstead from comment #29)
> > (In reply to Darin Adler from comment #28)
> > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > 
> > (In reply to Darin Adler from comment #27)
> > > (In reply to Don Olmstead from comment #26)
> > > > After this patch I see the following error in a non-unified build.
> > > 
> > > The fix for this is to add the necessary includes. It looks like the
> > > AddToIncludesForIDLType function is missing code for the case of
> > > "undefined". I think it might be something like this:
> > > 
> > >     if ($type->name eq "undefined") {
> > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > >         return;
> > >     }
> > 
> > I can give that a shot. Might not get to it until tomorrow though.
> > 
> > Thanks for the hint! Always appreciated!
> 
> Follow-up build fix landed in <https://commits.webkit.org/r274906>.

So after the change I was still getting the error.

If I manually add #include "JSDOMConvertBase.h" it compiles fine.

[Chris Dumez]

(In reply to Don Olmstead from comment #35)
> (In reply to Chris Dumez from comment #30)
> > (In reply to Don Olmstead from comment #29)
> > > (In reply to Darin Adler from comment #28)
> > > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > > 
> > > (In reply to Darin Adler from comment #27)
> > > > (In reply to Don Olmstead from comment #26)
> > > > > After this patch I see the following error in a non-unified build.
> > > > 
> > > > The fix for this is to add the necessary includes. It looks like the
> > > > AddToIncludesForIDLType function is missing code for the case of
> > > > "undefined". I think it might be something like this:
> > > > 
> > > >     if ($type->name eq "undefined") {
> > > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > > >         return;
> > > >     }
> > > 
> > > I can give that a shot. Might not get to it until tomorrow though.
> > > 
> > > Thanks for the hint! Always appreciated!
> > 
> > Follow-up build fix landed in <https://commits.webkit.org/r274906>.
> 
> So after the change I was still getting the error.

"the" error or a different error.

The error was:
> use of undeclared identifier 'IDLUndefined'

My patch included the right error for that type.

JSDOMConvertBase.h may be needed too, not for IDLUndefined.


> 
> If I manually add #include "JSDOMConvertBase.h" it compiles fine.

[Darin Adler]

(In reply to Chris Dumez from comment #36)
> (In reply to Don Olmstead from comment #35)
> > So after the change I was still getting the error.
> 
> "the" error or a different error.

Yes, can we see the error?

[Don Olmstead]

(In reply to Darin Adler from comment #37)
> (In reply to Chris Dumez from comment #36)
> > (In reply to Don Olmstead from comment #35)
> > > So after the change I was still getting the error.
> > 
> > "the" error or a different error.
> 
> Yes, can we see the error?

Yea sorry I misspoke. Should've been `an` not `the`. It was just on the same line but now on toJS<IDLUndefined>.

WebCore/DerivedSources/JSWebGLLoseContext.cpp:147:57: error: 'IDLUndefined' does not refer to a value
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.loseContext(); })));
                                                        ^
../../Source/WebCore/bindings\IDLTypes.h:96:8: note: declared here
struct IDLUndefined : IDLType<void> { };

[Darin Adler]

Given that detail, I agree: we should use JSDOMConvertBase.h in AddToIncludesForIDLType instead of IDLTypes.h.