Bug 81221 – Expose attack, release as DynamicsCompressorNode's attributes.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Gao Chun]

According to spec, attack, release should be implemented as DynamicsCompressorNode's attributes.

[Gao Chun]

Created attachment 132053 [details]
Patch

[Build Bot]

Comment on attachment 132053 [details]
Patch

Attachment 132053 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11956687

[WebKit Review Bot]

Comment on attachment 132053 [details]
Patch

Clearing flags on attachment: 132053

Committed r110951: <http://trac.webkit.org/changeset/110951>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

crogers: Why did you mark this patch for commit when the mac-ews said this would fail to compile on Mac?

http://build.webkit.org/builders/Lion%20Intel%20Release%20%28Build%29/builds/7596

[Adam Barth]

DOMDynamicsCompressorNode conflicts with NSObject method release

[Chris Rogers]

(In reply to comment #5)
> crogers: Why did you mark this patch for commit when the mac-ews said this would fail to compile on Mac?
> 
> http://build.webkit.org/builders/Lion%20Intel%20Release%20%28Build%29/builds/7596

Sorry Adam, my bad - should have caught this...

[Chris Rogers]

Kentaro Hara, adding you as possibly somebody who could help with the code generator problem.

It seems that CodeGeneratorObjC.pm does not like to deal with JS attributes which are named "release" as in this patch since it somehow conflicts with the Obj-C release method.  Do you have a suggestion for an easy fix to the code generator?

[Kentaro Hara]

(In reply to comment #8)
> Kentaro Hara, adding you as possibly somebody who could help with the code generator problem.
> 
> It seems that CodeGeneratorObjC.pm does not like to deal with JS attributes which are named "release" as in this patch since it somehow conflicts with the Obj-C release method.  Do you have a suggestion for an easy fix to the code generator?

You can use [ImplementedAs=releaseFunction]. See https://trac.webkit.org/wiki/WebKitIDL#ImplementedAs

[Chris Rogers]

(In reply to comment #9)
> (In reply to comment #8)
> > Kentaro Hara, adding you as possibly somebody who could help with the code generator problem.
> > 
> > It seems that CodeGeneratorObjC.pm does not like to deal with JS attributes which are named "release" as in this patch since it somehow conflicts with the Obj-C release method.  Do you have a suggestion for an easy fix to the code generator?
> 
> You can use [ImplementedAs=releaseFunction]. See https://trac.webkit.org/wiki/WebKitIDL#ImplementedAs

Kentaro, thank you very much for the suggestion.  But it appears that [ImplementedAs] is for "methods" and not "attributes".  I tried the following:


        readonly attribute [ImplementedAs=releaseTime] AudioParam release; // in Seconds

But,  [ImplementedAs] was apparently ignored and the glue code continued to call into DynamicsCompressorNode::release()  (instead of releaseTime)

Any ideas?

[Kentaro Hara]

(In reply to comment #10)
> Kentaro, thank you very much for the suggestion.  But it appears that [ImplementedAs] is for "methods" and not "attributes".  I tried the following:
> 
> 
>         readonly attribute [ImplementedAs=releaseTime] AudioParam release; // in Seconds
> 
> But,  [ImplementedAs] was apparently ignored and the glue code continued to call into DynamicsCompressorNode::release()  (instead of releaseTime)
> 
> Any ideas?

Thanks, we should fix the code generator to support [ImplementedAs] for attributes. Would you file the bug?

[Gao Chun]

Thanks for providing these suggestion. When I remove "release" from the "conflictMethod" blacklist in CodeGeneratorObjC.pm, I can successfully build webkit with this patch and it also works well. The file DOMDynamicsCompressorNode.mm and DOMDynamicsCompressorNode.h generated from DynamicsCompressorNode.idl were not compiled actually. And they are also not used by any other files. It may be not necessary to generate OBJC files for all IDL files. And maybe a new rule is need to generate OBJC files from IDL.

[Gao Chun]

The issue still exist.

[Chris Rogers]

(In reply to comment #13)
> The issue still exist.

Chun, could you please try re-uploading the patch with:

readonly attribute [ImplementedAs=releaseTime] AudioParam release;

(and then changing the method name to "releaseTime()")

Kentaro has landed the fix for this:
https://bugs.webkit.org/show_bug.cgi?id=81605

Thanks!

[Gao Chun]

Created attachment 134231 [details]
Patch

[Build Bot]

Comment on attachment 134231 [details]
Patch

Attachment 134231 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12147980

[Gao Chun]

(In reply to comment #14)
> (In reply to comment #13)
> > The issue still exist.
> 
> Chun, could you please try re-uploading the patch with:
> 
> readonly attribute [ImplementedAs=releaseTime] AudioParam release;
> 
> (and then changing the method name to "releaseTime()")
> 
> Kentaro has landed the fix for this:
> https://bugs.webkit.org/show_bug.cgi?id=81605
> 
> Thanks!

Hi Chris, I pushed a new patch, but it still can not be successfully build on Mac.

[Chris Rogers]

Kentaro, any ideas why the [ImplementedAs] is still not working?

[Kentaro Hara]

(In reply to comment #18)
> Kentaro, any ideas why the [ImplementedAs] is still not working?

Would you paste the IDL file and the generated code that fails to build? I am happy to take a look.

[Kentaro Hara]

(In reply to comment #18)
> Kentaro, any ideas why the [ImplementedAs] is still not working?

FYI, This is the patch that I landed: https://bugs.webkit.org/attachment.cgi?id=133483&action=review Is there any issue around the change in ObjC/DOMTestObj.h and ObjC/DOMTestObj.mm?

[Chris Rogers]

(In reply to comment #19)
> (In reply to comment #18)
> > Kentaro, any ideas why the [ImplementedAs] is still not working?
> 
> Would you paste the IDL file and the generated code that fails to build? I am happy to take a look.

Hi Kentaro, the interesting part of the IDL is here:
readonly attribute [ImplementedAs=releaseTime] AudioParam release; // in Seconds

You can see in the patch in DynamicsCompressorNode.h that the method is called releaseTime():
AudioParam* releaseTime() { return m_release.get(); }

The mac port (EWS bot) still seems unhappy?

[Kentaro Hara]

(In reply to comment #21)
> readonly attribute [ImplementedAs=releaseTime] AudioParam release; // in Seconds
> 
> You can see in the patch in DynamicsCompressorNode.h that the method is called releaseTime():
> AudioParam* releaseTime() { return m_release.get(); }
> 
> The mac port (EWS bot) still seems unhappy?

Ah, maybe the problem is not the method name in WebCore but the method name in ObjC-generated code. [ImplementedAs] just changes the method name in WebCore (e.g. release -> releaseTime), and it doesn't change the method name in ObjC-generated code. Specifically,

 680 - (int)strawberry
 681{
 682    WebCore::JSMainThreadNullState state;
 683    return IMPL->blueberry();
 684}

should be (int)getStrawberry or something like that, to avoid name conflict.

BTW, do you want release() in ObjC? If not, the simplest approach is just to skip generating code for release() in CodeGeneratorObjC.pm. (There are already a lot of attributes/methods that are skipped in ObjC.)

[Chris Rogers]

(In reply to comment #22)
> (In reply to comment #21)
> > readonly attribute [ImplementedAs=releaseTime] AudioParam release; // in Seconds
> > 
> > You can see in the patch in DynamicsCompressorNode.h that the method is called releaseTime():
> > AudioParam* releaseTime() { return m_release.get(); }
> > 
> > The mac port (EWS bot) still seems unhappy?
> 
> Ah, maybe the problem is not the method name in WebCore but the method name in ObjC-generated code. [ImplementedAs] just changes the method name in WebCore (e.g. release -> releaseTime), and it doesn't change the method name in ObjC-generated code. Specifically,
> 
>  680 - (int)strawberry
>  681{
>  682    WebCore::JSMainThreadNullState state;
>  683    return IMPL->blueberry();
>  684}
> 
> should be (int)getStrawberry or something like that, to avoid name conflict.
> 
> BTW, do you want release() in ObjC? If not, the simplest approach is just to skip generating code for release() in CodeGeneratorObjC.pm. (There are already a lot of attributes/methods that are skipped in ObjC.)

Kentaro, thanks for the answer.  I guess we don't need an ObjC binding for this.  How can I skip generating code for release in the IDL file?

[Kentaro Hara]

(In reply to comment #23)
> Kentaro, thanks for the answer.  I guess we don't need an ObjC binding for this.  How can I skip generating code for release in the IDL file?

We cannot do it in the IDL file and need to skip it in CodeGeneratorObjC.pm.

But things are a bit confusing. CodeGeneratorGObject.pm and CodeGeneratorCPP.pm already have SkipAttribute(), SkipFunction() and ShouldSkipType(), which judge if we should skip generating code for each attribute/method. On the other hand, CodeGeneratorObjC.pm does not yet have something like that. There are a lot of attributes/methods that CodeGeneratorObjC.pm cannot generate correctly, but they are not skipped,  and CodeGeneratorObjC.pm just generates "meaningless" code.

So the right fix would be

- Add SkipAttribute() to CodeGeneratorObjC.pm, just like CodeGeneratorGObject.pm.
- Add release() to SkipAttribute().

In the future, maybe we want to

- Add SkipFunction() to CodeGeneratorObjC.pm.
- Add more attributes/methods to SkipAttribute()/SkipFunction() in CodeGeneratorObjC.pm, so that "meaningless" code is not generated
- Rename ShouldSkipType() in CodeGeneratorCPP.pm to SkipAttribute()/SkipFunction(), for consistency.

Of course, you need to do them in this patch.

[Raymond Toy]

Created attachment 136328 [details]
Fix DynamicsCompressorNode idl issue with ObjC release() method.

This is a very minimal patch that fixes the release() issue with the DynamicsCompressorNode idl.  I did not copy SkipAttribute() from CodeGeneratorsGObject.pm and Kentaro suggested.  That didn't work for me and caused an error about a change in the public API of HMLEElement.  I stripped it down to the bare minimum to remove the conflict with release().

Please review this and let me know if this is the correct approach; I'm not at all familiar with these scripts to generate the bindings.

[Chris Rogers]

(In reply to comment #25)
> Created an attachment (id=136328) [details]
> Fix DynamicsCompressorNode idl issue with ObjC release() method.
> 
> This is a very minimal patch that fixes the release() issue with the DynamicsCompressorNode idl.  I did not copy SkipAttribute() from CodeGeneratorsGObject.pm and Kentaro suggested.  That didn't work for me and caused an error about a change in the public API of HMLEElement.  I stripped it down to the bare minimum to remove the conflict with release().
> 
> Please review this and let me know if this is the correct approach; I'm not at all familiar with these scripts to generate the bindings.

Raymond, it looks like you forgot to include the original changes along with your build fix.  Please re-upload patch.

Also, did you check that the .release attribute is actually exposed in the DynamicsCompressorNode on the mac port?  It should be pretty easy to check by setting a break-point on a sample which creates this node (granular.html for example) and looking at the JS object in the developer tools inspector.

[Raymond Toy]

Created attachment 136363 [details]
Patch

[WebKit Review Bot]

Attachment 136363 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/weba..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raymond Toy]

(In reply to comment #26)
> (In reply to comment #25)
> > Created an attachment (id=136328) [details] [details]
> > Fix DynamicsCompressorNode idl issue with ObjC release() method.
> > 
> > This is a very minimal patch that fixes the release() issue with the DynamicsCompressorNode idl.  I did not copy SkipAttribute() from CodeGeneratorsGObject.pm and Kentaro suggested.  That didn't work for me and caused an error about a change in the public API of HMLEElement.  I stripped it down to the bare minimum to remove the conflict with release().
> > 
> > Please review this and let me know if this is the correct approach; I'm not at all familiar with these scripts to generate the bindings.
> 
> Raymond, it looks like you forgot to include the original changes along with your build fix.  Please re-upload patch.

Oops.  Didn't mean to do that.  New patch uploaded.
> 
> Also, did you check that the .release attribute is actually exposed in the DynamicsCompressorNode on the mac port?  It should be pretty easy to check by setting a break-point on a sample which creates this node (granular.html for example) and looking at the JS object in the developer tools inspector.

What I already did was to look at box2d and used the inspector to create a new dynamics compressor node.  I looked at the object and saw that there a release attribute.  I did not inspect the attribute itself, though.

[Raymond Toy]

Created attachment 136368 [details]
Patch

[Chris Rogers]

Eric, can you please confirm if the fix to CodeGeneratorObjC.pm is the right approach?

[Eric Carlson]

(In reply to comment #31)
> Eric, can you please confirm if the fix to CodeGeneratorObjC.pm is the right approach?

Sorry Chris, I have no idea :-(

[Chris Rogers]

Kentaro, can you please confirm if the fix to CodeGeneratorObjC.pm is the right approach?

Thanks

[Kentaro Hara]

Comment on attachment 136368 [details]
Patch

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

> Source/WebCore/Modules/webaudio/DynamicsCompressorNode.h:56
> +    AudioParam* releaseTime() { return m_release.get(); }

Now this can just be release() by removing [ImplementedAs=releaseTime].

> Source/WebCore/Modules/webaudio/DynamicsCompressorNode.idl:35
> +        readonly attribute [ImplementedAs=releaseTime] AudioParam release; // in Seconds

[ImplementedAs=releaseTime] is no longer needed, right?

[Chris Rogers]

(In reply to comment #34)
> (From update of attachment 136368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136368&action=review
> 
> > Source/WebCore/Modules/webaudio/DynamicsCompressorNode.h:56
> > +    AudioParam* releaseTime() { return m_release.get(); }
> 
> Now this can just be release() by removing [ImplementedAs=releaseTime].
> 
> > Source/WebCore/Modules/webaudio/DynamicsCompressorNode.idl:35
> > +        readonly attribute [ImplementedAs=releaseTime] AudioParam release; // in Seconds
> 
> [ImplementedAs=releaseTime] is no longer needed, right?

Yes, I think it can now be simplified as you suggest.

[Raymond Toy]

Created attachment 137785 [details]
Patch

[Kentaro Hara]

Comment on attachment 137785 [details]
Patch

r+ for the IDL part and ObjC part. I am not familiar with the webaudio part though.

[Chris Rogers]

Comment on attachment 137785 [details]
Patch

Web Audio part looks fine.

[WebKit Review Bot]

Comment on attachment 137785 [details]
Patch

Clearing flags on attachment: 137785

Committed r114603: <http://trac.webkit.org/changeset/114603>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.