use MEDIA_BLINK_EXPORT in media/blink

use MEDIA_BLINK_EXPORT in media/blink

media/blink is a separate module from media/, and needs it's own export macro.

Currently everything works, but mostly because of happy accidents. When adding new code and tests to media/blink I discovered that some symbols were not available on component builds because they were imported rather than exported. This fixes that. Unfortunately I do not remember which symbols I had a problem with.

Committed: https://crrev.com/08fb799636968bc14629b4c24e0ce5834dac0f71
Cr-Commit-Position: refs/heads/master@{#358485}

[hubbe, 2015-11-06 21:50:38]

hubbe@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2015-11-06 22:04:23]

ddorwin@chromium.org changed reviewers:
+ xhwang@chromium.org

[ddorwin, 2015-11-06 22:04:23]

+xhwang for thoughts on MediaClient and media/blink.

Please provide more on the motivation in the description. Is there a bug? Does
some configuration not work? IOW, if the code works fine today, why do we need
this?

https://codereview.chromium.org/1430063003/diff/1/media/base/media_client.h
File media/base/media_client.h (right):

https://codereview.chromium.org/1430063003/diff/1/media/base/media_client.h#n...
media/base/media_client.h:24: #if defined(MEDIA_IMPLEMENTATION) ||
defined(MEDIA_BLINK_IMPLEMENTATION)
This feels wrong. First, two different modules do this. If you think about it as
in terms of implementation or user, that makes sense, but that leads to: Second,
either it is implementation or not - why do we have two separate defines and
would we keep adding more here?

Should blink also define MEDIA_IMPLEMENTATION or should we otherwise have a
single value to check here?

Should media/blink be completely separate and have to obtain the client in some
other way?

[hubbe, 2015-11-06 22:11:33]

Description was changed from

==========
use MEDIA_BLINK_EXPORT in media/blink

media/blink is a separate module from media/, and needs it's own export macro.
==========

to

==========
use MEDIA_BLINK_EXPORT in media/blink

media/blink is a separate module from media/, and needs it's own export macro.

Currently everything works, but mostly because of happy accidents, I have code
in other CLs that I could not make work properly without fixing this first.
==========

[hubbe, 2015-11-06 22:19:40]

CL description updated...

https://codereview.chromium.org/1430063003/diff/1/media/base/media_client.h
File media/base/media_client.h (right):

https://codereview.chromium.org/1430063003/diff/1/media/base/media_client.h#n...
media/base/media_client.h:24: #if defined(MEDIA_IMPLEMENTATION) ||
defined(MEDIA_BLINK_IMPLEMENTATION)
On 2015/11/06 22:04:23, ddorwin wrote:
> This feels wrong. First, two different modules do this. If you think about it
as
> in terms of implementation or user, that makes sense, but that leads to:
Second,
> either it is implementation or not - why do we have two separate defines and
> would we keep adding more here?
> 
> Should blink also define MEDIA_IMPLEMENTATION or should we otherwise have a
> single value to check here?
> 
> Should media/blink be completely separate and have to obtain the client in
some
> other way?

There are actually several things wrong here IMHO.
MEDIA_IMPLEMENTATION is tied to the export/import semantics, which means it
should only be used within the media module. Other modules, such as media/blink/
are still conceptually part of the media code, but are not part of the media
module because of dependencies on blink. What I think the author wanted when
this line was written was to say "this should be only be visible to media/*".
That's a weird thing to do, but if we want that behavior, then perhaps all
media/* should have a define for that, but that define must be separate from the
one that controls import/export from the media module.

Alternatively, we could make this a private static class method and add the
callers as friends.

What do you prefer?

[ddorwin, 2015-11-06 22:49:02]

https://codereview.chromium.org/1430063003/diff/1/media/base/media_client.h
File media/base/media_client.h (right):

https://codereview.chromium.org/1430063003/diff/1/media/base/media_client.h#n...
media/base/media_client.h:24: #if defined(MEDIA_IMPLEMENTATION) ||
defined(MEDIA_BLINK_IMPLEMENTATION)
On 2015/11/06 22:19:40, hubbe wrote:
> On 2015/11/06 22:04:23, ddorwin wrote:
> > This feels wrong. First, two different modules do this. If you think about
it
> as
> > in terms of implementation or user, that makes sense, but that leads to:
> Second,
> > either it is implementation or not - why do we have two separate defines and
> > would we keep adding more here?
> > 
> > Should blink also define MEDIA_IMPLEMENTATION or should we otherwise have a
> > single value to check here?
> > 
> > Should media/blink be completely separate and have to obtain the client in
> some
> > other way?
> 
> There are actually several things wrong here IMHO.
> MEDIA_IMPLEMENTATION is tied to the export/import semantics, which means it
> should only be used within the media module. Other modules, such as
media/blink/
> are still conceptually part of the media code, but are not part of the media
> module because of dependencies on blink. What I think the author wanted when
> this line was written was to say "this should be only be visible to media/*".
> That's a weird thing to do, but if we want that behavior, then perhaps all
> media/* should have a define for that, but that define must be separate from
the
> one that controls import/export from the media module.
> 
> Alternatively, we could make this a private static class method and add the
> callers as friends.
> 
> What do you prefer?
> 
> 

I don't think we want to friend all call sites - it's messy and requires
maintenance. I'll defer to xhwang on the other questions.

[xhwang, 2015-11-06 23:43:19]

Actually we already have CC_BLINK_EXPORT and GPU_BLINK_EXPORT. So this change
looks good % nits.

But please elaborate in the CL description why we are doing this now. What
exactly was the issue you saw. For example, importing and exporting could be
reversed when including media/base headers from media/blink since the latter is
still a MEDIA_IMPLEMENTATION...

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/base/media_cli...
File media/base/media_client.h (right):

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/base/media_cli...
media/base/media_client.h:24: #if defined(MEDIA_IMPLEMENTATION) ||
defined(MEDIA_BLINK_IMPLEMENTATION)
On 2015/11/06 22:49:01, ddorwin wrote:
> On 2015/11/06 22:19:40, hubbe wrote:
> > On 2015/11/06 22:04:23, ddorwin wrote:
> > > This feels wrong. First, two different modules do this. If you think about
> it
> > as
> > > in terms of implementation or user, that makes sense, but that leads to:
> > Second,
> > > either it is implementation or not - why do we have two separate defines
and
> > > would we keep adding more here?
> > > 
> > > Should blink also define MEDIA_IMPLEMENTATION or should we otherwise have
a
> > > single value to check here?
> > > 
> > > Should media/blink be completely separate and have to obtain the client in
> > some
> > > other way?
> > 
> > There are actually several things wrong here IMHO.
> > MEDIA_IMPLEMENTATION is tied to the export/import semantics, which means it
> > should only be used within the media module. Other modules, such as
> media/blink/
> > are still conceptually part of the media code, but are not part of the media
> > module because of dependencies on blink. What I think the author wanted when
> > this line was written was to say "this should be only be visible to
media/*".
> > That's a weird thing to do, but if we want that behavior, then perhaps all
> > media/* should have a define for that, but that define must be separate from
> the
> > one that controls import/export from the media module.
> > 
> > Alternatively, we could make this a private static class method and add the
> > callers as friends.
> > 
> > What do you prefer?
> > 
> > 
> 
> I don't think we want to friend all call sites - it's messy and requires
> maintenance. I'll defer to xhwang on the other questions.

GetMediaClient was added to be used within the media/ code. It was  exported
because we have multiple media modules (media, media_blink), and
GetMediaClient() is defined in the media module.

But since it's exported, suddenly any code that depends on media can use it,
which doesn't make sense. Therefore we have this line to protect unintended use.
This is actually following the content_client model.

Now with a separate export macro, this change sgtm. If we ever want to split
media into more modules (which I highly doubt), we can define a macro for all
media modules and use it here. But now it seems overkill.

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/BUILD.gn
File media/blink/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/BUILD.gn...
media/blink/BUILD.gn:28: sources = [
Please add media_blink_export.h to the gyp/gn file.

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/active_l...
File media/blink/active_loader.h (right):

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/active_l...
media/blink/active_loader.h:34: friend class MultibufferDataSourceTest;
why do you need this change now?

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/media_bl...
File media/blink/media_blink_export.h (right):

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/media_bl...
media/blink/media_blink_export.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
s/Copyright (c) 2012/Copyright 2015/

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/media_bl...
media/blink/media_blink_export.h:5: #ifndef MEDIA_BASE_MEDIA_BLINK_EXPORT_H_
here and below: s/BASE/BLINK

[hubbe, 2015-11-06 23:52:12]

PTAL

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/base/media_cli...
File media/base/media_client.h (right):

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/base/media_cli...
media/base/media_client.h:24: #if defined(MEDIA_IMPLEMENTATION) ||
defined(MEDIA_BLINK_IMPLEMENTATION)
On 2015/11/06 23:43:18, xhwang wrote:
> On 2015/11/06 22:49:01, ddorwin wrote:
> > On 2015/11/06 22:19:40, hubbe wrote:
> > > On 2015/11/06 22:04:23, ddorwin wrote:
> > > > This feels wrong. First, two different modules do this. If you think
about
> > it
> > > as
> > > > in terms of implementation or user, that makes sense, but that leads to:
> > > Second,
> > > > either it is implementation or not - why do we have two separate defines
> and
> > > > would we keep adding more here?
> > > > 
> > > > Should blink also define MEDIA_IMPLEMENTATION or should we otherwise
have
> a
> > > > single value to check here?
> > > > 
> > > > Should media/blink be completely separate and have to obtain the client
in
> > > some
> > > > other way?
> > > 
> > > There are actually several things wrong here IMHO.
> > > MEDIA_IMPLEMENTATION is tied to the export/import semantics, which means
it
> > > should only be used within the media module. Other modules, such as
> > media/blink/
> > > are still conceptually part of the media code, but are not part of the
media
> > > module because of dependencies on blink. What I think the author wanted
when
> > > this line was written was to say "this should be only be visible to
> media/*".
> > > That's a weird thing to do, but if we want that behavior, then perhaps all
> > > media/* should have a define for that, but that define must be separate
from
> > the
> > > one that controls import/export from the media module.
> > > 
> > > Alternatively, we could make this a private static class method and add
the
> > > callers as friends.
> > > 
> > > What do you prefer?
> > > 
> > > 
> > 
> > I don't think we want to friend all call sites - it's messy and requires
> > maintenance. I'll defer to xhwang on the other questions.
> 
> GetMediaClient was added to be used within the media/ code. It was  exported
> because we have multiple media modules (media, media_blink), and
> GetMediaClient() is defined in the media module.
> 
> But since it's exported, suddenly any code that depends on media can use it,
> which doesn't make sense. Therefore we have this line to protect unintended
use.
> This is actually following the content_client model.
> 
> Now with a separate export macro, this change sgtm. If we ever want to split
> media into more modules (which I highly doubt), we can define a macro for all
> media modules and use it here. But now it seems overkill.

Acknowledged.

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/BUILD.gn
File media/blink/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/BUILD.gn...
media/blink/BUILD.gn:28: sources = [
On 2015/11/06 23:43:18, xhwang wrote:
> Please add media_blink_export.h to the gyp/gn file.

Done.

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/active_l...
File media/blink/active_loader.h (right):

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/active_l...
media/blink/active_loader.h:34: friend class MultibufferDataSourceTest;
On 2015/11/06 23:43:18, xhwang wrote:
> why do you need this change now?

Oops, didn't mean for that to sneak in there. Removed.

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/media_bl...
File media/blink/media_blink_export.h (right):

https://chromiumcodereview.appspot.com/1430063003/diff/1/media/blink/media_bl...
media/blink/media_blink_export.h:5: #ifndef MEDIA_BASE_MEDIA_BLINK_EXPORT_H_
On 2015/11/06 23:43:18, xhwang wrote:
> here and below: s/BASE/BLINK

Done.

[xhwang, 2015-11-06 23:58:33]

Could you please update your CL description with some more info about the issue
you saw with the old code? I am curious about that and it'll be useful in the
future.

otherwise lgtm % one tiny nit

https://chromiumcodereview.appspot.com/1430063003/diff/20001/media/blink/medi...
File media/blink/media_blink_export.h (right):

https://chromiumcodereview.appspot.com/1430063003/diff/20001/media/blink/medi...
media/blink/media_blink_export.h:1: // Copyright (c) 2015 The Chromium Authors.
All rights reserved.
nit: I think in the new style we don't use "(c)"

[hubbe, 2015-11-07 00:17:44]

Description was changed from

==========
use MEDIA_BLINK_EXPORT in media/blink

media/blink is a separate module from media/, and needs it's own export macro.

Currently everything works, but mostly because of happy accidents, I have code
in other CLs that I could not make work properly without fixing this first.
==========

to

==========
use MEDIA_BLINK_EXPORT in media/blink

media/blink is a separate module from media/, and needs it's own export macro.

Currently everything works, but mostly because of happy accidents. When adding
new code and tests to media/blink I discovered that some symbols were not
available on component builds because they were imported rather than exported.
This fixes that. Unfortunately I do not remember which symbols I had a problem
with.
==========

[hubbe, 2015-11-07 00:18:21]

Updated the description a bit, unfortunately I do not remember the exact
symbols..

https://chromiumcodereview.appspot.com/1430063003/diff/20001/media/blink/medi...
File media/blink/media_blink_export.h (right):

https://chromiumcodereview.appspot.com/1430063003/diff/20001/media/blink/medi...
media/blink/media_blink_export.h:1: // Copyright (c) 2015 The Chromium Authors.
All rights reserved.
On 2015/11/06 23:58:33, xhwang wrote:
> nit: I think in the new style we don't use "(c)"

Done.

[hubbe, 2015-11-07 00:18:25]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2015-11-07 00:18:26]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1430063003/#ps40001
(title: "no (c)")

[commit-bot: I haz the power, 2015-11-07 00:19:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430063003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430063003/40001

[commit-bot: I haz the power, 2015-11-07 01:26:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-07 01:27:16]

Patchset 3 (id:??) landed as
https://crrev.com/08fb799636968bc14629b4c24e0ce5834dac0f71
Cr-Commit-Position: refs/heads/master@{#358485}