Bug 216935

Created attachment 409591 [details]
Patch

Created attachment 409595 [details]
Patch

Created attachment 409596 [details]
Patch

Comment on attachment 409596 [details]
Patch

Nice!

Comment on attachment 409596 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> +        instead of many.

Conceptually, this feels wrong. The intent of platform is to abstract away differences between different platforms base libraries. Having it know about a web platform concept like a specific "render quantum size" seems like a violation of that.

Comment on attachment 409596 [details]
Patch

Crap, bugzilla in-air-conflict made it automatically change the review flag.

Eric, please re-do what ever change to the flag you made.

(In reply to Sam Weinig from comment #5)
> Comment on attachment 409596 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409596&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> > +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> > +        instead of many.
> 
> Conceptually, this feels wrong. The intent of platform is to abstract away
> differences between different platforms base libraries. Having it know about
> a web platform concept like a specific "render quantum size" seems like a
> violation of that.

The situation right now is that we have different constants in platform/ and WebAudio/ that need to be kept in sync. You view it as a WebPlatform concept but I am not sure I agree, this is WebKit's render quantum size.

(In reply to Chris Dumez from comment #7)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 409596 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409596&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> > > +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> > > +        instead of many.
> > 
> > Conceptually, this feels wrong. The intent of platform is to abstract away
> > differences between different platforms base libraries. Having it know about
> > a web platform concept like a specific "render quantum size" seems like a
> > violation of that.
> 
> The situation right now is that we have different constants in platform/ and
> WebAudio/ that need to be kept in sync. You view it as a WebPlatform concept
> but I am not sure I agree, this is WebKit's render quantum size.

To put it in other word, the thing you are saying is bad is not new in my patch. What my patch does it merge 5+ constants (that have to be in sync) into a single one, in a common header. It does not feel that controversial to me.

(In reply to Chris Dumez from comment #7)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 409596 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409596&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> > > +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> > > +        instead of many.
> > 
> > Conceptually, this feels wrong. The intent of platform is to abstract away
> > differences between different platforms base libraries. Having it know about
> > a web platform concept like a specific "render quantum size" seems like a
> > violation of that.
> 
> The situation right now is that we have different constants in platform/ and
> WebAudio/ that need to be kept in sync. You view it as a WebPlatform concept
> but I am not sure I agree, this is WebKit's render quantum size.

Ok, I could buy that.

(In reply to Chris Dumez from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Sam Weinig from comment #5)
> > > Comment on attachment 409596 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=409596&action=review
> > > 
> > > > Source/WebCore/ChangeLog:10
> > > > +        Declare render quantum size [1] constant in AudioUtilities.h so that it can be used in both
> > > > +        platform/ and Modules/webaudio/. Also update the code so that we have a single constant
> > > > +        instead of many.
> > > 
> > > Conceptually, this feels wrong. The intent of platform is to abstract away
> > > differences between different platforms base libraries. Having it know about
> > > a web platform concept like a specific "render quantum size" seems like a
> > > violation of that.
> > 
> > The situation right now is that we have different constants in platform/ and
> > WebAudio/ that need to be kept in sync. You view it as a WebPlatform concept
> > but I am not sure I agree, this is WebKit's render quantum size.
> 
> To put it in other word, the thing you are saying is bad is not new in my
> patch. What my patch does it merge 5+ constants (that have to be in sync)
> into a single one, in a common header. It does not feel that controversial
> to me.

Fair. 

Also, I really didn't mean to change the review flag at all. It was just this dumb site :(. Please put it back to the way it was.

Committed r267541: <https://trac.webkit.org/changeset/267541>

<rdar://problem/69515634>