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:

Bindings

Assignee:

Nael Ouedraogo <nael.ouedp>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, eric.carlson, ggaren, nael.ouedp, youennf

Priority:

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	Details Formatted Diff Diff
Patch (3.17 KB, patch) 2016-09-19 03:32 PDT, Nael Ouedraogo	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Nael Ouedraogo

Comment 1 2016-09-15 03:32:09 PDT

Created attachment 288947 [details] Patch

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

Created attachment 289222 [details] Patch

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.