Create media.dll / libmedia.so for the component build

Create media.dll
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95496

[scherkus (not reviewing), 2011-07-29 17:22:51]

syntactically it's LGTM but I've got a few questions about shared build

So a lot of classes got tacked on with MEDIA_API but the callees are typically
media_unittests (or even other unittests).

If we have a bunch of unittests doesn't that end up adding MEDIA_API to
everything?

http://codereview.chromium.org/7523051/diff/3008/media/audio/audio_util.h
File media/audio/audio_util.h (right):

http://codereview.chromium.org/7523051/diff/3008/media/audio/audio_util.h#new...
media/audio/audio_util.h:81: void SwizzleCoreAudioLayout5_1(Format* b, uint32
filled) {
hmm I'll move this function into a better place in a separate CL:
http://codereview.chromium.org/7533020

http://codereview.chromium.org/7523051/diff/3008/media/audio/fake_audio_outpu...
File media/audio/fake_audio_output_stream.h (right):

http://codereview.chromium.org/7523051/diff/3008/media/audio/fake_audio_outpu...
media/audio/fake_audio_output_stream.h:18: class MEDIA_API FakeAudioOutputStream
: public AudioOutputStream {
looks like the only non-media external user is:
content/browser/renderer_host/media/audio_renderer_host_unittest.cc

so would this be considered something we should clean up or is it not worth it?

http://codereview.chromium.org/7523051/diff/3008/media/audio/simple_sources.h
File media/audio/simple_sources.h (right):

http://codereview.chromium.org/7523051/diff/3008/media/audio/simple_sources.h...
media/audio/simple_sources.h:14: class MEDIA_API SineWaveAudioSource
only unittests use this, is that ok?

http://codereview.chromium.org/7523051/diff/3008/media/base/channel_layout.h
File media/base/channel_layout.h (right):

http://codereview.chromium.org/7523051/diff/3008/media/base/channel_layout.h#...
media/base/channel_layout.h:89: MEDIA_API int
ChannelLayoutToChannelCount(ChannelLayout layout);
ahh!

content/browser/speech/speech_recognizer_unittest.cc

[rvargas (doing something else), 2011-07-29 20:12:04]

Looks like we need MEDIA_TEST for this one?

[darin (slow to review), 2011-07-29 20:41:38]

We could do that, but I'm not sure I see the value.  It seems to add
maintenance overhead if you have to decide whether an exported function is
exported only for a test.  There is also no help from the build system to
ensure that developers only use "functions exported for tests" in tests.
 They could easily make use of those functions in production and the system
wouldn't complain.

-Darin



On Fri, Jul 29, 2011 at 1:12 PM, <rvargas@chromium.org> wrote:

> Looks like we need MEDIA_TEST for this one?
>
>
>
http://codereview.chromium.**org/7523051/<http://codereview.chromium.org/7523...
>

[darin (slow to review), 2011-07-29 20:47:52]

On Fri, Jul 29, 2011 at 10:22 AM, <scherkus@chromium.org> wrote:

> syntactically it's LGTM but I've got a few questions about shared build
>
> So a lot of classes got tacked on with MEDIA_API but the callees are
> typically
> media_unittests (or even other unittests).
>
> If we have a bunch of unittests doesn't that end up adding MEDIA_API to
> everything?
>

Yes, it would.

Maybe the problem is with the name "MEDIA_API"?  Maybe the fact that it
contains the term "API" makes it sound like it should only document things
that are intended for consumers of the media library to use?

I had only intended this as a way to mark functions to be exported from the
media DLL.  Perhaps MEDIA_EXPORT would have been a better choice.

In the net/ module, a distinction is made between functions exported for
consumers to use and functions exported for tests to use: NET_API and
NET_TEST.

I suspect that any class not intended to be used by consumers should
live within the media::internal:: namespace.  That way it becomes obvious
that the class is a private implementation detail.  I think it is reasonable
to
have private APIs... hence MEDIA_API applied to a media::internal:: class.

I think media::internal:: + MEDIA_API is better than MEDIA_API + MEDIA_TEST.

-Darin



>
>
> http://codereview.chromium.**org/7523051/diff/3008/media/**
>
audio/audio_util.h<http://codereview.chromium.org/7523051/diff/3008/media/audio/audio_util.h>
> File media/audio/audio_util.h (right):
>
> http://codereview.chromium.**org/7523051/diff/3008/media/**
>
audio/audio_util.h#newcode81<http://codereview.chromium.org/7523051/diff/3008/media/audio/audio_util.h#newcode81>
> media/audio/audio_util.h:81: void SwizzleCoreAudioLayout5_1(**Format* b,
> uint32 filled) {
> hmm I'll move this function into a better place in a separate CL:
>
http://codereview.chromium.**org/7533020<http://codereview.chromium.org/7533020>
>
> http://codereview.chromium.**org/7523051/diff/3008/media/**
>
audio/fake_audio_output_**stream.h<http://codereview.chromium.org/7523051/diff/3008/media/audio/fake_audio_output_stream.h>
> File media/audio/fake_audio_output_**stream.h (right):
>
> http://codereview.chromium.**org/7523051/diff/3008/media/**
>
audio/fake_audio_output_**stream.h#newcode18<http://codereview.chromium.org/7523051/diff/3008/media/audio/fake_audio_output_stream.h#newcode18>
> media/audio/fake_audio_output_**stream.h:18: class MEDIA_API
> FakeAudioOutputStream : public AudioOutputStream {
> looks like the only non-media external user is:
> content/browser/renderer_host/**media/audio_renderer_host_**unittest.cc
>
> so would this be considered something we should clean up or is it not
> worth it?
>
> http://codereview.chromium.**org/7523051/diff/3008/media/**
>
audio/simple_sources.h<http://codereview.chromium.org/7523051/diff/3008/media/audio/simple_sources.h>
> File media/audio/simple_sources.h (right):
>
> http://codereview.chromium.**org/7523051/diff/3008/media/**
>
audio/simple_sources.h#**newcode14<http://codereview.chromium.org/7523051/diff/3008/media/audio/simple_sources.h#newcode14>
> media/audio/simple_sources.h:**14: class MEDIA_API SineWaveAudioSource
> only unittests use this, is that ok?
>
> http://codereview.chromium.**org/7523051/diff/3008/media/**
>
base/channel_layout.h<http://codereview.chromium.org/7523051/diff/3008/media/base/channel_layout.h>
> File media/base/channel_layout.h (right):
>
> http://codereview.chromium.**org/7523051/diff/3008/media/**
>
base/channel_layout.h#**newcode89<http://codereview.chromium.org/7523051/diff/3008/media/base/channel_layout.h#newcode89>
> media/base/channel_layout.h:**89: MEDIA_API int
> ChannelLayoutToChannelCount(**ChannelLayout layout);
> ahh!
>
> content/browser/speech/speech_**recognizer_unittest.cc
>
>
http://codereview.chromium.**org/7523051/<http://codereview.chromium.org/7523...
>

[rvargas (doing something else), 2011-07-29 21:12:47]

Yes it adds overhead to think about whether the class or method is intended for
consumers or not, but it is way less overhead than having to scope internal
classes into a namespace.

The lack of support from the build system is an issue, but once a class has been
marked with XX_TEST is also relatively obvious that is not intended to be used
for things that are not tests. On the other hand, stuff clearly marked with
::internal in base ended up being used by code from chrome so it is not like any
system is developer proof. This is one of those things that become more clear as
more people gets familiar with code that uses the annotation... with time (think
OVERRIDE).

I think that part of going though this exercise is to get closer to better
interfaces between modules, not just to export everything and be done with it.
At least that was the reasoning behind naming this _API, to start conveying the
sense that this is the public interface of the module.

I would consider xx_API applied to internal stuff to be actually more confusing,
even if there is a namespace involved.

On 2011/07/29 20:41:38, darin wrote:
> We could do that, but I'm not sure I see the value.  It seems to add
> maintenance overhead if you have to decide whether an exported function is
> exported only for a test.  There is also no help from the build system to
> ensure that developers only use "functions exported for tests" in tests.
>  They could easily make use of those functions in production and the system
> wouldn't complain.
> 
> -Darin
> 
> 
> 
> On Fri, Jul 29, 2011 at 1:12 PM, <mailto:rvargas@chromium.org> wrote:
> 
> > Looks like we need MEDIA_TEST for this one?
> >
> >
> >
>
http://codereview.chromium.**org/7523051/%3Chttp://codereview.chromium.org/75...>
> >

[darin (slow to review), 2011-07-29 22:06:07]

On Fri, Jul 29, 2011 at 2:12 PM, <rvargas@chromium.org> wrote:

> Yes it adds overhead to think about whether the class or method is intended
> for
> consumers or not, but it is way less overhead than having to scope internal
> classes into a namespace.
>

Right, but there is a long term benefit to putting internal stuff in an
internal namespace.



>
> The lack of support from the build system is an issue, but once a class has
> been
> marked with XX_TEST is also relatively obvious that is not intended to be
> used
> for things that are not tests. On the other hand, stuff clearly marked with
> ::internal in base ended up being used by code from chrome so it is not
> like any
> system is developer proof. This is one of those things that become more
> clear as
> more people gets familiar with code that uses the annotation... with time
> (think
> OVERRIDE).
>

It's very unsatisfying to hear that people used base::internal:: stuff
outside of base/.  It should be a very obvious clue that you are doing
something wrong!



>
> I think that part of going though this exercise is to get closer to better
> interfaces between modules, not just to export everything and be done with
> it.
> At least that was the reasoning behind naming this _API, to start conveying
> the
> sense that this is the public interface of the module.
>

I think the use of an internal:: namespace goes a lot further toward this
goal.  Anything outside of that is fair game to use externally.  Anything
inside the internal namespace should not be used externally.

In contrast the xx_API is not visible at the callsites, so it is not
re-inforced when people use a public class.




>
> I would consider xx_API applied to internal stuff to be actually more
> confusing,
> even if there is a namespace involved.


I don't know... I often hear people refer to "private APIs"... internal:: +
xx_API sounds like a way of describing a private API.

I'm starting to think that xx_API is probably not a great choice because of
this problem.  It seems like we should rename it to xx_EXPORT.  The internal
namespace is the better way to hide internal details!

-Darin



>
>
> On 2011/07/29 20:41:38, darin wrote:
>
>> We could do that, but I'm not sure I see the value.  It seems to add
>> maintenance overhead if you have to decide whether an exported function is
>> exported only for a test.  There is also no help from the build system to
>> ensure that developers only use "functions exported for tests" in tests.
>>  They could easily make use of those functions in production and the
>> system
>> wouldn't complain.
>>
>
>  -Darin
>>
>
>
>
>  On Fri, Jul 29, 2011 at 1:12 PM, <mailto:rvargas@chromium.org> wrote:
>>
>
>  > Looks like we need MEDIA_TEST for this one?
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/7523051/%3Chttp://coderevi**
> ew.chromium.org/7523051/ <http://codereview.chromium.org/7523051/>>
>
>  >
>>
>
>
>
>
http://codereview.chromium.**org/7523051/<http://codereview.chromium.org/7523...
>

[darin (slow to review), 2011-07-29 22:23:24]

I posted to chromium-dev to discuss this further.  Perhaps others will want
to weigh in.
-Darin

On Fri, Jul 29, 2011 at 3:05 PM, Darin Fisher <darin@chromium.org> wrote:

> On Fri, Jul 29, 2011 at 2:12 PM, <rvargas@chromium.org> wrote:
>
>> Yes it adds overhead to think about whether the class or method is
>> intended for
>> consumers or not, but it is way less overhead than having to scope
>> internal
>> classes into a namespace.
>>
>
> Right, but there is a long term benefit to putting internal stuff in an
> internal namespace.
>
>
>
>>
>> The lack of support from the build system is an issue, but once a class
>> has been
>> marked with XX_TEST is also relatively obvious that is not intended to be
>> used
>> for things that are not tests. On the other hand, stuff clearly marked
>> with
>> ::internal in base ended up being used by code from chrome so it is not
>> like any
>> system is developer proof. This is one of those things that become more
>> clear as
>> more people gets familiar with code that uses the annotation... with time
>> (think
>> OVERRIDE).
>>
>
> It's very unsatisfying to hear that people used base::internal:: stuff
> outside of base/.  It should be a very obvious clue that you are doing
> something wrong!
>
>
>
>>
>> I think that part of going though this exercise is to get closer to better
>> interfaces between modules, not just to export everything and be done with
>> it.
>> At least that was the reasoning behind naming this _API, to start
>> conveying the
>> sense that this is the public interface of the module.
>>
>
> I think the use of an internal:: namespace goes a lot further toward this
> goal.  Anything outside of that is fair game to use externally.  Anything
> inside the internal namespace should not be used externally.
>
> In contrast the xx_API is not visible at the callsites, so it is not
> re-inforced when people use a public class.
>
>
>
>
>>
>> I would consider xx_API applied to internal stuff to be actually more
>> confusing,
>> even if there is a namespace involved.
>
>
> I don't know... I often hear people refer to "private APIs"... internal:: +
> xx_API sounds like a way of describing a private API.
>
> I'm starting to think that xx_API is probably not a great choice because of
> this problem.  It seems like we should rename it to xx_EXPORT.  The internal
> namespace is the better way to hide internal details!
>
> -Darin
>
>
>
>>
>>
>> On 2011/07/29 20:41:38, darin wrote:
>>
>>> We could do that, but I'm not sure I see the value.  It seems to add
>>> maintenance overhead if you have to decide whether an exported function
>>> is
>>> exported only for a test.  There is also no help from the build system to
>>> ensure that developers only use "functions exported for tests" in tests.
>>>  They could easily make use of those functions in production and the
>>> system
>>> wouldn't complain.
>>>
>>
>>  -Darin
>>>
>>
>>
>>
>>  On Fri, Jul 29, 2011 at 1:12 PM, <mailto:rvargas@chromium.org> wrote:
>>>
>>
>>  > Looks like we need MEDIA_TEST for this one?
>>> >
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.****org/7523051/%3Chttp://coderevi**
>> ew.chromium.org/7523051/ <http://codereview.chromium.org/7523051/>>
>>
>>  >
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/7523051/<http://codereview.chromium.org/7523...
>>
>
>

[darin (slow to review), 2011-08-03 23:38:54]

PTAL

Now with MEDIA_API renamed to MEDIA_EXPORT (and media_api.h renamed to
media_export.h).

I plan to go back and make similar changes for other modules.  I wanted to get
this out so you could see what it looks like.

[rvargas (doing something else), 2011-08-04 18:51:47]

LGTM

http://codereview.chromium.org/7523051/diff/14001/media/base/media_export.h
File media/base/media_export.h (right):

http://codereview.chromium.org/7523051/diff/14001/media/base/media_export.h#n...
media/base/media_export.h:9: // Defines MEDIA_EXPORT so that funtionality
implemented by the Media module
typo: funtionality

http://codereview.chromium.org/7523051/diff/14001/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7523051/diff/14001/media/media.gyp#newcode111
media/media.gyp:111: 'base/media_api.h',
media_export.h