use MEDIA_BLINK_EXPORT in media/blink

media/base/media_client.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MEDIA_BASE_MEDIA_CLIENT_H_
 #define MEDIA_BASE_MEDIA_CLIENT_H_
 
 #include <string>
 #include <vector>
 
 #include "media/base/key_system_info.h"
 #include "media/base/media_export.h"
 #include "url/gurl.h"
 
 namespace media {
 
 class MediaClient;
 
 // Setter for the client. If a customized client is needed, it should be set
 // early, before the client could possibly be used.
 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
 
 struct MEDIA_EXPORT KeySystemInfoForUMA {
   KeySystemInfoForUMA(const std::string& key_system,
                       const std::string& key_system_name_for_uma,
                       bool reports_key_system_support_to_uma);
   ~KeySystemInfoForUMA();
 
@@ skipping 33 matching lines @@
       std::vector<KeySystemInfo>* key_systems_info) = 0;
 
   // Records a domain and registry of a url to a Rappor privacy-preserving
   // metric. See: https://www.chromium.org/developers/design-documents/rappor
   virtual void RecordRapporURL(const std::string& metric, const GURL& url) = 0;
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_MEDIA_CLIENT_H_