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: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 143211 | ||||||||
Attachments: |
|
Description
Adam Bergkvist
2015-10-14 05:59:08 PDT
Created attachment 288947 [details]
Patch
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 ? 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 (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. 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. 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. > 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?
(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. Created attachment 289222 [details]
Patch
Comment on attachment 289222 [details] Patch Clearing flags on attachment: 289222 Committed r206094: <http://trac.webkit.org/changeset/206094> All reviewed patches have been landed. Closing bug. |