Bug 150121

Summary: If a host object is only used as a variadic argument, its bindings header isn't properly included
Product: WebKit Reporter: Adam Bergkvist <adam.bergkvist>
Component: BindingsAssignee: Nael Ouedraogo <nael.ouedp>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, eric.carlson, ggaren, nael.ouedp, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 143211    
Attachments:
Description Flags
Patch
none
Patch none

Adam Bergkvist
Reported 2015-10-14 05:59:08 PDT
For example: in RTCPeerConnection.idl: [StrictTypeChecking, RaisesException] RTCRtpSender addTrack(MediaStreamTrack track, MediaStream... streams); If MediaStream is only mentioned at the line above (as a variadic arg), JSMediaStream.cpp won't be included in JSRTCPeerConnection.cpp.
Attachments
Patch (3.43 KB, patch)
2016-09-15 03:32 PDT, Nael Ouedraogo
no flags
Patch (3.17 KB, patch)
2016-09-19 03:32 PDT, Nael Ouedraogo
no flags
Nael Ouedraogo
Comment 1 2016-09-15 03:32:09 PDT
Nael Ouedraogo
Comment 2 2016-09-15 03:44:30 PDT
I have not added new binding test in the uploaded patch since seems covered by TestObj.idl test. Is it necessary to add a specific test for this ?
Adam Bergkvist
Comment 3 2016-09-15 04:31:38 PDT
Thanks for picking up this bug. There's a FIXME related to this bug at [1] where an extra include is used as a workaround. [1] https://trac.webkit.org/browser/trunk/Source/WebCore/Modules/mediastream/RTCPeerConnection.h#L41
Nael Ouedraogo
Comment 4 2016-09-15 05:37:07 PDT
(In reply to comment #3) > Thanks for picking up this bug. > > There's a FIXME related to this bug at [1] where an extra include is used as > a workaround. > > [1] > https://trac.webkit.org/browser/trunk/Source/WebCore/Modules/mediastream/ > RTCPeerConnection.h#L41 Yes, I removed this workaround in the uploaded patch.
youenn fablet
Comment 5 2016-09-15 06:59:26 PDT
Comment on attachment 288947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288947&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4010 > + AddVariadicToImplIncludes($type, $function->signature->extendedAttributes->{"Conditional"}); Can we write instead AddToImplIncludes(...) unless ...? That would remove the need for a dedicated AddVariadicToImplIncludes, which is not really about variadic so should probably be renamed.
Chris Dumez
Comment 6 2016-09-15 08:57:15 PDT
Comment on attachment 288947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288947&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4010 >> + AddVariadicToImplIncludes($type, $function->signature->extendedAttributes->{"Conditional"}); > > Can we write instead AddToImplIncludes(...) unless ...? > That would remove the need for a dedicated AddVariadicToImplIncludes, which is not really about variadic so should probably be renamed. I think there is already a AddIncludesForTypeInImpl() for this purpose. I also don't think we need a new subroutine that is specific to variadic.
Nael Ouedraogo
Comment 7 2016-09-15 10:06:45 PDT
> Comment on attachment 288947 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=288947&action=review > > >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4010 > >> + AddVariadicToImplIncludes($type, $function->signature->extendedAttributes->{"Conditional"}); > > > > Can we write instead AddToImplIncludes(...) unless ...? > > That would remove the need for a dedicated AddVariadicToImplIncludes, which is not really about variadic so should probably be renamed. > > I think there is already a AddIncludesForTypeInImpl() for this purpose. I > also don't think we need a new subroutine that is specific to variadic. Thanks for the reviews. Actually, I added a new function for variadic since AddIncludesForTypeInImpl() is not supporting conditional attributes. In addition, isCallback argument of AddIncludesForTypeInImpl() must be set to 1 to include the binding header even if the parameter is not a callback. So, is it OK to use AddToImplIncludes() as proposed by Youenn?
Chris Dumez
Comment 8 2016-09-15 10:20:20 PDT
(In reply to comment #7) > > Comment on attachment 288947 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=288947&action=review > > > > >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4010 > > >> + AddVariadicToImplIncludes($type, $function->signature->extendedAttributes->{"Conditional"}); > > > > > > Can we write instead AddToImplIncludes(...) unless ...? > > > That would remove the need for a dedicated AddVariadicToImplIncludes, which is not really about variadic so should probably be renamed. > > > > I think there is already a AddIncludesForTypeInImpl() for this purpose. I > > also don't think we need a new subroutine that is specific to variadic. > Thanks for the reviews. > > Actually, I added a new function for variadic since > AddIncludesForTypeInImpl() is not supporting conditional attributes. In > addition, isCallback argument of AddIncludesForTypeInImpl() must be set to 1 > to include the binding header even if the parameter is not a callback. > > So, is it OK to use AddToImplIncludes() as proposed by Youenn? Fine by me as long as it works.
Nael Ouedraogo
Comment 9 2016-09-19 03:32:38 PDT
WebKit Commit Bot
Comment 10 2016-09-19 05:33:05 PDT
Comment on attachment 289222 [details] Patch Clearing flags on attachment: 289222 Committed r206094: <http://trac.webkit.org/changeset/206094>
WebKit Commit Bot
Comment 11 2016-09-19 05:33:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.