use MEDIA_BLINK_EXPORT in media/blink

media/base/media_client.h

Index: media/base/media_client.h
diff --git a/media/base/media_client.h b/media/base/media_client.h
index 4b6bef5794b994f75d2298c925a1802f365d382d..dc8da49218fe2e8bedfe1bb559463d7412dca980 100644
--- a/media/base/media_client.h
+++ b/media/base/media_client.h
@@ -21,7 +21,7 @@ class MediaClient;
 MEDIA_EXPORT void SetMediaClient(MediaClient* media_client);
 
 // Media's embedder API should only be used by media.
-#if defined(MEDIA_IMPLEMENTATION)
+#if defined(MEDIA_IMPLEMENTATION) || defined(MEDIA_BLINK_IMPLEMENTATION)

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

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:19:40]

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:01]

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:18]

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.

[hubbe, 2015/11/06 23:52:11]

Acknowledged.

 // Getter for the client. Returns NULL if no customized client is needed.
 MEDIA_EXPORT MediaClient* GetMediaClient();
 #endif