Summary: | Expose attack, release as DynamicsCompressorNode's attributes. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gao Chun <chun.gao> | ||||||||||||||
Component: | Web Audio | Assignee: | 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
Gao Chun
2012-03-15 07:58:00 PDT
Created attachment 132053 [details]
Patch
Comment on attachment 132053 [details] Patch Attachment 132053 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11956687 Comment on attachment 132053 [details] Patch Clearing flags on attachment: 132053 Committed r110951: <http://trac.webkit.org/changeset/110951> All reviewed patches have been landed. Closing bug. 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 DOMDynamicsCompressorNode conflicts with NSObject method release (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... 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? (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 (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? (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? 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. The issue still exist. (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! Created attachment 134231 [details]
Patch
Comment on attachment 134231 [details] Patch Attachment 134231 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12147980 (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. Kentaro, any ideas why the [ImplementedAs] is still not working? (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. (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? (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? (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.) (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? (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. 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.
(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. Created attachment 136363 [details]
Patch
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.
(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. Created attachment 136368 [details]
Patch
Eric, can you please confirm if the fix to CodeGeneratorObjC.pm is the right approach? (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 :-( Kentaro, can you please confirm if the fix to CodeGeneratorObjC.pm is the right approach? Thanks 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? (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. Created attachment 137785 [details]
Patch
Comment on attachment 137785 [details]
Patch
r+ for the IDL part and ObjC part. I am not familiar with the webaudio part though.
Comment on attachment 137785 [details]
Patch
Web Audio part looks fine.
Comment on attachment 137785 [details] Patch Clearing flags on attachment: 137785 Committed r114603: <http://trac.webkit.org/changeset/114603> All reviewed patches have been landed. Closing bug. |