Move ContentDecryptionModule* to platform/

Move ContentDecryptionModule* to platform/

BUG=297477
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161177

[abarth-chromium, 2013-10-13 15:49:19]

LGTM

Really, the clients of this code should just use the platform API directly
instead of going through these thunk classes.

[commit-bot: I haz the power, 2013-10-13 15:49:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/26975004/1

[commit-bot: I haz the power, 2013-10-13 16:36:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pilgrim_google, 2013-10-14 00:46:10]

On 2013/10/13 16:36:17, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on win_layout.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

This is a real link error that I have not seen before and don't understand.

[abarth-chromium, 2013-10-14 00:50:11]

On 2013/10/14 00:46:10, pilgrim_google wrote:
> This is a real link error that I have not seen before and don't understand.

You probably need to mark the WebKit::WebContentDecryptionModuleSession::Client
class as exported.  In the Windows build, you get that error when you try to
export a class that inherits from a non-exported base class.

[pilgrim_google, 2013-10-14 00:56:17]

On 2013/10/14 00:50:11, abarth wrote:
> On 2013/10/14 00:46:10, pilgrim_google wrote:
> > This is a real link error that I have not seen before and don't understand.
> 
> You probably need to mark the
WebKit::WebContentDecryptionModuleSession::Client
> class as exported.  In the Windows build, you get that error when you try to
> export a class that inherits from a non-exported base class.

BLINK_EXPORT, BLINK_PLATFORM_EXPORT, or BLINK_COMMON_EXPORT?

[abarth-chromium, 2013-10-14 01:14:05]

On 2013/10/14 00:56:17, pilgrim_google wrote:
> BLINK_EXPORT, BLINK_PLATFORM_EXPORT, or BLINK_COMMON_EXPORT?

The way you tell is that you look at where the
WebContentDecryptionModuleSession.cpp file is compiled.  If it's compiled as
part of blink_platform.dll, then you should use BLINK_PLATFORM_EXPORT.  If it's
built as part of blink_common.dll (rare!), then BLINK_COMMON_EXPORT.  Otherwise,
BLINK_EXPORT (which means webkit.dll).

Now, the trouble is that there isn't a WebContentDecryptionModuleSession.cpp,
which means there's no DLL from which to export the symbol.  We'll need to
create a WebContentDecryptionModuleSession.cpp and move
WebContentDecryptionModuleSession::Client::~Client into that CPP file.  We might
as well move
WebContentDecryptionModuleSession::~WebContentDecryptionModuleSession there as
well.  Given that this header is in public/platform, we should put the CPP file
in Source/platform/exported.  That will mean it ends up in blink_platform.dll,
and we should use BLINK_PLATFORM_EXPORT.

[pilgrim_google, 2013-10-14 15:19:31]

On 2013/10/14 01:14:05, abarth wrote:
> Now, the trouble is that there isn't a WebContentDecryptionModuleSession.cpp,
> which means there's no DLL from which to export the symbol.  We'll need to
> create a WebContentDecryptionModuleSession.cpp and move
> WebContentDecryptionModuleSession::Client::~Client into that CPP file.  We
might
> as well move
> WebContentDecryptionModuleSession::~WebContentDecryptionModuleSession there as
> well.  Given that this header is in public/platform, we should put the CPP
file
> in Source/platform/exported.  That will mean it ends up in blink_platform.dll,
> and we should use BLINK_PLATFORM_EXPORT.

Please re-review the latest patch to see if I did all this correctly.

[abarth-chromium, 2013-10-14 15:26:48]

lgtm

[commit-bot: I haz the power, 2013-10-14 15:26:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/26975004/18001

[commit-bot: I haz the power, 2013-10-14 15:55:22]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-14 18:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/26975004/36001

[jamesr1, 2013-10-14 19:05:34]

This is silly. The warning is just a warning that does not apply to this
situation. Just suppress the warning either here with a pragma or globally
for the blink api in gyp.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-10-14 20:09:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pilgrim_google, 2013-10-15 13:40:25]

On 2013/10/14 19:05:34, jamesr1 wrote:
> This is silly. The warning is just a warning that does not apply to this
> situation. Just suppress the warning either here with a pragma or globally
> for the blink api in gyp.

I don't understand this feedback at all.

[Stephen Chennney, 2013-10-15 13:46:04]

On 2013/10/15 13:40:25, pilgrim_google wrote:
> On 2013/10/14 19:05:34, jamesr1 wrote:
> > This is silly. The warning is just a warning that does not apply to this
> > situation. Just suppress the warning either here with a pragma or globally
> > for the blink api in gyp.
> 
> I don't understand this feedback at all.

I'm not sure what James is referring to but the error is a link error due to a
missing export on Windows. Click through the commit-bots link to see the output
from the Win build.

I happy to have you joining me in Windows link error hell.

[jamesr, 2013-10-15 17:45:35]

On 2013/10/15 13:40:25, pilgrim_google wrote:
> On 2013/10/14 19:05:34, jamesr1 wrote:
> > This is silly. The warning is just a warning that does not apply to this
> > situation. Just suppress the warning either here with a pragma or globally
> > for the blink api in gyp.
> 
> I don't understand this feedback at all.

The link error from comment #5 was this:

e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\drm\contentdecryptionmodulesession.h(60)
: warning C4275: non dll-interface class
'WebKit::WebContentDecryptionModuleSession::Client' used as base for
dll-interface class 'WebCore::ContentDecryptionModuleSession'


Here, dll-interface means a type marked with a dllexport declspec.  The warning
here is that WebKit::WebContentDecryptionModuleSession::Client is not marked as
exported but WebCore::ContentDecryptionModuleSession is, which generates warning
C4275.  Since we treat all warnings as errors, this is a compile error.  You can
read about this warning on MSDN - it's there because if the base class has
static data members the behavior of those members in subclasses is very
surprising. In this case (and in pretty much every single case I've ever seen in
Chromium/blink) the base class doesn't have any statics so the warning is not
useful.

In chromium, we have a macro NON_EXPORTED_BASE() that we use very liberally to
locally suppress this warning.  Adding the annotation is 99% the right fix to
this.  Adam suggested you go through a number of contortions to make
WK::WCDMS::Client be exported, but I think that's a ridiculously complicated
solution to a bogus warning.  You should go back to the earlier patch and either
suppress the warning (see base/compiler.h in chromium to see how
NON_EXPORTED_BASE works) or we should suppress this warning for all blink public
API types.

[pilgrim_google, 2013-10-21 15:05:50]

Please re-review.

[jamesr, 2013-10-21 18:20:08]

did you investigate suppressing this warning completely?

https://codereview.chromium.org/26975004/diff/54001/Source/platform/PlatformE...
File Source/platform/PlatformExport.h (right):

https://codereview.chromium.org/26975004/diff/54001/Source/platform/PlatformE...
Source/platform/PlatformExport.h:54: #define COMPILER_MSVC 1
if you're setting this just to use in the macro below, #undef it before the end
of the header.  I'm not really sure why you need this extra macro, though -
could you explain?

https://codereview.chromium.org/26975004/diff/54001/Source/platform/PlatformE...
Source/platform/PlatformExport.h:58: #define NON_EXPORTED_BASE(code)
MSVC_SUPPRESS_WARNING(4275)     \
where does the definition of MSVC_SUPPRESS_WARNING() come from?

the comments in base/compiler_specific.h are really useful

[pilgrim_google, 2013-10-22 13:55:02]

On 2013/10/21 18:20:08, jamesr wrote:
> did you investigate suppressing this warning completely?
> 
>
https://codereview.chromium.org/26975004/diff/54001/Source/platform/PlatformE...
> File Source/platform/PlatformExport.h (right):
> 
>
https://codereview.chromium.org/26975004/diff/54001/Source/platform/PlatformE...
> Source/platform/PlatformExport.h:54: #define COMPILER_MSVC 1
> if you're setting this just to use in the macro below, #undef it before the
end
> of the header.  I'm not really sure why you need this extra macro, though -
> could you explain?
> 
>
https://codereview.chromium.org/26975004/diff/54001/Source/platform/PlatformE...
> Source/platform/PlatformExport.h:58: #define NON_EXPORTED_BASE(code)
> MSVC_SUPPRESS_WARNING(4275)     \
> where does the definition of MSVC_SUPPRESS_WARNING() come from?
> 
> the comments in base/compiler_specific.h are really useful

OK, I think I fixed up the macro definition now. Sorry for the noise.

However, I am not comfortable being responsible for suppressing a compiler
warning unconditionally. I'm much more comfortable with the conservative
approach of requiring an explicit macro in places we need it.

[commit-bot: I haz the power, 2013-10-22 18:28:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/26975004/174001

[commit-bot: I haz the power, 2013-10-22 19:23:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jamesr, 2013-10-22 19:52:02]

Changing less is easier, but it makes the code globally worse to have little
duplications and complexities add up over time.

https://codereview.chromium.org/26975004/diff/174001/Source/platform/Platform...
File Source/platform/PlatformExport.h (right):

https://codereview.chromium.org/26975004/diff/174001/Source/platform/Platform...
Source/platform/PlatformExport.h:53: #if defined(_MSC_VER)
not lgtm without an explanatory comment.

[pilgrim_google, 2013-10-25 18:25:45]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-25 18:25:46]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-25 18:25:49]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-25 18:25:50]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-25 18:26:10]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-25 18:26:12]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-25 18:26:14]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-25 18:26:15]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-25 18:26:39]

rebase, suppress compiler warnings unconditionally, add explanatory comments

[pilgrim_google, 2013-10-28 14:29:32]

rebase

[pilgrim_google, 2013-10-28 14:29:45]

Please re-review.

[pilgrim_google, 2013-10-31 14:14:13]

rebase

[commit-bot: I haz the power, 2013-10-31 14:14:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/26975004/524001

[commit-bot: I haz the power, 2013-10-31 15:00:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-31 15:11:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/26975004/654001

[commit-bot: I haz the power, 2013-10-31 15:57:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pilgrim_google, 2013-10-31 17:37:35]

On 2013/10/31 15:57:07, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on win_layout.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

I give up. I do not know how to fix this.

[abarth-chromium, 2013-10-31 18:42:49]

https://codereview.chromium.org/26975004/diff/654001/public/platform/WebConte...
File public/platform/WebContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/26975004/diff/654001/public/platform/WebConte...
public/platform/WebContentDecryptionModuleSession.h:43: class Client {
It's complaining that you haven't put a BLINK_PLATFORM_EXPORT macro on this
line.

[pilgrim_google, 2013-11-01 13:31:00]

one more macro

[commit-bot: I haz the power, 2013-11-01 13:31:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/26975004/774001

[commit-bot: I haz the power, 2013-11-01 14:09:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pilgrim_google, 2013-11-01 15:46:57]

add WTF_MAKE_NONCOPYABLE because reasons

[commit-bot: I haz the power, 2013-11-01 15:47:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/26975004/924001

[commit-bot: I haz the power, 2013-11-01 17:10:51]

Change committed as 161177