Replace WIDEVINE_CDM_AVAILABLE with a gyp define 'enable_widevine_cdm'.

Replace WIDEVINE_CDM_AVAILABLE with a gyp define, enable_widevine_cdm.

This CL does not replace all widevine-related conditions on
Chrome branding in gyp files, but that change should be easier now.

BUG=349182

[sandersd (OOO until July 31), 2014-12-02 22:55:32]

sandersd@chromium.org changed reviewers:
+ dalecurtis@chromium.org, ddorwin@chromium.org

[sandersd (OOO until July 31), 2014-12-02 22:55:33]

The .cc changes are purely mechanical.

dalecurtis: please review for .gn sanity.

[DaleCurtis, 2014-12-02 22:58:39]

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode2674
build/common.gypi:2674: ['enable_widevine==1', {
These are global defines which should always be reluctantly added.  Is there any
way to avoid this at global scope? Can it be limited to just media/ and
third_party/widevine easily?

[sandersd (OOO until July 31), 2014-12-02 23:04:45]

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode2674
build/common.gypi:2674: ['enable_widevine==1', {
On 2014/12/02 22:58:38, DaleCurtis wrote:
> These are global defines which should always be reluctantly added.  Is there
any
> way to avoid this at global scope? Can it be limited to just media/ and
> third_party/widevine easily?

I don't know. There is no common include that I am aware of, but I suspect we
can add includes until it's possible.

This specific define is used in chromecast, content/renderer, chrome/browser,
chrome/common, chrome/renderer.

[DaleCurtis, 2014-12-02 23:12:17]

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode2674
build/common.gypi:2674: ['enable_widevine==1', {
On 2014/12/02 23:04:44, sandersd wrote:
> On 2014/12/02 22:58:38, DaleCurtis wrote:
> > These are global defines which should always be reluctantly added.  Is there
> any
> > way to avoid this at global scope? Can it be limited to just media/ and
> > third_party/widevine easily?
> 
> I don't know. There is no common include that I am aware of, but I suspect we
> can add includes until it's possible.
> 
> This specific define is used in chromecast, content/renderer, chrome/browser,
> chrome/common, chrome/renderer.

Ah, that's unfortunate. Do all of these users depend directly on media? You can
then use dependent configs to keep these in media/ only.  See how libvpx
disabling is done across content/renderer and media/ and media/blink

https://code.google.com/p/chromium/codesearch#search/&q=MEDIA_DISABLE_LIBVPX

[lcwu1, 2014-12-02 23:23:16]

lcwu@chromium.org changed reviewers:
+ lcwu@chromium.org

[lcwu1, 2014-12-02 23:23:17]

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode1988
build/common.gypi:1988: 'enable_widevine': 0,
Drive-by comment: Could you add '%' to the gyp variable so that it can be
overridden by different platforms/implementations.

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode2674
build/common.gypi:2674: ['enable_widevine==1', {
On 2014/12/02 23:12:17, DaleCurtis wrote:
> On 2014/12/02 23:04:44, sandersd wrote:
> > On 2014/12/02 22:58:38, DaleCurtis wrote:
> > > These are global defines which should always be reluctantly added.  Is
there
> > any
> > > way to avoid this at global scope? Can it be limited to just media/ and
> > > third_party/widevine easily?
> > 
> > I don't know. There is no common include that I am aware of, but I suspect
we
> > can add includes until it's possible.
> > 
> > This specific define is used in chromecast, content/renderer,
chrome/browser,
> > chrome/common, chrome/renderer.
> 
> Ah, that's unfortunate. Do all of these users depend directly on media? You
can
> then use dependent configs to keep these in media/ only.  See how libvpx
> disabling is done across content/renderer and media/ and media/blink
> 
> https://code.google.com/p/chromium/codesearch#search/&q=MEDIA_DISABLE_LIBVPX
> 

Chromecast uses the define. We do depend on media/ directly. So as long as the
define is exposed to chromecast/, either way is fine.

[gunsch, 2014-12-02 23:29:40]

gunsch@chromium.org changed reviewers:
+ gunsch@chromium.org

[gunsch, 2014-12-02 23:29:41]

https://codereview.chromium.org/772043004/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/772043004/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:11: #include
"widevine_cdm_version.h" // In SHARED_INTERMEDIATE_DIR.
Drive-by comment: for all of the cases where WIDEVINE_CDM_AVAILABLE was checked
and now ENABLE_WIDEVINE is checked, it should be possible to remove
widevine_cdm_version.h, no?

[ddorwin, 2014-12-02 23:31:56]

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode1991
build/common.gypi:1991: # Widevine is distributed as a component for Mac and
Win.
This is only for Chrome. I guess we should make this 'branding=="Chrome" and
(...)

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode2674
build/common.gypi:2674: ['enable_widevine==1', {
On 2014/12/02 23:12:17, DaleCurtis wrote:
> On 2014/12/02 23:04:44, sandersd wrote:
> > On 2014/12/02 22:58:38, DaleCurtis wrote:
> > > These are global defines which should always be reluctantly added.  Is
there
> > any
> > > way to avoid this at global scope? Can it be limited to just media/ and
> > > third_party/widevine easily?
> > 
> > I don't know. There is no common include that I am aware of, but I suspect
we
> > can add includes until it's possible.
> > 
> > This specific define is used in chromecast, content/renderer,
chrome/browser,
> > chrome/common, chrome/renderer.
> 
> Ah, that's unfortunate. Do all of these users depend directly on media? You
can
> then use dependent configs to keep these in media/ only.  See how libvpx
> disabling is done across content/renderer and media/ and media/blink
> 
> https://code.google.com/p/chromium/codesearch#search/&q=MEDIA_DISABLE_LIBVPX
> 

Looking at the paths in this CL, there are some that do not appear to depend on
media.gyp. We could create a new target or .gyp file for those, though I'm not
sure how best to do that. How close are we to transitioning to GN? Does it make
sense do focus on doing this in GN?

https://codereview.chromium.org/772043004/diff/1/chrome/browser/component_upd...
File chrome/browser/component_updater/widevine_cdm_component_installer.cc
(right):

https://codereview.chromium.org/772043004/diff/1/chrome/browser/component_upd...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:35:
#include "third_party/widevine/cdm/widevine_cdm_common.h"
We shouldn't need this in most places - it's included by the version file
anyway. However, where we aren't using the version but do need some piece of
information, we should prefer this to the version file. (See below for why the
version files are problematic.)

https://codereview.chromium.org/772043004/diff/1/chrome/browser/component_upd...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:37:
#include "widevine_cdm_version.h"  // In SHARED_INTERMEDIATE_DIR. NOLINT
We should be able to eliminate many, if not most, of these includes and the 11
corresponding instances of a dependency on
widevine_cdm.gyp:widevine_cdm_version_h: 
https://code.google.com/p/chromium/codesearch#search/&q=widevine_cdm.gyp%3Awi...
We'll need to find the GN equivalents too.

We should also be able to remove some of the corresponding .gyp/.gn uses of
SHARED_INTERMEDIATE_DIR, though we need to be careful there are not other
dependencies. Sometimes there are comments documenting the dependency.

https://codereview.chromium.org/772043004/diff/1/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/772043004/diff/1/media/media_options.gni#newc...
media/media_options.gni:57: widevine_cdm_is_component = is_mac || is_win
ditto

https://codereview.chromium.org/772043004/diff/1/third_party/widevine/cdm/and...
File third_party/widevine/cdm/android/widevine_cdm_version.h (right):

https://codereview.chromium.org/772043004/diff/1/third_party/widevine/cdm/and...
third_party/widevine/cdm/android/widevine_cdm_version.h:9: 
Maybe we should add a comment that the CDM is provided by the platform, so there
is no version information.

[DaleCurtis, 2014-12-03 01:11:00]

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode2674
build/common.gypi:2674: ['enable_widevine==1', {
On 2014/12/02 23:31:56, ddorwin wrote:
> On 2014/12/02 23:12:17, DaleCurtis wrote:
> > On 2014/12/02 23:04:44, sandersd wrote:
> > > On 2014/12/02 22:58:38, DaleCurtis wrote:
> > > > These are global defines which should always be reluctantly added.  Is
> there
> > > any
> > > > way to avoid this at global scope? Can it be limited to just media/ and
> > > > third_party/widevine easily?
> > > 
> > > I don't know. There is no common include that I am aware of, but I suspect
> we
> > > can add includes until it's possible.
> > > 
> > > This specific define is used in chromecast, content/renderer,
> chrome/browser,
> > > chrome/common, chrome/renderer.
> > 
> > Ah, that's unfortunate. Do all of these users depend directly on media? You
> can
> > then use dependent configs to keep these in media/ only.  See how libvpx
> > disabling is done across content/renderer and media/ and media/blink
> > 
> > https://code.google.com/p/chromium/codesearch#search/&q=MEDIA_DISABLE_LIBVPX
> > 
> 
> Looking at the paths in this CL, there are some that do not appear to depend
on
> media.gyp. We could create a new target or .gyp file for those, though I'm not
> sure how best to do that. How close are we to transitioning to GN? Does it
make
> sense do focus on doing this in GN?

Which targets don't depend on media? third_party/widevine seems like it might
the only one. Of which media depends on, so perhaps it should host all these
defines? media/ can then propagate them outward to its dependents.

[ddorwin, 2014-12-03 01:29:45]

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode2674
build/common.gypi:2674: ['enable_widevine==1', {
On 2014/12/03 01:11:00, DaleCurtis wrote:
> On 2014/12/02 23:31:56, ddorwin wrote:
> > On 2014/12/02 23:12:17, DaleCurtis wrote:
> > > On 2014/12/02 23:04:44, sandersd wrote:
> > > > On 2014/12/02 22:58:38, DaleCurtis wrote:
> > > > > These are global defines which should always be reluctantly added.  Is
> > there
> > > > any
> > > > > way to avoid this at global scope? Can it be limited to just media/
and
> > > > > third_party/widevine easily?
> > > > 
> > > > I don't know. There is no common include that I am aware of, but I
suspect
> > we
> > > > can add includes until it's possible.
> > > > 
> > > > This specific define is used in chromecast, content/renderer,
> > chrome/browser,
> > > > chrome/common, chrome/renderer.
> > > 
> > > Ah, that's unfortunate. Do all of these users depend directly on media?
You
> > can
> > > then use dependent configs to keep these in media/ only.  See how libvpx
> > > disabling is done across content/renderer and media/ and media/blink
> > > 
> > >
https://code.google.com/p/chromium/codesearch#search/&q=MEDIA_DISABLE_LIBVPX
> > > 
> > 
> > Looking at the paths in this CL, there are some that do not appear to depend
> on
> > media.gyp. We could create a new target or .gyp file for those, though I'm
not
> > sure how best to do that. How close are we to transitioning to GN? Does it
> make
> > sense do focus on doing this in GN?
> 
> Which targets don't depend on media? third_party/widevine seems like it might
> the only one. Of which media depends on, so perhaps it should host all these
> defines? media/ can then propagate them outward to its dependents.

As an example, from a brief look, it was not clear that files from
chrome/common. and chrome/renderer. in this CL depend on media.gyp.

[DaleCurtis, 2014-12-04 02:45:08]

You'll need buy in from brettw@ for the build/config change, so if you're sure
there's not a dependent way to do this (there wasn't for PROPRIETARY_CODECS for
instance) please reach out to him.

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode2674
build/common.gypi:2674: ['enable_widevine==1', {
On 2014/12/03 01:29:45, ddorwin wrote:
> On 2014/12/03 01:11:00, DaleCurtis wrote:
> > On 2014/12/02 23:31:56, ddorwin wrote:
> > > On 2014/12/02 23:12:17, DaleCurtis wrote:
> > > > On 2014/12/02 23:04:44, sandersd wrote:
> > > > > On 2014/12/02 22:58:38, DaleCurtis wrote:
> > > > > > These are global defines which should always be reluctantly added. 
Is
> > > there
> > > > > any
> > > > > > way to avoid this at global scope? Can it be limited to just media/
> and
> > > > > > third_party/widevine easily?
> > > > > 
> > > > > I don't know. There is no common include that I am aware of, but I
> suspect
> > > we
> > > > > can add includes until it's possible.
> > > > > 
> > > > > This specific define is used in chromecast, content/renderer,
> > > chrome/browser,
> > > > > chrome/common, chrome/renderer.
> > > > 
> > > > Ah, that's unfortunate. Do all of these users depend directly on media?
> You
> > > can
> > > > then use dependent configs to keep these in media/ only.  See how libvpx
> > > > disabling is done across content/renderer and media/ and media/blink
> > > > 
> > > >
> https://code.google.com/p/chromium/codesearch#search/&q=MEDIA_DISABLE_LIBVPX
> > > > 
> > > 
> > > Looking at the paths in this CL, there are some that do not appear to
depend
> > on
> > > media.gyp. We could create a new target or .gyp file for those, though I'm
> not
> > > sure how best to do that. How close are we to transitioning to GN? Does it
> > make
> > > sense do focus on doing this in GN?
> > 
> > Which targets don't depend on media? third_party/widevine seems like it
might
> > the only one. Of which media depends on, so perhaps it should host all these
> > defines? media/ can then propagate them outward to its dependents.
> 
> As an example, from a brief look, it was not clear that files from
> chrome/common. and chrome/renderer. in this CL depend on media.gyp.

I see, I misread the paths, yeah I don't think a direct dependent setting would
reach those.

[sandersd (OOO until July 31), 2014-12-09 23:56:25]

Patchset #3 (id:40001) has been deleted

[sandersd (OOO until July 31), 2014-12-09 23:56:33]

Patchset #2 (id:20001) has been deleted

[sandersd (OOO until July 31), 2014-12-10 00:10:37]

Patchset #2 (id:60001) has been deleted

[sandersd (OOO until July 31), 2014-12-10 00:36:50]

Patchset #2 (id:80001) has been deleted

[sandersd (OOO until July 31), 2014-12-10 01:42:36]

Patchset #2 (id:100001) has been deleted

[sandersd (OOO until July 31), 2014-12-12 01:34:47]

Changes since first patch set:
  - Renamed enable_widevine to enable_widevine_cdm.
  - Moved args from media/media_options.gni to build/config/features.gni. This
was necessary because build/config/BUILD.gn depends on them.
  - Some conditions are updated to include defined(ENABLE_WIDEVINE_CDM), as
widevine_cdm_common.h is now wrapped in the same.

Removing dependencies on widevine_cdm_version.h will be done in separate CLs.

https://codereview.chromium.org/772043004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/1/build/common.gypi#newcode1988
build/common.gypi:1988: 'enable_widevine': 0,
On 2014/12/02 23:23:16, lcwu1 wrote:
> Drive-by comment: Could you add '%' to the gyp variable so that it can be
> overridden by different platforms/implementations.

Done.

https://codereview.chromium.org/772043004/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/772043004/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:11: #include
"widevine_cdm_version.h" // In SHARED_INTERMEDIATE_DIR.
On 2014/12/02 23:29:41, gunsch wrote:
> Drive-by comment: for all of the cases where WIDEVINE_CDM_AVAILABLE was
checked
> and now ENABLE_WIDEVINE is checked, it should be possible to remove
> widevine_cdm_version.h, no?

This is true, but my attempt resulted in a very large, very fragile CL. I'll
split up that work in separate CLs.

https://codereview.chromium.org/772043004/diff/1/third_party/widevine/cdm/and...
File third_party/widevine/cdm/android/widevine_cdm_version.h (right):

https://codereview.chromium.org/772043004/diff/1/third_party/widevine/cdm/and...
third_party/widevine/cdm/android/widevine_cdm_version.h:9: 
On 2014/12/02 23:31:56, ddorwin wrote:
> Maybe we should add a comment that the CDM is provided by the platform, so
there
> is no version information.

Done.

[DaleCurtis, 2014-12-12 23:07:28]

lgtm assuming there was no way to remove those build\config defines after all.

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi#newco...
build/common.gypi:769: # Widevine CDM is enabled by default for Chrome builds,
and is a platform
80 chars.

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
File third_party/widevine/cdm/android/widevine_cdm_version.h (right):

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/android/widevine_cdm_version.h:8: #include
"third_party/widevine/cdm/widevine_cdm_common.h"
What's the point of this file now? why not directly include the other file?

[ddorwin, 2014-12-15 18:31:06]

There will be another CL to remove all the version includes and build
dependencies, etc., right?

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi#newco...
build/common.gypi:777: # Widevine is distributed as a component for Mac and Win.
This should be under/combined with branding==chrome

(Though it's technically not reasonable on Linux or Chrome OS.)

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi#newco...
build/common.gypi:2672: 'defines': ['ENABLE_WIDEVINE_CDM'],
=1 like in the .gn file?
Same below.

https://codereview.chromium.org/772043004/diff/120001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/772043004/diff/120001/build/config/features.g...
build/config/features.gni:48: widevine_cdm_is_component = is_mac || is_win
ditto on && chrome

https://codereview.chromium.org/772043004/diff/120001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_uma.cc (right):

https://codereview.chromium.org/772043004/diff/120001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma.cc:181: #if defined(ENABLE_WIDEVINE_CDM) &&
defined(ENABLE_PEPPER_CDMS)
Minor issue. I think it was intentional that this code would be present
regardless of the WIDEVINE define. Is this necessary for this CL? Check with
xhwang.

Actually, if Pepper is enabled at all (not just CDMs), we could run this code.

nit: Reverse the checks. Pepper is the broader category here.

https://codereview.chromium.org/772043004/diff/120001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_uma_unittest.cc (right):

https://codereview.chromium.org/772043004/diff/120001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma_unittest.cc:106: #if
defined(ENABLE_WIDEVINE_CDM) && defined(ENABLE_PEPPER_CDMS)
ditto

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
File third_party/widevine/cdm/BUILD.gn (right):

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/BUILD.gn:14: # Always available on Android regardless
of branding.
Remove.

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/BUILD.gn:39: # No CDM, use the default one.
# Not enabled - use the empty file.

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
File third_party/widevine/cdm/android/widevine_cdm_version.h (right):

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/android/widevine_cdm_version.h:8: #include
"third_party/widevine/cdm/widevine_cdm_common.h"
On 2014/12/12 23:07:28, DaleCurtis wrote:
> What's the point of this file now? why not directly include the other file?

We assume that each platform has a version file.

The next CL should remove many uses of the version file, which might eliminate
the need for this file since it would never be included on Android. Is that
right, Dan?

Until then, we should add a comment that the CDM is provided by the platform, so
there
is no version information.

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:65: #if defined(OS_MACOSX) ||
defined(OS_WIN)
Why are these 4 lines back?

[sandersd (OOO until July 31), 2014-12-15 21:51:22]

> There will be another CL to remove all the version includes and build
> dependencies, etc., right?

Yes, although it will likely be several separate CLs.

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi#newco...
build/common.gypi:769: # Widevine CDM is enabled by default for Chrome builds,
and is a platform
On 2014/12/12 23:07:28, DaleCurtis wrote:
> 80 chars.

Done.

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi#newco...
build/common.gypi:777: # Widevine is distributed as a component for Mac and Win.
On 2014/12/15 18:31:06, ddorwin wrote:
> This should be under/combined with branding==chrome
> 
> (Though it's technically not reasonable on Linux or Chrome OS.)

Done.

https://codereview.chromium.org/772043004/diff/120001/build/common.gypi#newco...
build/common.gypi:2672: 'defines': ['ENABLE_WIDEVINE_CDM'],
On 2014/12/15 18:31:06, ddorwin wrote:
> =1 like in the .gn file?
> Same below.

Done.

https://codereview.chromium.org/772043004/diff/120001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/772043004/diff/120001/build/config/features.g...
build/config/features.gni:48: widevine_cdm_is_component = is_mac || is_win
On 2014/12/15 18:31:06, ddorwin wrote:
> ditto on && chrome

Done.

https://codereview.chromium.org/772043004/diff/120001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_uma.cc (right):

https://codereview.chromium.org/772043004/diff/120001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma.cc:181: #if defined(ENABLE_WIDEVINE_CDM) &&
defined(ENABLE_PEPPER_CDMS)
On 2014/12/15 18:31:06, ddorwin wrote:
> Minor issue. I think it was intentional that this code would be present
> regardless of the WIDEVINE define. Is this necessary for this CL? Check with
> xhwang.
> 
> Actually, if Pepper is enabled at all (not just CDMs), we could run this code.
> 
> nit: Reverse the checks. Pepper is the broader category here.

It can't be enabled because kWidevineCdmPluginMimeType won't be defined anymore.

https://codereview.chromium.org/772043004/diff/120001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_uma_unittest.cc (right):

https://codereview.chromium.org/772043004/diff/120001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma_unittest.cc:106: #if
defined(ENABLE_WIDEVINE_CDM) && defined(ENABLE_PEPPER_CDMS)
On 2014/12/15 18:31:06, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
File third_party/widevine/cdm/BUILD.gn (right):

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/BUILD.gn:14: # Always available on Android regardless
of branding.
On 2014/12/15 18:31:06, ddorwin wrote:
> Remove.

Done.

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/BUILD.gn:39: # No CDM, use the default one.
On 2014/12/15 18:31:06, ddorwin wrote:
> # Not enabled - use the empty file.

Done.

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
File third_party/widevine/cdm/android/widevine_cdm_version.h (right):

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/android/widevine_cdm_version.h:8: #include
"third_party/widevine/cdm/widevine_cdm_common.h"
On 2014/12/15 18:31:06, ddorwin wrote:
> On 2014/12/12 23:07:28, DaleCurtis wrote:
> > What's the point of this file now? why not directly include the other file?
> 
> We assume that each platform has a version file.
> 
> The next CL should remove many uses of the version file, which might eliminate
> the need for this file since it would never be included on Android. Is that
> right, Dan?
> 
> Until then, we should add a comment that the CDM is provided by the platform,
so
> there
> is no version information.

That's correct. It'll probably be a few separate CLs, but once they are done
only chrome/common/chrome_content_client.cc will include widevine_cdm_version.h,
and at that point we can look for a better solution.

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/772043004/diff/120001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:65: #if defined(OS_MACOSX) ||
defined(OS_WIN)
On 2014/12/15 18:31:06, ddorwin wrote:
> Why are these 4 lines back?

Done.

[ddorwin, 2014-12-15 21:56:48]

LGTM % 1 comment. Thanks.

https://codereview.chromium.org/772043004/diff/140001/third_party/widevine/cd...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/772043004/diff/140001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:64: 
Restore #endif  // defined(ENABLE_PEPPER_CDMS) below.

[gunsch, 2014-12-16 01:37:56]

On 2014/12/15 21:56:48, ddorwin wrote:
> LGTM % 1 comment. Thanks.
> 
>
https://codereview.chromium.org/772043004/diff/140001/third_party/widevine/cd...
> File third_party/widevine/cdm/widevine_cdm_common.h (right):
> 
>
https://codereview.chromium.org/772043004/diff/140001/third_party/widevine/cd...
> third_party/widevine/cdm/widevine_cdm_common.h:64: 
> Restore #endif  // defined(ENABLE_PEPPER_CDMS) below.

chromecast/ lgtm, will keep an eye out for the follow-up CLs

[gunsch, 2014-12-16 16:28:40]

On 2014/12/16 01:37:56, gunsch wrote:
> On 2014/12/15 21:56:48, ddorwin wrote:
> > LGTM % 1 comment. Thanks.
> > 
> >
>
https://codereview.chromium.org/772043004/diff/140001/third_party/widevine/cd...
> > File third_party/widevine/cdm/widevine_cdm_common.h (right):
> > 
> >
>
https://codereview.chromium.org/772043004/diff/140001/third_party/widevine/cd...
> > third_party/widevine/cdm/widevine_cdm_common.h:64: 
> > Restore #endif  // defined(ENABLE_PEPPER_CDMS) below.
> 
> chromecast/ lgtm, will keep an eye out for the follow-up CLs

quick nit, can you update the commit message / issue title from
'enable_widevine' to 'enable_widevine_cdm'?

[sandersd (OOO until July 31), 2014-12-16 19:41:35]

sandersd@chromium.org changed reviewers:
+ brettw@chromium.org

[sandersd (OOO until July 31), 2014-12-16 19:41:36]

brettw@: Please take a look at build/ and chrome/. (chrome/ changes are mostly
mechanical.)

[brettw, 2014-12-17 19:05:46]

I don't understand the motivation for moving the widevine defines from being in
a header to being global GYP defines for every single file in the build (I don't
seem to have permission on the bug). The old way seemed strictly better. Can you
elaborate on the background for this change?

This is the 4th and 5th global define addition that I've reviewed in the past
2.5 weeks. This does not scale. I've rejected all of them and the authors have
found better ways to do their feature flags without having, for example,
libjpeg's compile get a bunch of widevine related defines.

[sandersd (OOO until July 31), 2014-12-17 19:32:35]

> The old way seemed strictly better. Can you
> elaborate on the background for this change?

Certainly. The primary motivation is to move away from using Chrome branding to
toggle Widevine-related features and to instead make it independently
configurable. A secondary motivation is to eliminate the widevine_cdm_version_h
target, because it requires build system gymnastics. From our view, the new
setup is strictly better.

> This is the 4th and 5th global define addition that I've reviewed in the past
> 2.5 weeks. This does not scale. I've rejected all of them and the authors have
> found better ways to do their feature flags without having, for example,
> libjpeg's compile get a bunch of widevine related defines.

We agree, and see this CL as an intermediate step. The longer-term plan is to
move these variables into media/cdm (note that there are currently no GN files
there), but the current situation is that there is no one dependency that all of
the users of these defines have. We would like to have a path to incrementally
fix them, and I believe that this CL is the simplest way to start that.

[ddorwin, 2014-12-17 19:49:51]

On 2014/12/17 19:32:35, sandersd wrote:
> > The old way seemed strictly better. Can you
> > elaborate on the background for this change?
> 
> Certainly. The primary motivation is to move away from using Chrome branding
to
> toggle Widevine-related features and to instead make it independently
> configurable. A secondary motivation is to eliminate the
widevine_cdm_version_h
> target, because it requires build system gymnastics. From our view, the new
> setup is strictly better.

Specifically, moving away from the defines in the header allows us to remove
dependencies on the widevine_cdm_version_h and an additional include directory
from ~10 build files where the platform-specific version is not actually needed.

[brettw, 2014-12-18 17:55:55]

On 2014/12/17 19:49:51, ddorwin wrote:
> On 2014/12/17 19:32:35, sandersd wrote:
> > > The old way seemed strictly better. Can you
> > > elaborate on the background for this change?
> > 
> > Certainly. The primary motivation is to move away from using Chrome branding
> to
> > toggle Widevine-related features and to instead make it independently
> > configurable. A secondary motivation is to eliminate the
> widevine_cdm_version_h
> > target, because it requires build system gymnastics. From our view, the new
> > setup is strictly better.
> 
> Specifically, moving away from the defines in the header allows us to remove
> dependencies on the widevine_cdm_version_h and an additional include directory
> from ~10 build files where the platform-specific version is not actually
needed.

Two global defines for this one feature just doesn't make sense to me. Again,
these are the 4th 5th global defines in the past 3 weeks I've personally
reviewed. This is not a scalable way to run a project.

It's not clear to me why reducing dependencies on the widevine_cdm_version_h
target matters or is desirable.

There are lots of ways to make include directories work or not have any at all.
I don't know what particularly you're referring to, where are the include dirs
you're trying to remove?