[WebIDL] Remove the need to specify [MayThrowException] for operations
Created attachment 327587 [details] Patch
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.
Created attachment 327590 [details] Patch
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.
Created attachment 327593 [details] Patch
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.
Created attachment 327594 [details] Patch
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.
*** Bug 177453 has been marked as a duplicate of this bug. ***
I have a change to remove all the remaining uses of [MayThrowException], so am going to broaden this a bit.
Created attachment 423828 [details] Patch
It's a big one, but it finally does away with [MayThrowException].
And a huge amount of it is just test changes.
Created attachment 423830 [details] Just the code changes (no IDL or test result changes)
Added a version with just the code changes to make it a bit easier to look at.
Created attachment 423898 [details] Patch
Comment on attachment 423898 [details] Patch r=me if the bots are happy. Very nice.
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
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?
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.
OK, done with my review pass.
Created attachment 423956 [details] Patch
Committed r274832: <https://commits.webkit.org/r274832> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423956 [details].
<rdar://problem/75717440>
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(); })));
(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; }
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 #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!
(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>.
(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?
(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.
(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).
(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 :)
(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.
(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.
(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?
(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> { };
Given that detail, I agree: we should use JSDOMConvertBase.h in AddToIncludesForIDLType instead of IDLTypes.h.