Bug 81221

Summary: Expose attack, release as DynamicsCompressorNode's attributes.
Product: WebKit Reporter: Gao Chun <chun.gao>
Component: Web AudioAssignee: Raymond Toy <rtoy>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, crogers, eric.carlson, feature-media-reviews, haraken, japhet, ojan, rtoy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 81316    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Fix DynamicsCompressorNode idl issue with ObjC release() method.
none
Patch
none
Patch
none
Patch none

Description Gao Chun 2012-03-15 07:58:00 PDT
According to spec, attack, release should be implemented as DynamicsCompressorNode's attributes.
Comment 1 Gao Chun 2012-03-15 08:17:58 PDT
Created attachment 132053 [details]
Patch
Comment 2 Build Bot 2012-03-15 10:33:22 PDT
Comment on attachment 132053 [details]
Patch

Attachment 132053 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11956687
Comment 3 WebKit Review Bot 2012-03-15 23:56:26 PDT
Comment on attachment 132053 [details]
Patch

Clearing flags on attachment: 132053

Committed r110951: <http://trac.webkit.org/changeset/110951>
Comment 4 WebKit Review Bot 2012-03-15 23:56:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Adam Barth 2012-03-16 00:17:46 PDT
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
Comment 6 Adam Barth 2012-03-16 00:19:50 PDT
DOMDynamicsCompressorNode conflicts with NSObject method release
Comment 7 Chris Rogers 2012-03-16 11:56:37 PDT
(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...
Comment 8 Chris Rogers 2012-03-16 12:05:44 PDT
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?
Comment 9 Kentaro Hara 2012-03-16 19:37:11 PDT
(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
Comment 10 Chris Rogers 2012-03-19 12:32:37 PDT
(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?
Comment 11 Kentaro Hara 2012-03-19 17:56:50 PDT
(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?
Comment 12 Gao Chun 2012-03-19 19:01:19 PDT
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.
Comment 13 Gao Chun 2012-03-19 19:08:05 PDT
The issue still exist.
Comment 14 Chris Rogers 2012-03-27 19:02:28 PDT
(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!
Comment 15 Gao Chun 2012-03-28 00:55:27 PDT
Created attachment 134231 [details]
Patch
Comment 16 Build Bot 2012-03-28 01:12:28 PDT
Comment on attachment 134231 [details]
Patch

Attachment 134231 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12147980
Comment 17 Gao Chun 2012-03-28 02:05:59 PDT
(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.
Comment 18 Chris Rogers 2012-03-28 10:37:07 PDT
Kentaro, any ideas why the [ImplementedAs] is still not working?
Comment 19 Kentaro Hara 2012-03-28 16:43:16 PDT
(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.
Comment 20 Kentaro Hara 2012-03-28 16:46:48 PDT
(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?
Comment 21 Chris Rogers 2012-03-28 16:55:47 PDT
(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?
Comment 22 Kentaro Hara 2012-03-28 17:08:41 PDT
(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.)
Comment 23 Chris Rogers 2012-03-28 18:09:21 PDT
(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?
Comment 24 Kentaro Hara 2012-03-28 18:26:08 PDT
(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.
Comment 25 Raymond Toy 2012-04-09 16:01:44 PDT
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.
Comment 26 Chris Rogers 2012-04-09 16:57:17 PDT
(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.
Comment 27 Raymond Toy 2012-04-09 18:37:07 PDT
Created attachment 136363 [details]
Patch
Comment 28 WebKit Review Bot 2012-04-09 18:40:49 PDT
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.
Comment 29 Raymond Toy 2012-04-09 18:47:16 PDT
(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.
Comment 30 Raymond Toy 2012-04-09 18:51:00 PDT
Created attachment 136368 [details]
Patch
Comment 31 Chris Rogers 2012-04-10 16:22:07 PDT
Eric, can you please confirm if the fix to CodeGeneratorObjC.pm is the right approach?
Comment 32 Eric Carlson 2012-04-11 10:37:06 PDT
(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 :-(
Comment 33 Chris Rogers 2012-04-11 11:42:32 PDT
Kentaro, can you please confirm if the fix to CodeGeneratorObjC.pm is the right approach?

Thanks
Comment 34 Kentaro Hara 2012-04-18 13:13:55 PDT
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?
Comment 35 Chris Rogers 2012-04-18 13:18:42 PDT
(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.
Comment 36 Raymond Toy 2012-04-18 15:30:51 PDT
Created attachment 137785 [details]
Patch
Comment 37 Kentaro Hara 2012-04-18 15:33:18 PDT
Comment on attachment 137785 [details]
Patch

r+ for the IDL part and ObjC part. I am not familiar with the webaudio part though.
Comment 38 Chris Rogers 2012-04-18 15:35:39 PDT
Comment on attachment 137785 [details]
Patch

Web Audio part looks fine.
Comment 39 WebKit Review Bot 2012-04-18 19:33:01 PDT
Comment on attachment 137785 [details]
Patch

Clearing flags on attachment: 137785

Committed r114603: <http://trac.webkit.org/changeset/114603>
Comment 40 WebKit Review Bot 2012-04-18 19:33:08 PDT
All reviewed patches have been landed.  Closing bug.