Fix the breakage on Windows.

Fix the breakage on Windows.

Setting dllimport/dllexport on types is not allowed, of course. For more context, please, read https://crbug.com/609564#c40 and below.

BUG=609564
R=pcc@chromium.org, xhwang@chromium.org

Committed: https://chromium.googlesource.com/chromium/cdm/+/245af7782c9f54d776722a2c7b53372ee040e5fc

[krasin1, 2016-05-11 00:52:58]

krasin@chromium.org changed reviewers:
+ ddorwin@chromium.org, pcc@chromium.org, xhwang@chromium.org

[krasin1, 2016-05-11 00:52:59]

This fix is verified to work on Windows for MSVC and Clang on media_unittests /
x86-64.

Please, take a look.

[pcc1, 2016-05-11 00:57:41]

https://codereview.chromium.org/1964083003/diff/20001/content_decryption_modu...
File content_decryption_module.h (right):

https://codereview.chromium.org/1964083003/diff/20001/content_decryption_modu...
content_decryption_module.h:23: #define CDM_CLASS_API
__attribute__((visibility("default")))
I don't think this will have any effect. It changes the ELF visibility, which
isn't a thing on Windows. See also
https://bugs.chromium.org/p/chromium/issues/detail?id=609564#c45

[krasin1, 2016-05-11 01:09:30]

Description was changed from

==========
Fix the breakage on Windows.

Setting dllimport/dllexport on types is not allowed, of course.

BUG=609564
==========

to

==========
Fix the breakage on Windows.

Setting dllimport/dllexport on types is not allowed, of course. For more
context, please, read https://crbug.com/609564#c40 and below.

BUG=609564
==========

[krasin1, 2016-05-11 01:09:47]

https://codereview.chromium.org/1964083003/diff/20001/content_decryption_modu...
File content_decryption_module.h (right):

https://codereview.chromium.org/1964083003/diff/20001/content_decryption_modu...
content_decryption_module.h:23: #define CDM_CLASS_API
__attribute__((visibility("default")))
On 2016/05/11 00:57:41, pcc1 wrote:
> I don't think this will have any effect. It changes the ELF visibility, which
> isn't a thing on Windows. See also
> https://bugs.chromium.org/p/chromium/issues/detail?id=609564#c45

Done.

[pcc1, 2016-05-11 01:12:18]

lgtm

[krasin, 2016-05-11 01:13:12]

David, Xiaohan,

may I have your blessing on the fix? Sorry for not getting it right from the
first try.

krasin

[xhwang, 2016-05-12 06:54:30]

I don't fully understand the details. But from the bug discussion this rs lgtm %
one request to add documentation. 

We also need to test that chrome built with this "new" interface is still
working with existing CDMs using the "old" interface.

https://chromiumcodereview.appspot.com/1964083003/diff/40001/content_decrypti...
File content_decryption_module.h (right):

https://chromiumcodereview.appspot.com/1964083003/diff/40001/content_decrypti...
content_decryption_module.h:37: #endif  // defined(WIN32)
Could you summarize what's discussed in the bug and put some comments here.
Otherwise it'll be really hard to understand why we are doing all of these.
Thanks!

[krasin, 2016-05-16 19:02:17]

Hi there,

sorry for a delay: I had a couple of unexpected days off. Now, I am back to
work, and just finished testing it on Linux.
In particular, I did the following:

1. Synced an official checkout to master, built media_unittests with
gn gen out/gn-off '--args=is_chrome_branded=true is_official_build=true
is_debug=false' --check

2. Ran the tests (they pass)

3. Patched the CL and ran gn again:
gn gen out/gn-patch '--args=is_chrome_branded=true is_official_build=true
is_debug=false' --check

4. Ran the tests (they pass).
5. Then deleted libclearkeycdm.so and ran the tests (many of them failed, as
expected).
6. Copied libclearcdm.so from out/gn-off to out/gn-patch and ran the tests. They
passed.
7. Did the same thing with LTO-ed build:
gn gen out/gn-lto '--args=is_chrome_branded=true is_official_build=true
is_debug=false allow_posix_link_time_opt=true' --check

It passed -> failed -> passed, as expected.

Now, proceeding with testing on Mac.

[krasin, 2016-05-16 21:23:14]

Tested on Mac with GYP and empty GYP_DEFINES. Otherwise, the procedure was the
same (steps 1..6). I didn't test with LTO on Linux, as it's not something we
build now on any of the bots.

It was all good.

I will skip this kind of testing with Windows / Clang, as we don't ship
Clang-built binaries on Windows.

[krasin, 2016-05-16 21:40:10]

krasin@google.com changed reviewers:
+ krasin@google.com

[krasin, 2016-05-16 21:40:10]

All done.

I am running the final checks, and will submit the CL soon. Hopefully, the roll
deps CL will come green after trybots run.

https://codereview.chromium.org/1964083003/diff/40001/content_decryption_modu...
File content_decryption_module.h (right):

https://codereview.chromium.org/1964083003/diff/40001/content_decryption_modu...
content_decryption_module.h:37: #endif  // defined(WIN32)
On 2016/05/12 06:54:30, xhwang wrote:
> Could you summarize what's discussed in the bug and put some comments here.
> Otherwise it'll be really hard to understand why we are doing all of these.
> Thanks!

Done.

[krasin1, 2016-05-16 22:02:30]

Description was changed from

==========
Fix the breakage on Windows.

Setting dllimport/dllexport on types is not allowed, of course. For more
context, please, read https://crbug.com/609564#c40 and below.

BUG=609564
==========

to

==========
Fix the breakage on Windows.

Setting dllimport/dllexport on types is not allowed, of course. For more
context, please, read https://crbug.com/609564#c40 and below.

BUG=609564
R=pcc@chromium.org, xhwang@chromium.org

Committed:
https://chromium.googlesource.com/chromium/cdm/+/245af7782c9f54d776722a2c7b53...
==========

[krasin1, 2016-05-16 22:02:32]

Committed patchset #5 (id:80001) manually as
245af7782c9f54d776722a2c7b53372ee040e5fc (presubmit successful).