Add the "is_ensemble" build variable.

Add the "is_ensemble" build variable.

If "is_ensemble" is set to true, omit source files and dependencies that
need the X11, font, and graphics libraries.

Right now only the "media" target checks the "is_ensemble" build
variable. I plan to make the "media_unittests" target check
"is_ensemble" in a future CL.

R=dalecurtis@chromium.org,scherkus@chromium.org
BUG=457280
Committed: https://crrev.com/d01a45d2a982adf4941860013bc33fff2dbbbde3
Cr-Commit-Position: refs/heads/master@{#316068}

[wtc, 2015-02-11 21:26:13]

Dale: Please review. This CL implements the approach you suggested in email.

I haven't made the GYP changes yet. Once you agree with the direction of this
CL, I will add the GYP changes.

I also figured out how to modify the media_unittests target, but it requires a
change to ui/gfx, so I am postponing that part to a future CL.

[DaleCurtis, 2015-02-11 21:58:01]

https://codereview.chromium.org/914363002/diff/1/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/914363002/diff/1/media/BUILD.gn#newcode492
media/BUILD.gn:492: if (is_ensemble) {
I think it'll be less error prone if you gate the inclusion of these files and
deps on !is_ensemble instead; that way we only have one file listing for each
too.

[wtc, 2015-02-11 23:40:47]

It just occurred to me that since is_ensemble will only be used in GN, I can
leave the GYP files unchanged and just add a comment to document the is_ensemble
build variable is only used in GN.

This depends on whether you want me to modify the main sources and deps lists. I
await your answer to my question below.

https://codereview.chromium.org/914363002/diff/1/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/914363002/diff/1/media/BUILD.gn#newcode492
media/BUILD.gn:492: if (is_ensemble) {

On 2015/02/11 21:58:01, DaleCurtis wrote:
> I think it'll be less error prone if you gate the inclusion of these files and
> deps on !is_ensemble instead; that way we only have one file listing for each
> too.

Originally I did it this way to minimize my changes to media/BUILD.gn. Also I
felt that the reason we need to exclude these source files for the is_ensemble
case seems rather add-hoc, so I didn't want to muck with the main source file
and deps lists.

I just looked up this issue in the GN Style Guide. The Style Guide recommends
the current approach as the default:

  * Generally include all sources at the top and remove sources if they do
    not apply in a particular case. If you have a bunch of files for a
    particular case, you can consider adding them in that case to avoid
    listing them twice if it seems more clear.

The current approach also allows another special build configuration to remove
some of these source files and deps independently.

Did this convince you? Or would you still like me to gate the inclusion of these
source files and deps on !in_ensemble instead?

[DaleCurtis, 2015-02-11 23:53:47]

The style of the media .gn files is to avoid double entries where possible. I
think the number of files here constitutes more than a few :)

https://codereview.chromium.org/914363002/diff/1/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/914363002/diff/1/media/BUILD.gn#newcode492
media/BUILD.gn:492: if (is_ensemble) {
On 2015/02/11 23:40:47, wtc wrote:
> 
> On 2015/02/11 21:58:01, DaleCurtis wrote:
> > I think it'll be less error prone if you gate the inclusion of these files
and
> > deps on !is_ensemble instead; that way we only have one file listing for
each
> > too.
> 
> Originally I did it this way to minimize my changes to media/BUILD.gn. Also I
> felt that the reason we need to exclude these source files for the is_ensemble
> case seems rather add-hoc, so I didn't want to muck with the main source file
> and deps lists.
> 
> I just looked up this issue in the GN Style Guide. The Style Guide recommends
> the current approach as the default:
> 
>   * Generally include all sources at the top and remove sources if they do
>     not apply in a particular case. If you have a bunch of files for a
>     particular case, you can consider adding them in that case to avoid
>     listing them twice if it seems more clear.
> 
> The current approach also allows another special build configuration to remove
> some of these source files and deps independently.
> 
> Did this convince you? Or would you still like me to gate the inclusion of
these
> source files and deps on !in_ensemble instead?

I dislike the double listing and much prefer to optionally add them if it can be
done without a bunch of !is_ensemble sections (which it seems like it can).

[wtc, 2015-02-12 02:17:45]

Dale: please review patch set 2. I now gate on !is_ensemble instead, as you
suggested. I also added the GYP changes.

I am not sure if I like the results. Please take a look and let me know what you
think of this. Thanks.

https://codereview.chromium.org/914363002/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/914363002/diff/20001/media/media.gyp#newcode638
media/media.gyp:638: 'filters/default_renderer_factory.h',

These two files are excluded rather than added because they are excluded for
another build configuration (see lines 729-734).

[DaleCurtis, 2015-02-12 02:33:42]

Why add the .gyp changes?

https://codereview.chromium.org/914363002/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/914363002/diff/20001/media/BUILD.gn#newcode312
media/BUILD.gn:312: if (!is_ensemble) {
Why nest?

[wtc, 2015-02-12 15:20:31]

Dale: thanks for the review!

On 2015/02/12 02:33:42, DaleCurtis wrote:
> Why add the .gyp changes?

Mainly because I want to be ready for what a code review may ask for. Also, it
seems good to preserve the similarity between the GYP and GN build systems.
Perhaps it is now futile to do that? I actually tested my .gyp changes. I'm
happy to throw them away.

https://codereview.chromium.org/914363002/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/914363002/diff/20001/media/BUILD.gn#newcode312
media/BUILD.gn:312: if (!is_ensemble) {

On 2015/02/12 02:33:42, DaleCurtis wrote:
> Why nest?

In the original code, "filters/opus_audio_decoder.cc" and
"filters/opus_audio_decoder.h" are only added if is_android is false.

Would you like me to add these two files in a standalone if block?

  if (!is_android) {
    sources += [
      "filters/opus_audio_decoder.cc",
      "filters/opus_audio_decoder.h",
    ]
  } else {
    ...
  }
  if (!is_android && !is_ensemble) {
    sources += [
      "filters/default_renderer_factory.cc",
      "filters/default_renderer_factory.h",
    ]
  }
  
Or would you like me to add an "if (!is_android)" block nested inside the big
"if (!is_ensemble)" block below?

  if (!is_ensemble) {
    sources += [
      "filters/gpu_video_decoder.cc",
      "filters/gpu_video_decoder.h",
      ...
    ]
    if (!is_android) {
      sources += [
        "filters/default_renderer_factory.cc",
        "filters/default_renderer_factory.h",
      ]
    }
    deps += [
      "//skia",
      ...
    ]
  }

[DaleCurtis, 2015-02-12 19:19:12]

Ensemble is .gn only though, so I think it's a bit weird to  update .gyp files
for something that's always false there.

https://codereview.chromium.org/914363002/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/914363002/diff/20001/media/BUILD.gn#newcode312
media/BUILD.gn:312: if (!is_ensemble) {
On 2015/02/12 15:20:31, wtc wrote:
> 
> On 2015/02/12 02:33:42, DaleCurtis wrote:
> > Why nest?
> 
> In the original code, "filters/opus_audio_decoder.cc" and
> "filters/opus_audio_decoder.h" are only added if is_android is false.
> 
> Would you like me to add these two files in a standalone if block?
> 
>   if (!is_android) {
>     sources += [
>       "filters/opus_audio_decoder.cc",
>       "filters/opus_audio_decoder.h",
>     ]
>   } else {
>     ...
>   }
>   if (!is_android && !is_ensemble) {
>     sources += [
>       "filters/default_renderer_factory.cc",
>       "filters/default_renderer_factory.h",
>     ]
>   }
>   
> Or would you like me to add an "if (!is_android)" block nested inside the big
> "if (!is_ensemble)" block below?
> 
>   if (!is_ensemble) {
>     sources += [
>       "filters/gpu_video_decoder.cc",
>       "filters/gpu_video_decoder.h",
>       ...
>     ]
>     if (!is_android) {
>       sources += [
>         "filters/default_renderer_factory.cc",
>         "filters/default_renderer_factory.h",
>       ]
>     }
>     deps += [
>       "//skia",
>       ...
>     ]
>   }

I see. What you have is fine then.

[wtc, 2015-02-12 22:15:42]

I undid the media.gyp changes. Please review patch set 3. Thanks.

[DaleCurtis, 2015-02-12 22:37:51]

lgtm

[wtc, 2015-02-12 22:40:40]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2015-02-12 22:42:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914363002/40001

[commit-bot: I haz the power, 2015-02-12 22:45:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-12 22:46:17]

Patchset 3 (id:??) landed as
https://crrev.com/d01a45d2a982adf4941860013bc33fff2dbbbde3
Cr-Commit-Position: refs/heads/master@{#316068}