Bug 84058 - Move AudioDestinationChromium FIFO class to its own class.
Summary: Move AudioDestinationChromium FIFO class to its own class.
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: Raymond Toy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-16 11:49 PDT by Raymond Toy
Modified: 2012-04-23 17:39 PDT (History)
3 users (show)

See Also:

Attachments
Patch (21.96 KB, patch)
2012-04-16 11:52 PDT, Raymond Toy 		no flags Details | Formatted Diff | Diff
Patch (22.68 KB, patch)
2012-04-16 13:57 PDT, Raymond Toy 		no flags Details | Formatted Diff | Diff
Patch (22.49 KB, patch)
2012-04-17 10:09 PDT, Raymond Toy 		no flags 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 Raymond Toy 2012-04-16 11:49:26 PDT 
Move AudioDestinationChromium FIFO class to its own class.
Comment 1 Raymond Toy 2012-04-16 11:52:33 PDT 
Created attachment 137372 [details]
Patch
Comment 2 Chris Rogers 2012-04-16 13:00:38 PDT 
Comment on attachment 137372 [details]
Patch

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

> Source/WebCore/WebCore.gypi:3177
> +            'platform/audio/AudioPullFIFO.h',

nit: alpha order

> Source/WebCore/platform/audio/AudioPullFIFO.h:38
> +// fulfill a request.

It's probably a good idea to update this comment to be a little more general.  I think the first part:

"A FIFO (First In First Out) buffer to handle mismatches in"

and the last part:
"This FIFO will ask the provider for more data when necessary to fulfill a request."

Are good, but the middle part can be made more general -- and also maybe add the concept of "pull" in there (as opposed to push)

> Source/WebKit/chromium/src/AudioDestinationChromium.h:34
> +#include "AudioPullFIFO.h"

Can you forward declare this class in the header file, and move this #include to the .cpp?
Comment 3 Raymond Toy 2012-04-16 13:57:46 PDT 
Created attachment 137396 [details]
Patch
Comment 4 Raymond Toy 2012-04-16 13:58:52 PDT 
Comment on attachment 137372 [details]
Patch

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

>> Source/WebCore/WebCore.gypi:3177
>> +            'platform/audio/AudioPullFIFO.h',
> 
> nit: alpha order

Fixed.

>> Source/WebCore/platform/audio/AudioPullFIFO.h:38
>> +// fulfill a request.
> 
> It's probably a good idea to update this comment to be a little more general.  I think the first part:
> 
> "A FIFO (First In First Out) buffer to handle mismatches in"
> 
> and the last part:
> "This FIFO will ask the provider for more data when necessary to fulfill a request."
> 
> Are good, but the middle part can be made more general -- and also maybe add the concept of "pull" in there (as opposed to push)

Updated.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:34
>> +#include "AudioPullFIFO.h"
> 
> Can you forward declare this class in the header file, and move this #include to the .cpp?

Done.
Comment 5 Raymond Toy 2012-04-17 10:09:53 PDT 
Created attachment 137554 [details]
Patch
Comment 6 Chris Rogers 2012-04-23 17:02:53 PDT 
Comment on attachment 137554 [details]
Patch

Thanks Ray!
Comment 7 WebKit Review Bot 2012-04-23 17:39:35 PDT 
Comment on attachment 137554 [details]
Patch

Clearing flags on attachment: 137554

Committed r114970: <http://trac.webkit.org/changeset/114970>
Comment 8 WebKit Review Bot 2012-04-23 17:39:40 PDT 
All reviewed patches have been landed.  Closing bug.