Bug 82599 – IDLParser.pm should be able to parse sequence<T> as method argument

• Component: New Bugs
• Status: RESOLVED FIXED

[Vineet Chaudhary (vineetc)]

Currently IDLParser.pm is not able to parse sequence<T> if it is one methods argument.

[Vineet Chaudhary (vineetc)]

Created attachment 134554 [details]
proposed patch

Attaching proposed patch to get comments.
This is to add support for parser to parse arguments of type sequence<T>.
Though currently no function Source Code passes sequence<T> as argument but we can consider this.

Please let me know your review comments on this. Thanks.

[Kentaro Hara]

Comment on attachment 134554 [details]
proposed patch

The patch looks OK.

[Vineet Chaudhary (vineetc)]

Comment on attachment 134554 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134554&action=review

Thanks haraken for quick review.

> Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:697
>  {


Just doubtful here "WebDOMsequence<ScriptProfile>& sequenceArg" is this desired? Or may be this can be fixed when we actually start using sequence<T> as argument.

[Vineet Chaudhary (vineetc)]

Comment on attachment 134554 [details]
proposed patch

Marking for review.

[Kentaro Hara]

Comment on attachment 134554 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134554&action=review

>> Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:697
>>  {
> 
> Just doubtful here "WebDOMsequence<ScriptProfile>& sequenceArg" is this desired? Or may be this can be fixed when we actually start using sequence<T> as argument.

Oh, good catch! We are not planning to support sequence<T> in ObjC, GObject and CPP, for now. Then shall we skip generating code for sequence<T> in those three bindings? (See SkipFunction() in CodeGeneratorGObject.pm, ShouldSkipType() in CodeGeneratorCPP.pm. You might need to add something like it to CodeGeneratorObjC.pm.)

[Vineet Chaudhary (vineetc)]

(In reply to comment #5)
> (From update of attachment 134554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134554&action=review
> 
> >> Source/WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:697
> >>  {
> > 
> > Just doubtful here "WebDOMsequence<ScriptProfile>& sequenceArg" is this desired? Or may be this can be fixed when we actually start using sequence<T> as argument.
> 
> Oh, good catch! We are not planning to support sequence<T> in ObjC, GObject and CPP, for now. Then shall we skip generating code for sequence<T> in those three bindings? (See SkipFunction() in CodeGeneratorGObject.pm, ShouldSkipType() in CodeGeneratorCPP.pm. You might need to add something like it to CodeGeneratorObjC.pm.)

Sure I will update patch :)

[Adam Barth]

Is there an example of a feature that would use this functionality?

[Vineet Chaudhary (vineetc)]

(In reply to comment #7)
> Is there an example of a feature that would use this functionality?

As of now we don't have any plan to use this functionality but we can consider the using sequence<T> in implementations like 
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/MessagePort.idl?rev=106737#L40
where it passes Array.

[Vineet Chaudhary (vineetc)]

Created attachment 134636 [details]
updated_patch

[Adam Barth]

I would have thought in-parameters would use T[] rather than Sequence<T>, but I'm not sure I fully understand the subtle differences between the two.

[Kentaro Hara]

Comment on attachment 134636 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134636&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm:472
> +                if ($codeGenerator->GetArrayType($param->type)) {
> +                    next TOP;
> +                }

Scattering this kind of code around is a bit dirty. Shall we put the code in ShouldSkipType($function)? e.g.

sub ShouldSkipType() {
  ...;
  foreach my $param (@{$function->parameters}) {
    return 1 if $codeGenerator->GetArrayType($param->type);
  }
  ...;
}

Same comments for all "next TOP" in this patch.

> Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:848
>          foreach my $function (@{$dataNode->functions}) {

You can put something like ShouldSkipType() here. Maybe you need to prepare ShouldSkipType() in CodeGeneratorObjC.pm.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3606
> +                    AddToImplIncludes($arrayType);

This should be "${arrayType}.h"

[Kentaro Hara]

(In reply to comment #10)
> I would have thought in-parameters would use T[] rather than Sequence<T>, but I'm not sure I fully understand the subtle differences between the two.

vineet: Would you confirm the spec around this issue?

[Vineet Chaudhary (vineetc)]

(In reply to comment #12)
> (In reply to comment #10)
> > I would have thought in-parameters would use T[] rather than Sequence<T>, but I'm not sure I fully understand the subtle differences between the two.
> 
> vineet: Would you confirm the spec around this issue?

While going through spec. for MessagePort http://dev.w3.org/html5/postmsg/#dom-messageport-postmessage or http://www.w3.org/TR/html5/comms.html#messageportarray

The signature of postMessage() is like :
void postMessage(any message, optional sequence<Transferable> transfer);

Does that means in-parameter can use sequence<T> ?

[Vineet Chaudhary (vineetc)]

(In reply to comment #11)
> (From update of attachment 134636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134636&action=review

> Scattering this kind of code around is a bit dirty. Shall we put the code in ShouldSkipType($function)? e.g.
> 

> You can put something like ShouldSkipType() here. Maybe you need to prepare ShouldSkipType() in CodeGeneratorObjC.pm.

Added SkipFuntion and SkipAttribute.
The reason we could not keep common implementation as SkipType is 
foreach my $param (@{$function->parameters}) 

Attributes doesn't have call to "parameters" so I though of having separate functions is it oke?


> This should be "${arrayType}.h"
Done.

[Vineet Chaudhary (vineetc)]

Created attachment 134756 [details]
Patch

[Kentaro Hara]

Comment on attachment 134756 [details]
Patch

Thanks for the patch!

[Kentaro Hara]

(In reply to comment #13)
> While going through spec. for MessagePort http://dev.w3.org/html5/postmsg/#dom-messageport-postmessage or http://www.w3.org/TR/html5/comms.html#messageportarray
> 
> The signature of postMessage() is like :
> void postMessage(any message, optional sequence<Transferable> transfer);
> 
> Does that means in-parameter can use sequence<T> ?

Thanks for the clarification. I think the spec supports your patch.

(In reply to comment #14)
> Added SkipFuntion and SkipAttribute.
> The reason we could not keep common implementation as SkipType is 
> foreach my $param (@{$function->parameters}) 
> 
> Attributes doesn't have call to "parameters" so I though of having separate functions is it oke?

Yes, it is a desirable change.

[Vineet Chaudhary (vineetc)]

Comment on attachment 134756 [details]
Patch

(In reply to comment #17)
> (In reply to comment #13)
> > While going through spec. for MessagePort http://dev.w3.org/html5/postmsg/#dom-messageport-postmessage or http://www.w3.org/TR/html5/comms.html#messageportarray
> > 
> > The signature of postMessage() is like :
> > void postMessage(any message, optional sequence<Transferable> transfer);
> > 
> > Does that means in-parameter can use sequence<T> ?
> 
> Thanks for the clarification. I think the spec supports your patch.

Thanks haraken.. :) Should I take this as next <TO_DO> task..?

[Kentaro Hara]

(In reply to comment #18)
> Thanks haraken.. :) Should I take this as next <TO_DO> task..?

We are always happy to kill custom bindings:) (unless it messes code generators)

[WebKit Review Bot]

Comment on attachment 134756 [details]
Patch

Clearing flags on attachment: 134756

Committed r112653: <http://trac.webkit.org/changeset/112653>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.