Bug 68382 - Add settings to control the availability of the Web Audio API to WebKit and WebKit2.
Summary: Add settings to control the availability of the Web Audio API to WebKit and W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-19 13:19 PDT by Jer Noble
Modified: 2011-09-21 16:17 PDT (History)
2 users (show)

See Also:

Attachments
Patch (7.33 KB, patch)
2011-09-19 13:26 PDT, Jer Noble 		no flags Details | Formatted Diff | Diff
Updated patch to address Darin's comment about the many lines of code dedicated to null-checks. (7.40 KB, patch)
2011-09-20 09:02 PDT, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2011-09-20 10:16 PDT, Jer Noble 		darin: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-09-19 13:19:40 PDT 
Add settings to control the availability of the Web Audio API to WebKit and WebKit2.
Comment 1 Jer Noble 2011-09-19 13:26:24 PDT 
Created attachment 107908 [details]
Patch
Comment 2 Darin Adler 2011-09-19 18:16:08 PDT 
Comment on attachment 107908 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:616
>  #if ENABLE(WEB_AUDIO)
>  JSValue JSDOMWindow::webkitAudioContext(ExecState* exec) const
>  {
> +    Frame* frame = impl()->frame();
> +    if (!frame)
> +        return jsUndefined();
> +    Settings* settings = frame->settings();
> +    if (!settings || !settings->webAudioEnabled())
> +        return jsUndefined();
>      return getDOMConstructor<JSAudioContextConstructor>(exec, this);
>  }
>  #endif

I wish there was a tighter way to implement this so the null checks didn’t take so many lines of code.

Does the webkitAudioContext property still show up if you iterate properties?
Comment 3 Jer Noble 2011-09-19 23:34:59 PDT 
(In reply to comment #2)
> (From update of attachment 107908 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107908&action=review
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:616
> >  #if ENABLE(WEB_AUDIO)
> >  JSValue JSDOMWindow::webkitAudioContext(ExecState* exec) const
> >  {
> > +    Frame* frame = impl()->frame();
> > +    if (!frame)
> > +        return jsUndefined();
> > +    Settings* settings = frame->settings();
> > +    if (!settings || !settings->webAudioEnabled())
> > +        return jsUndefined();
> >      return getDOMConstructor<JSAudioContextConstructor>(exec, this);
> >  }
> >  #endif
> 
> I wish there was a tighter way to implement this so the null checks didn’t take so many lines of code.

I do as well.  This same pattern is used a couple of other places in this file; perhaps it could be pulled out into a file-static helper function?  I'll see what I can do.

> Does the webkitAudioContext property still show up if you iterate properties?

Just checked, and yes it still does.  I used the same pattern as we're using for the WebGL setting, and the WebGL properties still appear on Window as well.
Comment 4 Jer Noble 2011-09-20 09:02:56 PDT 
Created attachment 108009 [details]
Updated patch to address Darin's comment about the many lines of code dedicated to null-checks.
Comment 5 Jer Noble 2011-09-20 10:09:21 PDT 
Comment on attachment 108009 [details]
Updated patch to address Darin's comment about the many lines of code dedicated to null-checks.

Whoops, this will never work.  Uploading a new patch soon.
Comment 6 Jer Noble 2011-09-20 10:16:48 PDT 
Created attachment 108015 [details]
Patch

More LOC than the previous updated patch, but now with 100% fewer compile errors.
Comment 7 Darin Adler 2011-09-20 10:20:50 PDT 
Comment on attachment 108015 [details]
Patch

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

I think this version of runtime-enabled properties is not good. The feature will still seem to be there for people who use best practices such as:

    if (webkitAudioContext in window)

I think that needs to be resolved. I believe V8 currently has a better version of runtime-enabled features.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:610
> +    Frame* frame = impl()->frame();
> +    Settings* settings = frame ? frame->settings() : 0;

We could make a helper function to get settings and so remove one line of code from all of these.
Comment 8 Jer Noble 2011-09-20 10:48:16 PDT 
(In reply to comment #7)
> (From update of attachment 108015 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108015&action=review
> 
> I think this version of runtime-enabled properties is not good. The feature will still seem to be there for people who use best practices such as:
> 
>     if (webkitAudioContext in window)
> 
> I think that needs to be resolved. I believe V8 currently has a better version of runtime-enabled features.

Indeed.  I think the only way to do this is to manually handle each runtime-enabled property in JSDOMWindow::getOwnPropertySlot().  But I think Sam might have an idea about this; I'll talk to him.

> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:610
> > +    Frame* frame = impl()->frame();
> > +    Settings* settings = frame ? frame->settings() : 0;
> 
> We could make a helper function to get settings and so remove one line of code from all of these.

Will do.  Thanks!
Comment 9 Jer Noble 2011-09-21 16:05:52 PDT 
Committed r95678: <http://trac.webkit.org/changeset/95678>
Comment 10 Adam Roben (:aroben) 2011-09-21 16:09:13 PDT 
What about WebKit1 on Windows?
Comment 11 Jer Noble 2011-09-21 16:17:10 PDT 
(In reply to comment #10)
> What about WebKit1 on Windows?

Chris Rogers has already added a WebKit1 API for controlling this setting.