Bug 150121 – If a host object is only used as a variadic argument, its bindings header isn't properly included

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Adam Bergkvist]

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.

[Nael Ouedraogo]

Created attachment 288947 [details]
Patch

[Nael Ouedraogo]

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]

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]

(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 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 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 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]

(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]

Created attachment 289222 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 289222 [details]
Patch

Clearing flags on attachment: 289222

Committed r206094: <http://trac.webkit.org/changeset/206094>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.