Add PDFium build_overrides

Add PDFium build_overrides

This CL adds the build override settings so we can enable XFA in GN builds
in the future.

BUG=chromium:62400
Committed: https://crrev.com/192da0f866b250d8a674a7dd400ff2a5b6a01ac8
Cr-Commit-Position: refs/heads/master@{#390448}

[dsinclair, 2016-04-27 14:18:59]

Patchset #1 (id:1) has been deleted

[dsinclair, 2016-04-27 14:21:01]

dsinclair@chromium.org changed reviewers:
+ thakis@chromium.org

[dsinclair, 2016-04-27 14:21:01]

PTAL.

This is the Chrome side portion of: https://codereview.chromium.org/1923333002/.
I'll add the DEPS change to pull in the other change once it lands.

[Nico, 2016-04-27 16:42:02]

lgtm

[Nico, 2016-04-27 16:44:12]

https://codereview.chromium.org/1923343002/diff/20001/build_overrides/pdfium.gni
File build_overrides/pdfium.gni (right):

https://codereview.chromium.org/1923343002/diff/20001/build_overrides/pdfium....
build_overrides/pdfium.gni:8: pdf_enable_xfa = false
do you want to have this set to false on your bots? or do you want this to be an
arg in pdfium builds?

[dsinclair, 2016-04-27 16:47:41]

https://codereview.chromium.org/1923343002/diff/20001/build_overrides/pdfium.gni
File build_overrides/pdfium.gni (right):

https://codereview.chromium.org/1923343002/diff/20001/build_overrides/pdfium....
build_overrides/pdfium.gni:8: pdf_enable_xfa = false
On 2016/04/27 16:44:12, Nico wrote:
> do you want to have this set to false on your bots? or do you want this to be
an
> arg in pdfium builds?

This is the Chromium side change, our bots don't use Chromium to test so this
shouldn't effect our bot runs. Yea?

[Nico, 2016-04-27 16:52:15]

https://codereview.chromium.org/1923343002/diff/20001/build_overrides/pdfium.gni
File build_overrides/pdfium.gni (right):

https://codereview.chromium.org/1923343002/diff/20001/build_overrides/pdfium....
build_overrides/pdfium.gni:8: pdf_enable_xfa = false
On 2016/04/27 16:47:41, dsinclair wrote:
> On 2016/04/27 16:44:12, Nico wrote:
> > do you want to have this set to false on your bots? or do you want this to
be
> an
> > arg in pdfium builds?
> 
> This is the Chromium side change, our bots don't use Chromium to test so this
> shouldn't effect our bot runs. Yea?

ooh I see. Then shouldn't the cr side not include the gni file but just set the
values we want for everything in chromium? And shouldn't the pdfium side not use
an include of pdfium.gni but move the contents of pdfium.gni to the new file? We
probably don't want all these declare_args in a Chromium build, right?

[dsinclair, 2016-04-27 16:55:51]

On 2016/04/27 16:52:15, Nico wrote:
>
https://codereview.chromium.org/1923343002/diff/20001/build_overrides/pdfium.gni
> File build_overrides/pdfium.gni (right):
> 
>
https://codereview.chromium.org/1923343002/diff/20001/build_overrides/pdfium....
> build_overrides/pdfium.gni:8: pdf_enable_xfa = false
> On 2016/04/27 16:47:41, dsinclair wrote:
> > On 2016/04/27 16:44:12, Nico wrote:
> > > do you want to have this set to false on your bots? or do you want this to
> be
> > an
> > > arg in pdfium builds?
> > 
> > This is the Chromium side change, our bots don't use Chromium to test so
this
> > shouldn't effect our bot runs. Yea?
> 
> ooh I see. Then shouldn't the cr side not include the gni file but just set
the
> values we want for everything in chromium? And shouldn't the pdfium side not
use
> an include of pdfium.gni but move the contents of pdfium.gni to the new file?
We
> probably don't want all these declare_args in a Chromium build, right?


We may not want the declare args in Chromium, but if there are other consumers
of PDFium, they may want them? Or, would they just use the
build_override/pdfium.gni file? If there is no declare_args can I still set the
value when doing a 'gn args' to override the setting?

[dsinclair, 2016-04-27 16:56:30]

dsinclair@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[dsinclair, 2016-04-27 16:56:30]

+more PDFium devs to see if they have opinions on how this should work.

[Nico, 2016-04-27 16:59:08]

No, without declare_args you can't override this in `gn args`. Do you think
clients would want to change many of these args?

(tbh this whole build_overrides thing seems a bit messy -- normally you want to
set the default value for an arg in the upstream project, and then allow
downstream projects to tweak the default value for a subset of flags. I think
Dirk said there's a bug for that somewhere.)

[dsinclair, 2016-04-27 17:04:55]

On 2016/04/27 16:59:08, Nico wrote:
> No, without declare_args you can't override this in `gn args`. Do you think
> clients would want to change many of these args?
> 
> (tbh this whole build_overrides thing seems a bit messy -- normally you want
to
> set the default value for an arg in the upstream project, and then allow
> downstream projects to tweak the default value for a subset of flags. I think
> Dirk said there's a bug for that somewhere.)


We do have clients that build without XFA and without V8. The Skia flag is
changed by devs and will hopefully go away if we can get rid of agg.

We need to be able to disable XFA and V8 on the bots as well for testing
purposes.

So, I want to be able to flip the pdf_enable_xfa flag true from a Chromium
build. This was the only way I could figure out how to do that. If there is a
better way, can you point me to an example? We need to be able to flip these
flags outside of a chrome build (our standalone build for example) and have
default values set.

When you say 'upstream' do you mean the PDFium repo? We have the defaults set in
pdfium.gni but I couldn't figure out how to change it from chrome GN.

[Nico, 2016-04-27 17:10:27]

On 2016/04/27 17:04:55, dsinclair wrote:
> On 2016/04/27 16:59:08, Nico wrote:
> > No, without declare_args you can't override this in `gn args`. Do you think
> > clients would want to change many of these args?
> > 
> > (tbh this whole build_overrides thing seems a bit messy -- normally you want
> to
> > set the default value for an arg in the upstream project, and then allow
> > downstream projects to tweak the default value for a subset of flags. I
think
> > Dirk said there's a bug for that somewhere.)
> 
> 
> We do have clients that build without XFA and without V8.

Does that flag need to be an arg, or could clients just set that flag to false
in their build_overrides?

> The Skia flag is
> changed by devs and will hopefully go away if we can get rid of agg.
> 
> We need to be able to disable XFA and V8 on the bots as well for testing
> purposes.

Chrome bots or pdfium bots?

> So, I want to be able to flip the pdf_enable_xfa flag true from a Chromium
> build. This was the only way I could figure out how to do that. If there is a
> better way, can you point me to an example? We need to be able to flip these
> flags outside of a chrome build (our standalone build for example) and have
> default values set.

In pdfium's build_overrides/pdfium.gni:

  declare_args() {
    use_xfa = false
  }

In Chromium's build_overrides/pdfium.gni:

  use_xfa = false

And then flip in Chromium when you're ready. (By changing the "false" to "true"
in chromium's build_overrides/pdfium.gni)

This is close to what you have now, but it removes pdfium/pdfium.gni and inlines
it into pdfium/build_overrides/pdfium.gni, and doesn't include that on the
Chromium side.

> When you say 'upstream' do you mean the PDFium repo? We have the defaults set
in
> pdfium.gni but I couldn't figure out how to change it from chrome GN.

[dsinclair, 2016-04-27 17:13:28]

On 2016/04/27 17:10:27, Nico wrote:
> On 2016/04/27 17:04:55, dsinclair wrote:
> > On 2016/04/27 16:59:08, Nico wrote:
> > > No, without declare_args you can't override this in `gn args`. Do you
think
> > > clients would want to change many of these args?
> > > 
> > > (tbh this whole build_overrides thing seems a bit messy -- normally you
want
> > to
> > > set the default value for an arg in the upstream project, and then allow
> > > downstream projects to tweak the default value for a subset of flags. I
> think
> > > Dirk said there's a bug for that somewhere.)
> > 
> > 
> > We do have clients that build without XFA and without V8.
> 
> Does that flag need to be an arg, or could clients just set that flag to false
> in their build_overrides?


Isn't it still a flag in your example below because it's in the declare_args
section?


> 
> > The Skia flag is
> > changed by devs and will hopefully go away if we can get rid of agg.
> > 
> > We need to be able to disable XFA and V8 on the bots as well for testing
> > purposes.
> 
> Chrome bots or pdfium bots?


PDFium bots. Chrome should always build the same way (modulo us flipping the
flag to enable XFA).

> 
> > So, I want to be able to flip the pdf_enable_xfa flag true from a Chromium
> > build. This was the only way I could figure out how to do that. If there is
a
> > better way, can you point me to an example? We need to be able to flip these
> > flags outside of a chrome build (our standalone build for example) and have
> > default values set.
> 
> In pdfium's build_overrides/pdfium.gni:
> 
>   declare_args() {
>     use_xfa = false
>   }
> 
> In Chromium's build_overrides/pdfium.gni:
> 
>   use_xfa = false
> 
> And then flip in Chromium when you're ready. (By changing the "false" to
"true"
> in chromium's build_overrides/pdfium.gni)
> 

Let me try that and see what happens.

[Nico, 2016-04-27 17:13:53]

Yes, but only pdfium, not in Chromium.

[dsinclair, 2016-04-27 17:24:43]

On 2016/04/27 17:13:53, Nico wrote:
> Yes, but only pdfium, not in Chromium.

Ok, PTAL. The one downside is, every arg we have in the
pdfium/build_overrides/pdfium.gni also needs to appear in the
build_overrides/pdfium.gni for Chromium (and any other downstream embedder).

[Nico, 2016-04-27 19:38:02]

lgtm

https://codereview.chromium.org/1923343002/diff/40001/build_overrides/pdfium.gni
File build_overrides/pdfium.gni (right):

https://codereview.chromium.org/1923343002/diff/40001/build_overrides/pdfium....
build_overrides/pdfium.gni:9: # Build PDFium either with or without v8 support.
I think it'd be better to explain why Chromium chooses the value it chooses in
the comment, instead of repeating the variable name.

# Chromium ships with v8, so always use it for JavaScript in PDFs

ir some such (and similar for the others)

[dsinclair, 2016-04-27 20:09:32]

On 2016/04/27 19:38:02, Nico wrote:
> lgtm
> 
>
https://codereview.chromium.org/1923343002/diff/40001/build_overrides/pdfium.gni
> File build_overrides/pdfium.gni (right):
> 
>
https://codereview.chromium.org/1923343002/diff/40001/build_overrides/pdfium....
> build_overrides/pdfium.gni:9: # Build PDFium either with or without v8
support.
> I think it'd be better to explain why Chromium chooses the value it chooses in
> the comment, instead of repeating the variable name.
> 
> # Chromium ships with v8, so always use it for JavaScript in PDFs
> 
> ir some such (and similar for the others)

Done.

[dsinclair, 2016-04-27 20:10:32]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-04-27 20:10:32]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1923343002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-04-27 20:11:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923343002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923343002/60001

[commit-bot: I haz the power, 2016-04-27 20:14:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 20:14:01]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dsinclair, 2016-04-27 20:17:52]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-04-27 20:17:53]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1923343002/#ps80001
(title: "Rebase to master")

[commit-bot: I haz the power, 2016-04-27 20:18:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923343002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923343002/80001

[commit-bot: I haz the power, 2016-04-27 20:26:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 20:26:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dsinclair, 2016-04-27 20:27:41]

thestig@ for pdf/ OWNERS

[Lei Zhang, 2016-04-27 20:30:10]

lgtm

[dsinclair, 2016-04-27 20:32:15]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-04-27 20:32:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923343002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923343002/80001

[commit-bot: I haz the power, 2016-04-27 21:40:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 21:40:47]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Dirk Pranke, 2016-04-28 00:53:11]

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

[Dirk Pranke, 2016-04-28 00:53:12]

drive-by lgtm w/ a comment.

The whole build_overrides thing needs to be thrown out and redone before it gets
even further out of hand as we're trying to get more of our projects using GN.

The bug for that is crbug.com/588513 . If I ever finish flipping linux bots over
to GN, I hope to work on it next, but any sufficiently motivated soul should
feel free to take a stab in the meantime.

https://codereview.chromium.org/1923343002/diff/80001/build_overrides/pdfium.gni
File build_overrides/pdfium.gni (right):

https://codereview.chromium.org/1923343002/diff/80001/build_overrides/pdfium....
build_overrides/pdfium.gni:19: pdf_is_standalone = false
You could theoretically use the build_with_chromium flag in
//build_overrides/build.gni instead.

[dsinclair, 2016-04-28 14:14:45]

https://codereview.chromium.org/1923343002/diff/80001/build_overrides/pdfium.gni
File build_overrides/pdfium.gni (right):

https://codereview.chromium.org/1923343002/diff/80001/build_overrides/pdfium....
build_overrides/pdfium.gni:19: pdf_is_standalone = false
On 2016/04/28 00:53:12, Dirk Pranke wrote:
> You could theoretically use the build_with_chromium flag in
> //build_overrides/build.gni instead.

My preference is to not tie this to chromium as PDFium can be embedded in other
contexts as well.

[dsinclair, 2016-04-28 18:32:34]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-04-28 18:32:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, thakis@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1923343002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-04-28 18:33:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923343002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923343002/100001

[commit-bot: I haz the power, 2016-04-28 19:37:52]

Committed patchset #5 (id:100001)

[aizatsky, 2016-04-29 00:17:48]

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

[aizatsky, 2016-04-29 00:17:49]

Looks like this change broke all fuzzer bots:

https://uberchromegw.corp.google.com/i/chromium.fyi/builders/Libfuzzer%20Uplo...

[Dirk Pranke, 2016-04-29 01:00:48]

On 2016/04/29 00:17:49, aizatsky wrote:
> Looks like this change broke all fuzzer bots:
> 
>
https://uberchromegw.corp.google.com/i/chromium.fyi/builders/Libfuzzer%20Uplo...

Oh, whoops :(.

Yeah, you need to do a more complicated dance in order to not have the
build_overrides conflict with declared args.

See how we do this with mac_sdk_min and mac_sdk_min_build_override in
//build/config/mac/mac_sdk.gni and //build_overrides/build.gni

Sorry, I forgot about this.

[dsinclair, 2016-04-29 02:25:04]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/1933583002/ by dsinclair@chromium.org.

The reason for reverting is: Broke fuzzer bots..

[commit-bot: I haz the power, 2016-04-30 17:20:17]

Patchset 5 (id:??) landed as
https://crrev.com/192da0f866b250d8a674a7dd400ff2a5b6a01ac8
Cr-Commit-Position: refs/heads/master@{#390448}

[commit-bot: I haz the power, 2016-04-30 17:56:57]

Description was changed from

==========
Add PDFium build_overrides

This CL adds the build override settings so we can enable XFA in GN builds
in the future.

BUG=chromium:62400
==========

to

==========
Add PDFium build_overrides

This CL adds the build override settings so we can enable XFA in GN builds
in the future.

BUG=chromium:62400

Committed: https://crrev.com/192da0f866b250d8a674a7dd400ff2a5b6a01ac8
Cr-Commit-Position: refs/heads/master@{#390448}
==========