Move FilterType into MediaFilterCollection

Move FilterType into MediaFilterCollection

MediaFilterCollection is the only code that uses FilterType now so
I'm moving the enum into that class and removing all references from
the MediaFilter & derived classes.


BUG=61823
TEST=No new functionality. Existing tests pass.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66125

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66182

[scherkus (not reviewing), 2010-11-11 01:47:12]

nice change

biggest recommendation is to switch from overloaded Add/SelectFilter() to
explicitly named functions (i.e., AddDataSource(), SelectDataSource())

I have a feeling it'll read much better and avoid relying on the name of the
variable to hint at which type of filter we're adding/selecting.

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
File media/base/media_filter_collection.h (right):

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:22: void AddFilter(DataSource* filter);
FYI this will increase the size of this change but style guide isn't too hot on
function overloading and usually recommends it for equivalent types (i.e, char*
and std::string)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

I tend to agree and I'm considering we make it painfully obvious what's going on
and have AddDataSource, AddDemuxer, etc.. and same with SelectXXX()

I don't think we're relying on function overloading in pipeline_impl (i.e.,
there isn't any crazy template magic code) so this should be ok.

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:49: enum FilterType {
I've always hated the Filter prefix and given this is private I say we lop off
the Filter and FILTER_ prefixes

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:57: 
nit: remove extra blank line

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:60: typedef std::list<Tuple2<FilterType,
instead of mixing std and google Tuple2 maybe use std::pair/std::make_pair?

pretty indifferent about that one :)

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:64: // Helper funtion that adds a filter to
the filter list.
funtion -> function

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:68: // downcasting and mapping between
FilterType & Filter class.
nit: & -> and

from scanning I thought you meant a ref type so I imagine others might do the
same (pedantic nit, I know!)

[acolwell GONE FROM CHROMIUM, 2010-11-11 19:24:07]

Implemented suggested changes

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
File media/base/media_filter_collection.h (right):

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:22: void AddFilter(DataSource* filter);
Done. I totally agree.

On 2010/11/11 01:47:12, scherkus wrote:
> FYI this will increase the size of this change but style guide isn't too hot
on
> function overloading and usually recommends it for equivalent types (i.e,
char*
> and std::string)
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...
> 
> I tend to agree and I'm considering we make it painfully obvious what's going
on
> and have AddDataSource, AddDemuxer, etc.. and same with SelectXXX()
> 
> I don't think we're relying on function overloading in pipeline_impl (i.e.,
> there isn't any crazy template magic code) so this should be ok.

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:49: enum FilterType {
On 2010/11/11 01:47:12, scherkus wrote:
> I've always hated the Filter prefix and given this is private I say we lop off
> the Filter and FILTER_ prefixes

Done.

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:57: 
On 2010/11/11 01:47:12, scherkus wrote:
> nit: remove extra blank line

Done.

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:60: typedef std::list<Tuple2<FilterType,
On 2010/11/11 01:47:12, scherkus wrote:
> instead of mixing std and google Tuple2 maybe use std::pair/std::make_pair?
> 
> pretty indifferent about that one :)

Done.

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:64: // Helper funtion that adds a filter to
the filter list.
On 2010/11/11 01:47:12, scherkus wrote:
> funtion -> function

Done.

http://codereview.chromium.org/4653005/diff/1/media/base/media_filter_collect...
media/base/media_filter_collection.h:68: // downcasting and mapping between
FilterType & Filter class.
On 2010/11/11 01:47:12, scherkus wrote:
> nit: & -> and
> 
> from scanning I thought you meant a ref type so I imagine others might do the
> same (pedantic nit, I know!)

Done.

[scherkus (not reviewing), 2010-11-11 19:33:35]

LGTM this is awesome

http://codereview.chromium.org/4653005/diff/5001/media/tools/player_x11/playe...
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/4653005/diff/5001/media/tools/player_x11/playe...
media/tools/player_x11/player_x11.cc:102: collection->AddDataSource(new
media::FileDataSource());
wow this code reads much nicer :)

[acolwell GONE FROM CHROMIUM, 2010-11-15 21:30:03]

- Fix windows build buster. (Verified on try server)
- Fix movie.cc header so presubmit check passes.

[scherkus (not reviewing), 2010-11-15 21:48:27]

LGTM