Create a standalone pdfium build.

Create a standalone pdfium build.

I moved pdfium_test from chromium's repo to pdfium's. It's now buildable as a sample following the instructions on the wiki.

[jam, 2014-05-22 23:22:28]

I'm trying to use v8 as inspiration as you suggested.

After cloning pdfium, I put gyp in build\gyp and then ran 'build/gyp_pdfium'. I
get the following error which I'm not sure why I'm getting. for v8, they don't
have a 'targets' section in their standalone.gypi file either?


Updating projects from gyp files...
Using overrides found in C:\Users\jabdelmalek\.gyp\include.gypi
gyp: Unable to find targets in build file
F:\src\pdfium_self\pdfium\build\standalone.gypi

[jam, 2014-05-23 20:27:38]

ptal, this now works on mac/linux/win with and without Ninja. thanks for the
help.

pdfium_test.cc is a copy of the file in src\pdf, I will delete that when I roll
this in.

[Nico, 2014-05-23 21:14:33]

lgtm. Lots of nits below, do the ones you agree with.

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium
File build/gyp_pdfium (right):

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium#newcode28
build/gyp_pdfium:28: if __name__ == '__main__':
(nit: since python only has two namespaces – global and per-function – most code
is usually in a def main() and the bottom just calls main. As is, "args" is a
global, and if any function tries to write a local variable called args but the
author makes a typo and does "arsg = foo; print args", that function will then
silently use the global.

This script is short enough to not worry about it, but I thought I'd mention
it.)

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium#newcode45
build/gyp_pdfium:45: run_gyp(gyp_args)
nit: Just `run_gyp(args)` is probably enough. The "generator for the
architectures…" comment looks a bit out of place (in gyp_v8 too – i'm guessing
it made sense there once, but then the code it referred to got removed)

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium#newcode46
build/gyp_pdfium:46: 
nit: don't include an empty line at the end of the file

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium.py
File build/gyp_pdfium.py (right):

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium.py#new...
build/gyp_pdfium.py:8: execfile(os.path.join(path, 'gyp_pdfium'))
Chromium doesn't have this trampoline file, it has everything right in
build/gyp_chromium (no .py file). Unless this file is needed for something, I'd
probably omit it.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:11: 'visibility%': 'hidden',
Do you need this? (The mac branch of this file always builds with
-fvisibility=hidden, so maybe you don't need this option for linux and can use
it unconditionally?)

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:81: '-Os',
This looks strange. I'm guessing you don't need it.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:85: '-ffunction-sections',
cflags is only used on linux anyway, so add these two to the unconditional
cflags section in line 58.

…oh, I guess cflags is used for both linux and android. But you probably want
fdata-sectiosn / ffunction-sections on android too, for dead code stripping.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:89: 'xcode_settings': {
xcode_settings is only used on mac anyways, move it up a level and remove the
mac conditional

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:98: 'cflags!': [
Not sure why this is needed, since nothing added -Os

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:119: ['OS=="mac"', {
Again, doesn't have to be in a conditional

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:126: # is specified explicitly.
This is no longer true, -fstrict-aliasing is now on by default. Chromium
explicitly turns it off though (for linux too) since we have hundreds of
aliasing violations in chrome and I don't think we'll ever get this right. So
maybe change the setting to NO and add -fno-strict-aliasing to cflags. (Else,
just remove this setting.)

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:131: 'msvs_settings': {
msvs_settings is only used on windows, so no need for this conditional either
(like in the Debug case)

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:160: ['os_posix == 1 and OS != "mac"', {
cflags isn't used on mac, no need to check for OS!=mac. (…which makes the
os_posix check redundant too)

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:165: ['OS == "mac"', {
check unneeded

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:170: ['OS == "win"', {
check unneeded

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:185: ['OS=="linux"', {
you probably want all that on android too? in that case, conditional not needed

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:201: ['OS=="win"', {
conditional not needed

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:214: 'msvs_cygwin_dirs':
['<(DEPTH)/v8/third_party/cygwin'],
hopefully not needed! :-)

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:222: 'WarnAsError': 'true',
Fine with me, but linux and mac check 'werror' for warnings as errors, maybe
windows should too

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:272: 'GCC_THREADSAFE_STATICS': 'NO',           #
-fno-threadsafe-statics
Do you want this? I think msvs's statics are threadsafe since 2013 (?) and you
don't pass -fno-threadsafe-statics on linux. (Then again, I think we use that
flag with both linux and mac in chromium, for consistency with msvs.)

There probably aren't threads in pdfium, in that case I'd remove this line as it
doesn't matter.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:274: # MACOSX_DEPLOYMENT_TARGET maps to
-mmacosx-version-min
Remove comment, the thing it refers to isn't here

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:275: 'PREBINDING': 'NO',                       # No
-Wl,-prebind
You can probably remove this, current toolchains don't do prebinding, this was
removed long ago

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:14: #include "../fpdfsdk/include/fpdf_dataavail.h"
nit: Relative includes looks strange; maybe add `'include_dirs': ['<(DEPTH)'],`
to samples.gyp

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:22: #define snprintf _snprintf
Not sure if it's important, but _snprintf is different from snprintf in how they
behave if the string is too long for the buffer: snprintf adds a trailing \0,
while _snprintf prefers to include one more character instead of adding the \0. 
(If this is important, I think you can use _snprintf_s in _TRUNCATE mode to
emulate snprintf on windows)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:29: static void write_file(const char* pdf_name, int num,
nit: WritePpm?

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:50: int h, w;
declare these in the for loop, you only use them in the respective loop bodies
(`for (int h = 0; …`)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:67: }
(fwiw, writing an inefficiently compressed png isn't much harder:
https://github.com/nico/hack/blob/master/wpng/wpng.c and if you're going for
easy-to-write, consider tga instead of ppm:
https://github.com/nico/hack/blob/master/wpng/wtga.c)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:69: int Form_Alert(IPDF_JSPLATFORM*, FPDF_WIDESTRING,
FPDF_WIDESTRING, int, int) {
nit: no underscores in function names (?) (if so, also below)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:70: printf("Form_Alert called.\n");
should this print to stderr?

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:75: std::string feature = "Unknown";
(nit: i'd make this a const char*, then there's fewer copying and the printf
line is slightly shorter)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:115: printf("Unsupported feature: %s.\n",
feature.c_str());
stderr?

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:119: std::list<const char*>* files) {
nit: use a vector instead of a list (then the main loop can just be over an int
too)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:171: memset(&platform_callbacks, '\0',
sizeof(platform_callbacks));
nit: 0 is more common than \0 in memset calls (also below)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:201: (void) FPDFAvail_IsDocAvail(pdf_avail, &hints);
Maybe add a comment why this is being called; it's not clear to me

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:216: FPDF_SetFormFieldHighlightAlpha(form, 100);
nit: maybe use a number that isn't 100 here. The argument goes from 0-255, and
100 suggests it might be a percentage thing. Maybe `, 142);  // arbitrary)

or write 100 as 0x64, then it doesn't look like a percentage either

[jam, 2014-05-23 23:52:13]

Thanks

I've done almost all the comments except to pdfium_test.cc, which is a moved
file

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium
File build/gyp_pdfium (right):

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium#newcode28
build/gyp_pdfium:28: if __name__ == '__main__':
On 2014/05/23 21:14:33, Nico wrote:
> (nit: since python only has two namespaces – global and per-function – most
code
> is usually in a def main() and the bottom just calls main. As is, "args" is a
> global, and if any function tries to write a local variable called args but
the
> author makes a typo and does "arsg = foo; print args", that function will then
> silently use the global.
> 
> This script is short enough to not worry about it, but I thought I'd mention
> it.)

Done.

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium#newcode45
build/gyp_pdfium:45: run_gyp(gyp_args)
On 2014/05/23 21:14:33, Nico wrote:
> nit: Just `run_gyp(args)` is probably enough. The "generator for the
> architectures…" comment looks a bit out of place (in gyp_v8 too – i'm guessing
> it made sense there once, but then the code it referred to got removed)

Done.

https://codereview.chromium.org/298893007/diff/100001/build/gyp_pdfium#newcode46
build/gyp_pdfium:46: 
On 2014/05/23 21:14:33, Nico wrote:
> nit: don't include an empty line at the end of the file

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:11: 'visibility%': 'hidden',
On 2014/05/23 21:14:33, Nico wrote:
> Do you need this? (The mac branch of this file always builds with
> -fvisibility=hidden, so maybe you don't need this option for linux and can use
> it unconditionally?)

I had copied it from v8's file; removed.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:81: '-Os',
On 2014/05/23 21:14:33, Nico wrote:
> This looks strange. I'm guessing you don't need it.

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:85: '-ffunction-sections',
On 2014/05/23 21:14:33, Nico wrote:
> cflags is only used on linux anyway, so add these two to the unconditional
> cflags section in line 58.
> 
> …oh, I guess cflags is used for both linux and android. But you probably want
> fdata-sectiosn / ffunction-sections on android too, for dead code stripping.

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:89: 'xcode_settings': {
On 2014/05/23 21:14:33, Nico wrote:
> xcode_settings is only used on mac anyways, move it up a level and remove the
> mac conditional

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:98: 'cflags!': [
On 2014/05/23 21:14:33, Nico wrote:
> Not sure why this is needed, since nothing added -Os

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:119: ['OS=="mac"', {
On 2014/05/23 21:14:33, Nico wrote:
> Again, doesn't have to be in a conditional

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:126: # is specified explicitly.
On 2014/05/23 21:14:33, Nico wrote:
> This is no longer true, -fstrict-aliasing is now on by default. Chromium
> explicitly turns it off though (for linux too) since we have hundreds of
> aliasing violations in chrome and I don't think we'll ever get this right. So
> maybe change the setting to NO and add -fno-strict-aliasing to cflags. (Else,
> just remove this setting.)

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:131: 'msvs_settings': {
On 2014/05/23 21:14:33, Nico wrote:
> msvs_settings is only used on windows, so no need for this conditional either
> (like in the Debug case)

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:160: ['os_posix == 1 and OS != "mac"', {
On 2014/05/23 21:14:33, Nico wrote:
> cflags isn't used on mac, no need to check for OS!=mac. (…which makes the
> os_posix check redundant too)

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:165: ['OS == "mac"', {
On 2014/05/23 21:14:33, Nico wrote:
> check unneeded

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:170: ['OS == "win"', {
On 2014/05/23 21:14:33, Nico wrote:
> check unneeded

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:185: ['OS=="linux"', {
On 2014/05/23 21:14:33, Nico wrote:
> you probably want all that on android too? in that case, conditional not
needed

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:201: ['OS=="win"', {
On 2014/05/23 21:14:33, Nico wrote:
> conditional not needed

seems like defines that are windows specific don't need to pollute the command
line flags for other platforms so best to keep in a windows-conditional?

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:214: 'msvs_cygwin_dirs':
['<(DEPTH)/v8/third_party/cygwin'],
On 2014/05/23 21:14:33, Nico wrote:
> hopefully not needed! :-)

unfortunately it is (i had found out the hard way since I didn't copy it over
initially). v8 generates some source files from javascript and uses cygwin for
that on windows

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:222: 'WarnAsError': 'true',
On 2014/05/23 21:14:33, Nico wrote:
> Fine with me, but linux and mac check 'werror' for warnings as errors, maybe
> windows should too

this is copied from v8 and chrome's gypis, so since i'm not sure what to do,
i'll leave as is

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:272: 'GCC_THREADSAFE_STATICS': 'NO',           #
-fno-threadsafe-statics
On 2014/05/23 21:14:33, Nico wrote:
> Do you want this? I think msvs's statics are threadsafe since 2013 (?) and you
> don't pass -fno-threadsafe-statics on linux. (Then again, I think we use that
> flag with both linux and mac in chromium, for consistency with msvs.)
> 
> There probably aren't threads in pdfium, in that case I'd remove this line as
it
> doesn't matter.

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:274: # MACOSX_DEPLOYMENT_TARGET maps to
-mmacosx-version-min
On 2014/05/23 21:14:33, Nico wrote:
> Remove comment, the thing it refers to isn't here

Done.

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:275: 'PREBINDING': 'NO',                       # No
-Wl,-prebind
On 2014/05/23 21:14:33, Nico wrote:
> You can probably remove this, current toolchains don't do prebinding, this was
> removed long ago

Done.

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:14: #include "../fpdfsdk/include/fpdf_dataavail.h"
On 2014/05/23 21:14:33, Nico wrote:
> nit: Relative includes looks strange; maybe add `'include_dirs':
['<(DEPTH)'],`
> to samples.gyp

Done.

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:22: #define snprintf _snprintf
On 2014/05/23 21:14:33, Nico wrote:
> Not sure if it's important, but _snprintf is different from snprintf in how
they
> behave if the string is too long for the buffer: snprintf adds a trailing \0,
> while _snprintf prefers to include one more character instead of adding the
\0. 
> (If this is important, I think you can use _snprintf_s in _TRUNCATE mode to
> emulate snprintf on windows)

i'm going to leave this since it's all moved code
(http://src.chromium.org/viewvc/chrome/trunk/src/pdf/pdfium/pdfium_test.cc?rev...)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:29: static void write_file(const char* pdf_name, int num,
On 2014/05/23 21:14:33, Nico wrote:
> nit: WritePpm?

Done.

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:50: int h, w;
On 2014/05/23 21:14:33, Nico wrote:
> declare these in the for loop, you only use them in the respective loop bodies
> (`for (int h = 0; …`)

Done.

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:67: }
On 2014/05/23 21:14:33, Nico wrote:
> (fwiw, writing an inefficiently compressed png isn't much harder:
> https://github.com/nico/hack/blob/master/wpng/wpng.c and if you're going for
> easy-to-write, consider tga instead of ppm:
> https://github.com/nico/hack/blob/master/wpng/wtga.c)

ditto

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:69: int Form_Alert(IPDF_JSPLATFORM*, FPDF_WIDESTRING,
FPDF_WIDESTRING, int, int) {
On 2014/05/23 21:14:33, Nico wrote:
> nit: no underscores in function names (?) (if so, also below)

ditto

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:70: printf("Form_Alert called.\n");
On 2014/05/23 21:14:33, Nico wrote:
> should this print to stderr?

ditto

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:70: printf("Form_Alert called.\n");
On 2014/05/23 21:14:33, Nico wrote:
> should this print to stderr?

ditto (no idea about these questions, this tool is used by fuzzing)

https://codereview.chromium.org/298893007/diff/100001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:75: std::string feature = "Unknown";
On 2014/05/23 21:14:33, Nico wrote:
> (nit: i'd make this a const char*, then there's fewer copying and the printf
> line is slightly shorter)

ditto

[Nico, 2014-05-24 23:14:23]

lgtm with cflags fixed, and lebtm with host/target_arch removed

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:201: ['OS=="win"', {
On 2014/05/23 23:52:13, jam wrote:
> On 2014/05/23 21:14:33, Nico wrote:
> > conditional not needed
> 
> seems like defines that are windows specific don't need to pollute the command
> line flags for other platforms so best to keep in a windows-conditional?

Oh, you're right. Maybe move the msvs_foo out though.

https://codereview.chromium.org/298893007/diff/140001/build/gyp_pdfium
File build/gyp_pdfium (right):

https://codereview.chromium.org/298893007/diff/140001/build/gyp_pdfium#newcode44
build/gyp_pdfium:44: 
(nit: style guide says 2 empty lines between python toplevels)

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi#n...
build/standalone.gypi:10: 'clang%': 0,
Nothing reads this.

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi#n...
build/standalone.gypi:41: 'target_arch%': '<(target_arch)',
It looks like nothing reads host_arch and target_arch at the moment either,
maybe just remove them for now (this allows removing 4 nested variable blocks,
which is kind of nice). Sorry for missing this last time.

(It's possible to build for android with this gyp file as is, independent of the
presence of host_arch and target_arch, by setting CC_host and CC_target and
CFLAGS_target envvars. Chromium knows the location of the ndk compiler binaries,
so it can use make_global_settings to make this easier, but you don't have an
ndk deps'd in, so tweaking env vars is probably the best you can do.)

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi#n...
build/standalone.gypi:137: 'cflags!': [
You mean 'cflags', not 'cflags!' here, right? (The former sets them, the latter
removes them if present)

[jam, 2014-05-27 01:46:21]

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/298893007/diff/100001/build/standalone.gypi#n...
build/standalone.gypi:201: ['OS=="win"', {
On 2014/05/24 23:14:23, Nico (traveling) wrote:
> On 2014/05/23 23:52:13, jam wrote:
> > On 2014/05/23 21:14:33, Nico wrote:
> > > conditional not needed
> > 
> > seems like defines that are windows specific don't need to pollute the
command
> > line flags for other platforms so best to keep in a windows-conditional?
> 
> Oh, you're right. Maybe move the msvs_foo out though.

well those ones won't impact other platforms right?  i.e. it's my understanding
that msvs_foo will be ignored on mac/linux. that's different from this
situation, where a defines[] outside os==win that's only for windows would have
polluted max/linux's build flags.

https://codereview.chromium.org/298893007/diff/140001/build/gyp_pdfium
File build/gyp_pdfium (right):

https://codereview.chromium.org/298893007/diff/140001/build/gyp_pdfium#newcode44
build/gyp_pdfium:44: 
On 2014/05/24 23:14:23, Nico (traveling) wrote:
> (nit: style guide says 2 empty lines between python toplevels)

will fix in a followup

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi#n...
build/standalone.gypi:10: 'clang%': 0,
On 2014/05/24 23:14:23, Nico (traveling) wrote:
> Nothing reads this.

will also remove in a followup

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi#n...
build/standalone.gypi:41: 'target_arch%': '<(target_arch)',
On 2014/05/24 23:14:23, Nico (traveling) wrote:
> It looks like nothing reads host_arch and target_arch at the moment either,
> maybe just remove them for now (this allows removing 4 nested variable blocks,
> which is kind of nice). Sorry for missing this last time.

these were needed by v8's gyp files. does that make sense?

> 
> (It's possible to build for android with this gyp file as is, independent of
the
> presence of host_arch and target_arch, by setting CC_host and CC_target and
> CFLAGS_target envvars. Chromium knows the location of the ndk compiler
binaries,
> so it can use make_global_settings to make this easier, but you don't have an
> ndk deps'd in, so tweaking env vars is probably the best you can do.)

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi#n...
build/standalone.gypi:137: 'cflags!': [
On 2014/05/24 23:14:23, Nico (traveling) wrote:
> You mean 'cflags', not 'cflags!' here, right? (The former sets them, the
latter
> removes them if present)

oops, that was by accident. fixing in followup

[jam, 2014-05-27 04:45:06]

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/298893007/diff/140001/build/standalone.gypi#n...
build/standalone.gypi:10: 'clang%': 0,
On 2014/05/27 01:46:21, jam wrote:
> On 2014/05/24 23:14:23, Nico (traveling) wrote:
> > Nothing reads this.
> 
> will also remove in a followup

actually looks like it's used in pdfium.gyp

[Nico, 2014-05-27 08:20:06]

Ah, that makes sense. Maybe add a "# Needed by v8" to the _arch thingies
when you touch this file again.


On Tue, May 27, 2014 at 6:45 AM, <jam@chromium.org> wrote:

>
> https://codereview.chromium.org/298893007/diff/140001/
> build/standalone.gypi
> File build/standalone.gypi (right):
>
> https://codereview.chromium.org/298893007/diff/140001/
> build/standalone.gypi#newcode10
> build/standalone.gypi:10: 'clang%': 0,
> On 2014/05/27 01:46:21, jam wrote:
>
>> On 2014/05/24 23:14:23, Nico (traveling) wrote:
>> > Nothing reads this.
>>
>
>  will also remove in a followup
>>
>
> actually looks like it's used in pdfium.gyp
>
> https://codereview.chromium.org/298893007/
>

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