Add libpoppler PDF rasterizer

The CL adds libpoppler to DEPS and adds a libpoppler-cpp gyp target for Linux, Windows, and Mac. This does not currently change the GM tool to use poppler - that will be a refactor job in a separate CL.

Several third-party libraries are pulled in, and platform-specific headers are included under third_party/(library name). Chromium style READMEs containing a description of the library, as well as a LICENSE file are also included in that directory. (is there a Skia-specific style for these?)

Committed: http://code.google.com/p/skia/source/detail?r=10823

[ducky, 2013-07-31 22:44:38]

Here's a monster CL which pulls in 4 libraries (poppler and dependencies),
complete with DEPS, GYP targets, and generated platform-specific headers. 

Everything except the LICENSE and README files under third_party were generated
by the library's configure process and included here to allow GYP to build the
libraries. The README files are in Chromium style (does Skia have a style for
these?).

I think the main files needing review are the changes outside third_party, like
the gypfiles, the DEPS change, and the SkPDFRasterizer interface for libpoppler.
Note that this doesn't change the GM tool - that's a bigger refactor job for
another CL.

I attempted to minimize the gypfiles, removing files that weren't necessary for
the rasterizer to build (and as a result, this doesn't build the complete
library).

[vandebo (ex-Chrome), 2013-08-01 17:49:51]

Danny: please comment on anything you'd like to look scarier ;-)

https://codereview.chromium.org/20220002/diff/21001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/20220002/diff/21001/gyp/fontconfig.gyp#newcode67
gyp/fontconfig.gyp:67: ['skia_os == "linux"', {
Above you said this isn't built on Linux.

https://codereview.chromium.org/20220002/diff/21001/gyp/fontconfig.gyp#newcod...
gyp/fontconfig.gyp:107: 'inline=__inline',	# necessary to compile on C, where
inline is not recognized and has to be __inline
80 col?

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp
File gyp/poppler.gyp (right):

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp#newcode3
gyp/poppler.gyp:3: # !!! WARNING !!! Poppler is GPL software, and should not be
used in anything
Danny: let us know if you'd like this to change at all.

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp#newcode15
gyp/poppler.gyp:15: 'target_name': 'libpoppler',
for extra safety name this libpoppler-gpl ?

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp#newcode36
gyp/poppler.gyp:36: '../third_party/externals/poppler/fofi/FoFiBase.cc',
alphabetical order please

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp#newcode164
gyp/poppler.gyp:164: 'target_name': 'libpoppler-cpp',
libpoppler-shared ?

https://codereview.chromium.org/20220002/diff/21001/src/utils/SkPDFRasterizer.h
File src/utils/SkPDFRasterizer.h (right):

https://codereview.chromium.org/20220002/diff/21001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.h:2: * Copyright 2013 Google Inc.
Should this file be name SkPDFPopplerRasterizer.h ?

https://codereview.chromium.org/20220002/diff/21001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.h:13: bool SkPopplerRasterizePDF(SkStream* pdf,
SkBitmap* output);
Should it take a page number as an argument?

https://codereview.chromium.org/20220002/diff/21001/third_party/fontconfig/li...
File third_party/fontconfig/linux/config.h (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/fontconfig/li...
third_party/fontconfig/linux/config.h:1: /* config.h.  Generated from
config.h.in by configure.  */
It looks like this is pretty similar to the mac one.  Maybe this should get
stripped down to the common ones, and then the rest of the #defines put into the
gyp file?

https://codereview.chromium.org/20220002/diff/21001/third_party/iconv/global/...
File third_party/iconv/global/aliases.h (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/iconv/global/...
third_party/iconv/global/aliases.h:1: /* ANSI-C code produced by gperf version
3.0.4 */
If these aren't platform specific, can they be programmatically generated in the
gyp file?

https://codereview.chromium.org/20220002/diff/21001/third_party/poppler/READM...
File third_party/poppler/README.chromium (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/poppler/READM...
third_party/poppler/README.chromium:10: A PDF rendering library, currently used
in the Skia gm for regression-testing PDF output.
You might want to add here that it is and should only be used for testing code.

[ducky, 2013-08-01 20:40:23]

More style improvements, cleanup, and fixes.

https://codereview.chromium.org/20220002/diff/21001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/20220002/diff/21001/gyp/fontconfig.gyp#newcode67
gyp/fontconfig.gyp:67: ['skia_os == "linux"', {
On 2013/08/01 17:49:51, vandebo wrote:
> Above you said this isn't built on Linux.

Good point, I guess this is dead code. It probably still works on Linux, just
not tested or used. Removed.

https://codereview.chromium.org/20220002/diff/21001/gyp/fontconfig.gyp#newcod...
gyp/fontconfig.gyp:107: 'inline=__inline',	# necessary to compile on C, where
inline is not recognized and has to be __inline
On 2013/08/01 17:49:51, vandebo wrote:
> 80 col?

Fixed. Some gypfiles seem to be really bad at following 80 cols, so I wasn't
sure if it was a standard.

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp
File gyp/poppler.gyp (right):

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp#newcode15
gyp/poppler.gyp:15: 'target_name': 'libpoppler',
On 2013/08/01 17:49:51, vandebo wrote:
> for extra safety name this libpoppler-gpl ?

Done.

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp#newcode36
gyp/poppler.gyp:36: '../third_party/externals/poppler/fofi/FoFiBase.cc',
On 2013/08/01 17:49:51, vandebo wrote:
> alphabetical order please

Done. Original was the order was invoked by make, but I guess that really
doesn't matter.

https://codereview.chromium.org/20220002/diff/21001/gyp/poppler.gyp#newcode164
gyp/poppler.gyp:164: 'target_name': 'libpoppler-cpp',
On 2013/08/01 17:49:51, vandebo wrote:
> libpoppler-shared ?

I think this is more in line with the terminology used by popper, which builds
the C++ interface for libpoppler. I'll add -gpl to it just to be safe, as with
the above.

https://codereview.chromium.org/20220002/diff/21001/src/utils/SkPDFRasterizer.h
File src/utils/SkPDFRasterizer.h (right):

https://codereview.chromium.org/20220002/diff/21001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.h:2: * Copyright 2013 Google Inc.
On 2013/08/01 17:49:51, vandebo wrote:
> Should this file be name SkPDFPopplerRasterizer.h ?

I actually intended this to be for rasterizing PDF files in general, where
interfaces for multiple backends could live (even though we only use poppler
currently)

https://codereview.chromium.org/20220002/diff/21001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.h:13: bool SkPopplerRasterizePDF(SkStream* pdf,
SkBitmap* output);
On 2013/08/01 17:49:51, vandebo wrote:
> Should it take a page number as an argument?

The problem with that then becomes that the caller to this function doesn't know
how many pages there are total, so actually using the page argument becomes a
bit difficult.
This was also based off the Mac renderer interface, which had a hardcoded page 1
as well.
It also makes sense to rasterize a whole document, since for testing purposes,
we're concerned about whole-document accuracy. Though since (I think) no GMs
actually outputs multi-page PDFs, I don't think that code has been necessary so
far.

https://codereview.chromium.org/20220002/diff/21001/third_party/fontconfig/li...
File third_party/fontconfig/linux/config.h (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/fontconfig/li...
third_party/fontconfig/linux/config.h:1: /* config.h.  Generated from
config.h.in by configure.  */
On 2013/08/01 17:49:51, vandebo wrote:
> It looks like this is pretty similar to the mac one.  Maybe this should get
> stripped down to the common ones, and then the rest of the #defines put into
the
> gyp file?

Anyways, I just nuked the entire linux folder, since we don't plan on building
fontconfig on Linux.
However, had it not been nuked, it would seem cleaner to keep everything in a
config file, rather than half in a config file and half in a .gyp file.

https://codereview.chromium.org/20220002/diff/21001/third_party/iconv/global/...
File third_party/iconv/global/aliases.h (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/iconv/global/...
third_party/iconv/global/aliases.h:1: /* ANSI-C code produced by gperf version
3.0.4 */
On 2013/08/01 17:49:51, vandebo wrote:
> If these aren't platform specific, can they be programmatically generated in
the
> gyp file?

I'm not sure how to do that. Is it really worth the work to get this
programmatically generated? It's only going to compile under Windows (so there's
no cross-platform benefit), and it works fine...

https://codereview.chromium.org/20220002/diff/21001/third_party/poppler/READM...
File third_party/poppler/README.chromium (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/poppler/READM...
third_party/poppler/README.chromium:10: A PDF rendering library, currently used
in the Skia gm for regression-testing PDF output.
On 2013/08/01 17:49:51, vandebo wrote:
> You might want to add here that it is and should only be used for testing
code.

Done.

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.cpp:12: #endif
I have no clue what I'm doing here, but I saw this as a fix to a similar problem
elsewhere in the Skia codebase. Something about exceptions...

[vandebo (ex-Chrome), 2013-08-02 15:48:54]

https://codereview.chromium.org/20220002/diff/21001/third_party/fontconfig/li...
File third_party/fontconfig/linux/config.h (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/fontconfig/li...
third_party/fontconfig/linux/config.h:1: /* config.h.  Generated from
config.h.in by configure.  */
On 2013/08/01 20:40:23, ducky wrote:
> On 2013/08/01 17:49:51, vandebo wrote:
> > It looks like this is pretty similar to the mac one.  Maybe this should get
> > stripped down to the common ones, and then the rest of the #defines put into
> the
> > gyp file?
> 
> Anyways, I just nuked the entire linux folder, since we don't plan on building
> fontconfig on Linux.
> However, had it not been nuked, it would seem cleaner to keep everything in a
> config file, rather than half in a config file and half in a .gyp file.

The point was a more general one.  Please compare the platform specific files to
see if any of them are substantially the same.  If there are just minor
differences between any of them and it is easy to combine them using the tools
available in gyp, then it might be worthwhile.

https://codereview.chromium.org/20220002/diff/21001/third_party/iconv/global/...
File third_party/iconv/global/aliases.h (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/iconv/global/...
third_party/iconv/global/aliases.h:1: /* ANSI-C code produced by gperf version
3.0.4 */
On 2013/08/01 20:40:23, ducky wrote:
> On 2013/08/01 17:49:51, vandebo wrote:
> > If these aren't platform specific, can they be programmatically generated in
> the
> > gyp file?
> 
> I'm not sure how to do that. Is it really worth the work to get this
> programmatically generated? It's only going to compile under Windows (so
there's
> no cross-platform benefit), and it works fine...

This too was a more general comment.  All of the files in "global" directories
probably should just be generated through the build system.

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.cpp:12: #endif
On 2013/08/01 20:40:23, ducky wrote:
> I have no clue what I'm doing here, but I saw this as a fix to a similar
problem
> elsewhere in the Skia codebase. Something about exceptions...

What code is using exceptions?

https://codereview.chromium.org/20220002/diff/49001/third_party/fontconfig/RE...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/49001/third_party/fontconfig/RE...
third_party/fontconfig/README.chromium:14: Platform-specific configuration files
added to third_party/fontconfig.
This file should clearly state how the checked in files where generated so that
when we want to update this code, someone doesn't have to figure it out all over
again.

https://codereview.chromium.org/20220002/diff/49001/third_party/fontconfig/li...
File third_party/fontconfig/linux/config.h (right):

https://codereview.chromium.org/20220002/diff/49001/third_party/fontconfig/li...
third_party/fontconfig/linux/config.h:1: /* config.h.  Generated from
config.h.in by configure.  */
You where going to remove these files.

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
File third_party/poppler/linux/poppler-version.h (right):

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
third_party/poppler/linux/poppler-version.h:1: /*
For example, this appears to be the same on all three platforms.

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
third_party/poppler/linux/poppler-version.h:24: #define POPPLER_VERSION "0.22.5"
And it needs to be updated if we update the version being pulled in.  If it was
generated as part of the build process, we wouldn't have to worry about it at
all.

[ducky, 2013-08-02 22:32:50]

Simplified the poppler config files. The rest of them either depend on
configure, the just-as-illegible products of configure, or tools (like gperf)
that aren't readily available on all three platforms.

Also added a gm OS guard to limit poppler to supported builds and a define to
tell the code when poppler exists.

https://codereview.chromium.org/20220002/diff/21001/third_party/fontconfig/li...
File third_party/fontconfig/linux/config.h (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/fontconfig/li...
third_party/fontconfig/linux/config.h:1: /* config.h.  Generated from
config.h.in by configure.  */
On 2013/08/02 15:48:54, vandebo wrote:
> On 2013/08/01 20:40:23, ducky wrote:
> > On 2013/08/01 17:49:51, vandebo wrote:
> > > It looks like this is pretty similar to the mac one.  Maybe this should
get
> > > stripped down to the common ones, and then the rest of the #defines put
into
> > the
> > > gyp file?
> > 
> > Anyways, I just nuked the entire linux folder, since we don't plan on
building
> > fontconfig on Linux.
> > However, had it not been nuked, it would seem cleaner to keep everything in
a
> > config file, rather than half in a config file and half in a .gyp file.
> 
> The point was a more general one.  Please compare the platform specific files
to
> see if any of them are substantially the same.  If there are just minor
> differences between any of them and it is easy to combine them using the tools
> available in gyp, then it might be worthwhile.

The Mac and Windows configs seem to have significant differences, I don't think
it would be worth the complexity to move it elsewhere.

https://codereview.chromium.org/20220002/diff/21001/third_party/iconv/global/...
File third_party/iconv/global/aliases.h (right):

https://codereview.chromium.org/20220002/diff/21001/third_party/iconv/global/...
third_party/iconv/global/aliases.h:1: /* ANSI-C code produced by gperf version
3.0.4 */
On 2013/08/02 15:48:54, vandebo wrote:
> On 2013/08/01 20:40:23, ducky wrote:
> > On 2013/08/01 17:49:51, vandebo wrote:
> > > If these aren't platform specific, can they be programmatically generated
in
> > the
> > > gyp file?
> > 
> > I'm not sure how to do that. Is it really worth the work to get this
> > programmatically generated? It's only going to compile under Windows (so
> there's
> > no cross-platform benefit), and it works fine...
> 
> This too was a more general comment.  All of the files in "global" directories
> probably should just be generated through the build system.

I looked more into this. According to the comment at the top of the file, it's
generated by gperf, which probably means it won't be easy to make
cross-platform. Furthermore, these files were copied from the release
distribution, and are not available in the git repo. It's created in
./autogen.sh, which is likely a even larger cross-platform headache.
This applies for all the iconv/global files.

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.cpp:12: #endif
On 2013/08/02 15:48:55, vandebo wrote:
> On 2013/08/01 20:40:23, ducky wrote:
> > I have no clue what I'm doing here, but I saw this as a fix to a similar
> problem
> > elsewhere in the Skia codebase. Something about exceptions...
> 
> What code is using exceptions?
> 
I think it's something internal to poppler. The MSVC warning wasn't helpful at
all, telling me there was an error in one of the MSVC headers, so it's very hard
to pinpoint the bug. This seems to work, and seems to be part of gmmain as
well...

https://codereview.chromium.org/20220002/diff/49001/third_party/fontconfig/RE...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/49001/third_party/fontconfig/RE...
third_party/fontconfig/README.chromium:14: Platform-specific configuration files
added to third_party/fontconfig.
On 2013/08/02 15:48:55, vandebo wrote:
> This file should clearly state how the checked in files where generated so
that
> when we want to update this code, someone doesn't have to figure it out all
over
> again.

Done.

https://codereview.chromium.org/20220002/diff/49001/third_party/fontconfig/li...
File third_party/fontconfig/linux/config.h (right):

https://codereview.chromium.org/20220002/diff/49001/third_party/fontconfig/li...
third_party/fontconfig/linux/config.h:1: /* config.h.  Generated from
config.h.in by configure.  */
On 2013/08/02 15:48:55, vandebo wrote:
> You where going to remove these files.

I was. I don't know why they're still here. They should be gone now.

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
File third_party/poppler/linux/poppler-version.h (right):

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
third_party/poppler/linux/poppler-version.h:1: /*
On 2013/08/02 15:48:55, vandebo wrote:
> For example, this appears to be the same on all three platforms.

For some reason, diff showed these as different, even though they are the same.
I've moved this into global now.

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
third_party/poppler/linux/poppler-version.h:24: #define POPPLER_VERSION "0.22.5"
On 2013/08/02 15:48:55, vandebo wrote:
> And it needs to be updated if we update the version being pulled in.  If it
was
> generated as part of the build process, we wouldn't have to worry about it at
> all.

This file turns out to be generated from a template as part of the make process,
but the makefile that does it was as illegible as the configure scripts...

[vandebo (ex-Chrome), 2013-08-05 17:46:30]

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.cpp:12: #endif
On 2013/08/02 22:32:50, ducky wrote:
> On 2013/08/02 15:48:55, vandebo wrote:
> > On 2013/08/01 20:40:23, ducky wrote:
> > > I have no clue what I'm doing here, but I saw this as a fix to a similar
> > problem
> > > elsewhere in the Skia codebase. Something about exceptions...
> > 
> > What code is using exceptions?
> > 
> I think it's something internal to poppler. The MSVC warning wasn't helpful at
> all, telling me there was an error in one of the MSVC headers, so it's very
hard
> to pinpoint the bug. This seems to work, and seems to be part of gmmain as
> well...

You should look at the code to understand how exceptions are used.  It might be
important to the functionality.

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
File third_party/poppler/linux/poppler-version.h (right):

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
third_party/poppler/linux/poppler-version.h:24: #define POPPLER_VERSION "0.22.5"
On 2013/08/02 22:32:50, ducky wrote:
> On 2013/08/02 15:48:55, vandebo wrote:
> > And it needs to be updated if we update the version being pulled in.  If it
> was
> > generated as part of the build process, we wouldn't have to worry about it
at
> > all.
> 
> This file turns out to be generated from a template as part of the make
process,
> but the makefile that does it was as illegible as the configure scripts...

This may show how it's being generated:
rm path/poppler-config.h ; V=1 make path/poppler-config.h  
set -x in the shell before the make command may also help.

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/RE...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/RE...
third_party/fontconfig/README.chromium:15: 
All the readme's should have instructions on how the config files where
generated.

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/gl...
File third_party/fontconfig/global/fc-case/fccase.h (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/gl...
third_party/fontconfig/global/fc-case/fccase.h:1: /*
I think a better directory layout would be to change global to config and put
the platform specific directories within there:

config/
config/linux/
config/mac/
config/win/

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/wi...
File third_party/fontconfig/windows/MSVC++2010-README (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/wi...
third_party/fontconfig/windows/MSVC++2010-README:1: Copyright
I think this should go in the lop level directory as LICENSE.win

https://codereview.chromium.org/20220002/diff/76001/third_party/poppler/READM...
File third_party/poppler/README.chromium (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/poppler/READM...
third_party/poppler/README.chromium:23: Additionally, these files are generated
during the make step:
You should be able to make this with a gyp rule.

[ducky, 2013-08-05 20:43:48]

Documentation updated.

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/49001/src/utils/SkPDFRasterizer...
src/utils/SkPDFRasterizer.cpp:12: #endif
On 2013/08/05 17:46:30, vandebo wrote:
> On 2013/08/02 22:32:50, ducky wrote:
> > On 2013/08/02 15:48:55, vandebo wrote:
> > > On 2013/08/01 20:40:23, ducky wrote:
> > > > I have no clue what I'm doing here, but I saw this as a fix to a similar
> > > problem
> > > > elsewhere in the Skia codebase. Something about exceptions...
> > > 
> > > What code is using exceptions?
> > > 
> > I think it's something internal to poppler. The MSVC warning wasn't helpful
at
> > all, telling me there was an error in one of the MSVC headers, so it's very
> hard
> > to pinpoint the bug. This seems to work, and seems to be part of gmmain as
> > well...
> 
> You should look at the code to understand how exceptions are used.  It might
be
> important to the functionality.

The error message is super-unintuitive, but it looks like it might be because
poppler uses STL, and STL throws exceptions.

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
File third_party/poppler/linux/poppler-version.h (right):

https://codereview.chromium.org/20220002/diff/49001/third_party/poppler/linux...
third_party/poppler/linux/poppler-version.h:24: #define POPPLER_VERSION "0.22.5"
On 2013/08/05 17:46:30, vandebo wrote:
> On 2013/08/02 22:32:50, ducky wrote:
> > On 2013/08/02 15:48:55, vandebo wrote:
> > > And it needs to be updated if we update the version being pulled in.  If
it
> > was
> > > generated as part of the build process, we wouldn't have to worry about it
> at
> > > all.
> > 
> > This file turns out to be generated from a template as part of the make
> process,
> > but the makefile that does it was as illegible as the configure scripts...
> 
> This may show how it's being generated:
> rm path/poppler-config.h ; V=1 make path/poppler-config.h  
> set -x in the shell before the make command may also help.

Did that and figured out that this wasn't generated by make. It's generated by
configure.

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/RE...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/RE...
third_party/fontconfig/README.chromium:15: 
On 2013/08/05 17:46:31, vandebo wrote:
> All the readme's should have instructions on how the config files where
> generated.

Done.

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/gl...
File third_party/fontconfig/global/fc-case/fccase.h (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/gl...
third_party/fontconfig/global/fc-case/fccase.h:1: /*
On 2013/08/05 17:46:31, vandebo wrote:
> I think a better directory layout would be to change global to config and put
> the platform specific directories within there:
> 
> config/
> config/linux/
> config/mac/
> config/win/

I actually kind of like having a clean base directory and completely isolating
the global configs from the platform configs...

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/wi...
File third_party/fontconfig/windows/MSVC++2010-README (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/wi...
third_party/fontconfig/windows/MSVC++2010-README:1: Copyright
On 2013/08/05 17:46:31, vandebo wrote:
> I think this should go in the lop level directory as LICENSE.win

Done.

https://codereview.chromium.org/20220002/diff/76001/third_party/poppler/READM...
File third_party/poppler/README.chromium (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/poppler/READM...
third_party/poppler/README.chromium:23: Additionally, these files are generated
during the make step:
On 2013/08/05 17:46:31, vandebo wrote:
> You should be able to make this with a gyp rule.

Turns out this was wrong, it's generated during configure. The differences
between the release tarball and the source repo got me confused.

[vandebo (ex-Chrome), 2013-08-07 17:36:43]

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/gl...
File third_party/fontconfig/global/fc-case/fccase.h (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/gl...
third_party/fontconfig/global/fc-case/fccase.h:1: /*
On 2013/08/05 20:43:49, ducky wrote:
> On 2013/08/05 17:46:31, vandebo wrote:
> > I think a better directory layout would be to change global to config and
put
> > the platform specific directories within there:
> > 
> > config/
> > config/linux/
> > config/mac/
> > config/win/
> 
> I actually kind of like having a clean base directory and completely isolating
> the global configs from the platform configs...

What I suggested is more in line with file layouts in the relevant trees.  In
particular global has no meaning, it's the platform-agnostic config files, so it
should go in a config directory.  Once you have that, the platform specific
files should go in subdirectories.

https://codereview.chromium.org/20220002/diff/111001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/111001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:5: License: MIT, LGPL (Windows)
nit: , -> and.  , often means a choice of licenses.

https://codereview.chromium.org/20220002/diff/111001/third_party/iconv/README...
File third_party/iconv/README.chromium (right):

https://codereview.chromium.org/20220002/diff/111001/third_party/iconv/README...
third_party/iconv/README.chromium:19: These files were created from the header
templates using instructions from
http://www.codeproject.com/Articles/302012/How-to-Build-libiconv-with-Microso...
(though, because of the version differences, not all instructions apply).
Can you list the instructions here?  Ideally these files will tell someone how
to regenerate all of this with minimal effort.

[ducky, 2013-08-08 00:43:19]

More cleanup of the READMEs

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/gl...
File third_party/fontconfig/global/fc-case/fccase.h (right):

https://codereview.chromium.org/20220002/diff/76001/third_party/fontconfig/gl...
third_party/fontconfig/global/fc-case/fccase.h:1: /*
On 2013/08/07 17:36:43, vandebo wrote:
> On 2013/08/05 20:43:49, ducky wrote:
> > On 2013/08/05 17:46:31, vandebo wrote:
> > > I think a better directory layout would be to change global to config and
> put
> > > the platform specific directories within there:
> > > 
> > > config/
> > > config/linux/
> > > config/mac/
> > > config/win/
> > 
> > I actually kind of like having a clean base directory and completely
isolating
> > the global configs from the platform configs...
> 
> What I suggested is more in line with file layouts in the relevant trees.  In
> particular global has no meaning, it's the platform-agnostic config files, so
it
> should go in a config directory.  Once you have that, the platform specific
> files should go in subdirectories.

Ok.

https://codereview.chromium.org/20220002/diff/111001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/111001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:5: License: MIT, LGPL (Windows)
On 2013/08/07 17:36:44, vandebo wrote:
> nit: , -> and.  , often means a choice of licenses.

Done.

https://codereview.chromium.org/20220002/diff/111001/third_party/iconv/README...
File third_party/iconv/README.chromium (right):

https://codereview.chromium.org/20220002/diff/111001/third_party/iconv/README...
third_party/iconv/README.chromium:19: These files were created from the header
templates using instructions from
http://www.codeproject.com/Articles/302012/How-to-Build-libiconv-with-Microso...
(though, because of the version differences, not all instructions apply).
On 2013/08/07 17:36:44, vandebo wrote:
> Can you list the instructions here?  Ideally these files will tell someone how
> to regenerate all of this with minimal effort.

Done.

[edisonn, 2013-08-08 20:14:16]

https://codereview.chromium.org/20220002/diff/131001/gyp/gm.gyp
File gyp/gm.gyp (right):

https://codereview.chromium.org/20220002/diff/131001/gyp/gm.gyp#newcode63
gyp/gm.gyp:63: ['skia_os in ["linux", "mac", "win"]', {
maybe it is better to have one variable has_plopper to be defines, common
variables gyp, and use it everywhere we need, otherwise if we add a new OS we
will need to update multiple gyp files

https://codereview.chromium.org/20220002/diff/131001/gyp/poppler.gyp
File gyp/poppler.gyp (right):

https://codereview.chromium.org/20220002/diff/131001/gyp/poppler.gyp#newcode133
gyp/poppler.gyp:133: '/usr/include/freetype2',
are we assured freetype is allways in this path.

It might be a nightmare for a developer without sudo to make the lib compile
without. Can we find an option that does not depend on standard paths? we can
have a variable with a default that can be override at build type

https://codereview.chromium.org/20220002/diff/131001/src/utils/SkPDFRasterize...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/131001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:22: bool SkPopplerRasterizePDF(SkStream* pdf,
SkBitmap* output) {
instead of a function pointer, what about an abstract class?

Pd viewer can render on gpu, or raster, and it would need a config too.

https://codereview.chromium.org/20220002/diff/131001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:47: bitmap.allocPixels();
check alloc pixels, we will crash here if we fail

[ducky, 2013-08-08 21:35:03]

https://codereview.chromium.org/20220002/diff/131001/gyp/gm.gyp
File gyp/gm.gyp (right):

https://codereview.chromium.org/20220002/diff/131001/gyp/gm.gyp#newcode63
gyp/gm.gyp:63: ['skia_os in ["linux", "mac", "win"]', {
On 2013/08/08 20:14:17, edisonn wrote:
> maybe it is better to have one variable has_plopper to be defines, common
> variables gyp, and use it everywhere we need, otherwise if we add a new OS we
> will need to update multiple gyp files

I think that if you add another OS, you need to update all the gypfiles anyways
to make sure the platform-specific config files and defines are correct. Short
of having supported OSes propagate up from the library gypfiles (this isn't a
supported feature, is it?), I don't think there is a good DRY way to express
what platforms poppler builds on.

https://codereview.chromium.org/20220002/diff/131001/gyp/poppler.gyp
File gyp/poppler.gyp (right):

https://codereview.chromium.org/20220002/diff/131001/gyp/poppler.gyp#newcode133
gyp/poppler.gyp:133: '/usr/include/freetype2',
On 2013/08/08 20:14:17, edisonn wrote:
> are we assured freetype is allways in this path.
> 
> It might be a nightmare for a developer without sudo to make the lib compile
> without. Can we find an option that does not depend on standard paths? we can
> have a variable with a default that can be override at build type

So I think the proper way to do this is to invoke freetype-config and get the
required c flags. I'm also how sure how standard this is on other Linux distros.
As an alternative, we can build all the libraries from scratch, especially since
gypfiles already exist for freetype, fontconfig, and their dependencies. Only
minimal effort should be required to re-enable the Linux builds. Thoughts?

https://codereview.chromium.org/20220002/diff/131001/src/utils/SkPDFRasterize...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/131001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:22: bool SkPopplerRasterizePDF(SkStream* pdf,
SkBitmap* output) {
On 2013/08/08 20:14:17, edisonn wrote:
> instead of a function pointer, what about an abstract class?
> 
> Pd viewer can render on gpu, or raster, and it would need a config too.

This was originally designed as a drop-in replacement for the Mac renderer, and
keeps the same input/output types.

I still think that this is a fairly clean and minimal interface, though.
Multiple configs can still be handled in gmmain using a function that
essentially does this, but with the native viewer code (so you would have
adaptors like native_render_gpu, native_render_raster).

I'm less sure about using an abstract class, since it would require some
unintuitive methods (like AbstractPDFRenderer::next(), or return an array of
bitmaps with descriptions), nor would you be able to select particular configs
to run in the gmtool without significant added complexity.

https://codereview.chromium.org/20220002/diff/131001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:47: bitmap.allocPixels();
On 2013/08/08 20:14:17, edisonn wrote:
> check alloc pixels, we will crash here if we fail

Done.

[Lei Zhang, 2013-08-08 21:47:55]

https://codereview.chromium.org/20220002/diff/131001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/20220002/diff/131001/gyp/freetype.gyp#newcode90
gyp/freetype.gyp:90: '../third_party/externals/freetype/src/pcf/pcf.c',
alphabetical in each section. I take it the sections are separated for some
logical reason?

https://codereview.chromium.org/20220002/diff/131001/gyp/iconv.gyp
File gyp/iconv.gyp (right):

https://codereview.chromium.org/20220002/diff/131001/gyp/iconv.gyp#newcode27
gyp/iconv.gyp:27: ['skia_os == "win"', {
I'm a bit inclined to say remove this. Since this file is meant to be used on
Windows only, just make it obviously broken for non-Windows.

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:6: License (Windows-specific files): LGPL
I'm pretty sure having this on a separate line will confuse the file parser.

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:7: License File: LICENSE / NOT_SHIPPED
just NOT_SHIPPED

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:11: Dependency of libpoppler.
Real description please.

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
File third_party/iconv/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:5: License: Libraries under LGPL, program
under GPL
Are you even checking out / building the "program" portion?

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:6: License File: LICENSE / NOT_SHIPPED
This should just say NOT_SHIPPED.

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:10: Dependency of libpoppler
Put in a real description of what libiconv is. It's clearly an icon vector
library, right?

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:14: Platform-specific configuration files
added to third_party/iconv/config/(platform):
Please try to observe the 80 col rule here too.

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:14: Platform-specific configuration files
added to third_party/iconv/config/(platform):
s/(platform)/windows/

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:28: 3. Modify localcharset.h as follows:
You may want to just include a .patch file and say "apply foo.patch".

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:66: These files were pulled from a release
tarball.
Put this closer to line 59. At first I thought you had to run some tool to
auto-generate them, then I realized where they actually came from.

Mention the release version of the tarball.

https://codereview.chromium.org/20220002/diff/131001/third_party/poppler/READ...
File third_party/poppler/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/poppler/READ...
third_party/poppler/README.chromium:4: Version: poppler-0.22.5 (upstream Git
repo tag)
Just 0.22.5 should be sufficient. You can describe it being the "poppler-0.22.5"
git repo tag below.

Ditto for the other README.chromium files.

https://codereview.chromium.org/20220002/diff/131001/third_party/poppler/READ...
third_party/poppler/README.chromium:11: This should ONLY be used for tesing
code.
typo

[ducky, 2013-08-09 22:14:57]

More style fixes!

https://codereview.chromium.org/20220002/diff/131001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/20220002/diff/131001/gyp/freetype.gyp#newcode90
gyp/freetype.gyp:90: '../third_party/externals/freetype/src/pcf/pcf.c',
On 2013/08/08 21:47:56, Lei Zhang wrote:
> alphabetical in each section. I take it the sections are separated for some
> logical reason?

These were ordered in the order make built them. I think there was a reason for
the spaces, but I don't remember. It looks like most of blocks 1 & 2 are font
drivers, so it's one big happy party now.

https://codereview.chromium.org/20220002/diff/131001/gyp/iconv.gyp
File gyp/iconv.gyp (right):

https://codereview.chromium.org/20220002/diff/131001/gyp/iconv.gyp#newcode27
gyp/iconv.gyp:27: ['skia_os == "win"', {
On 2013/08/08 21:47:56, Lei Zhang wrote:
> I'm a bit inclined to say remove this. Since this file is meant to be used on
> Windows only, just make it obviously broken for non-Windows.

I think this is a bit more obvious, and if people need to port this to something
other than Windows, exactly what needs to be done is already there.

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:6: License (Windows-specific files): LGPL
On 2013/08/08 21:47:56, Lei Zhang wrote:
> I'm pretty sure having this on a separate line will confuse the file parser.

I don't know a better way to do this. The , means choice of licenses, so that's
not a good option either. How do you include platform-specific licenses or
licenses for certain files?

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:7: License File: LICENSE / NOT_SHIPPED
On 2013/08/08 21:47:56, Lei Zhang wrote:
> just NOT_SHIPPED

Done.

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:11: Dependency of libpoppler.
On 2013/08/08 21:47:56, Lei Zhang wrote:
> Real description please.

Done. Though it's really a black box to me as well, since I really just care
about making libpoppler build everywhere.

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
File third_party/iconv/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:5: License: Libraries under LGPL, program
under GPL
On 2013/08/08 21:47:56, Lei Zhang wrote:
> Are you even checking out / building the "program" portion?

The program portion should not be built. But I think it's included in the
repository that gets checked out.

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:6: License File: LICENSE / NOT_SHIPPED
On 2013/08/08 21:47:56, Lei Zhang wrote:
> This should just say NOT_SHIPPED.

Done.

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:10: Dependency of libpoppler
On 2013/08/08 21:47:56, Lei Zhang wrote:
> Put in a real description of what libiconv is. It's clearly an icon vector
> library, right?

Done.
Depends on how much character encodings are related to vector icons - I'm sure
text characters can be described as vector icons...

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:14: Platform-specific configuration files
added to third_party/iconv/config/(platform):
On 2013/08/08 21:47:56, Lei Zhang wrote:
> s/(platform)/windows/

???

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:14: Platform-specific configuration files
added to third_party/iconv/config/(platform):
On 2013/08/08 21:47:56, Lei Zhang wrote:
> Please try to observe the 80 col rule here too.

Done

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:28: 3. Modify localcharset.h as follows:
On 2013/08/08 21:47:56, Lei Zhang wrote:
> You may want to just include a .patch file and say "apply foo.patch".

The files are pre-included, the instructions are only if people want to repeat
the process for a later version. That should be a bit more clear now.

https://codereview.chromium.org/20220002/diff/131001/third_party/poppler/READ...
File third_party/poppler/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/poppler/READ...
third_party/poppler/README.chromium:4: Version: poppler-0.22.5 (upstream Git
repo tag)
On 2013/08/08 21:47:56, Lei Zhang wrote:
> Just 0.22.5 should be sufficient. You can describe it being the
"poppler-0.22.5"
> git repo tag below.
> 
> Ditto for the other README.chromium files.

Done, fixed across the board. I think the DEPS file should be enough for people
to find the git repo tag? Or pattern match the version against the available
tags...

https://codereview.chromium.org/20220002/diff/131001/third_party/poppler/READ...
third_party/poppler/README.chromium:11: This should ONLY be used for tesing
code.
On 2013/08/08 21:47:56, Lei Zhang wrote:
> typo

Done.

[Lei Zhang, 2013-08-09 22:26:48]

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:6: License (Windows-specific files): LGPL
On 2013/08/09 22:14:57, ducky wrote:
> On 2013/08/08 21:47:56, Lei Zhang wrote:
> > I'm pretty sure having this on a separate line will confuse the file parser.
> 
> I don't know a better way to do this. The , means choice of licenses, so
that's
> not a good option either. How do you include platform-specific licenses or
> licenses for certain files?

Just write on 1 line:

License: MIT, except for Windows-specific files which are LGPL

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
File third_party/iconv/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:14: Platform-specific configuration files
added to third_party/iconv/config/(platform):
On 2013/08/09 22:14:57, ducky wrote:
> On 2013/08/08 21:47:56, Lei Zhang wrote:
> > s/(platform)/windows/
> 
> ???

As in:

Platform-specific configuration files added to third_party/iconv/config/windows/

https://codereview.chromium.org/20220002/diff/170001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/170001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:11: From their site: a library for
configuring and customizing font access.
drop the "From their site"

[ducky, 2013-08-12 22:05:10]

Readme fixes

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:6: License (Windows-specific files): LGPL
On 2013/08/09 22:26:48, Lei Zhang wrote:
> On 2013/08/09 22:14:57, ducky wrote:
> > On 2013/08/08 21:47:56, Lei Zhang wrote:
> > > I'm pretty sure having this on a separate line will confuse the file
parser.
> > 
> > I don't know a better way to do this. The , means choice of licenses, so
> that's
> > not a good option either. How do you include platform-specific licenses or
> > licenses for certain files?
> 
> Just write on 1 line:
> 
> License: MIT, except for Windows-specific files which are LGPL

Done.

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
File third_party/iconv/README.chromium (right):

https://codereview.chromium.org/20220002/diff/131001/third_party/iconv/README...
third_party/iconv/README.chromium:14: Platform-specific configuration files
added to third_party/iconv/config/(platform):
On 2013/08/09 22:26:48, Lei Zhang wrote:
> On 2013/08/09 22:14:57, ducky wrote:
> > On 2013/08/08 21:47:56, Lei Zhang wrote:
> > > s/(platform)/windows/
> > 
> > ???
> 
> As in:
> 
> Platform-specific configuration files added to
third_party/iconv/config/windows/

Done.

https://codereview.chromium.org/20220002/diff/170001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/170001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:11: From their site: a library for
configuring and customizing font access.
On 2013/08/09 22:26:48, Lei Zhang wrote:
> drop the "From their site"

Done.

[Lei Zhang, 2013-08-14 02:15:28]

No major concerns from me for the third party bits.

https://codereview.chromium.org/20220002/diff/182001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/182001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:29: Autogenerated files (consistent
across platforms) added to third_party/fontconfig/config:
80 col

https://codereview.chromium.org/20220002/diff/182001/third_party/poppler/READ...
File third_party/poppler/README.chromium (right):

https://codereview.chromium.org/20220002/diff/182001/third_party/poppler/READ...
third_party/poppler/README.chromium:32: On Windows, these are the files
generated with CMake and selecting a minimal
nit: blank line before this.

Does CMake have a "minimal feature set" checkbox? Is it trivial to select a
minimal feature set?

[ducky, 2013-08-14 02:27:01]

https://codereview.chromium.org/20220002/diff/182001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/20220002/diff/182001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:29: Autogenerated files (consistent
across platforms) added to third_party/fontconfig/config:
On 2013/08/14 02:15:28, Lei Zhang wrote:
> 80 col

Done.

https://codereview.chromium.org/20220002/diff/182001/third_party/poppler/READ...
File third_party/poppler/README.chromium (right):

https://codereview.chromium.org/20220002/diff/182001/third_party/poppler/READ...
third_party/poppler/README.chromium:32: On Windows, these are the files
generated with CMake and selecting a minimal
On 2013/08/14 02:15:28, Lei Zhang wrote:
> nit: blank line before this.
> 
> Does CMake have a "minimal feature set" checkbox? Is it trivial to select a
> minimal feature set?

No, there isn't. This is kind of just handwaving away some fudging (which I
don't remember anymore) to get CMake to spit out something useful, though the
strategy really was to disable as much stuff as possible.
If I remember correctly, I used the GUI version of CMake, and while there were
configuration options presented, part of it was also iteratively fixing
definitions until it was happy enough to output files.

[vandebo (ex-Chrome), 2013-08-14 15:41:37]

I was going to give this a last look before giving the ok, but I found a couple
more nits and questions.

https://codereview.chromium.org/20220002/diff/191001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/20220002/diff/191001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:73: 'FC_CACHEDIR='"/usr/local/var/cache/fontconfig"'',
The /usr/local prefix to these variable may be an artifact of you building it
yourself.

https://codereview.chromium.org/20220002/diff/191001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:91: '../third_party/externals/fontconfig/src/fccompat.c',
Are these two files needed on mac?

https://codereview.chromium.org/20220002/diff/191001/gyp/freetype.gyp
File gyp/freetype.gyp (left):

https://codereview.chromium.org/20220002/diff/191001/gyp/freetype.gyp#oldcode97
gyp/freetype.gyp:97: '../third_party/freetype/include_overrides',
Why are you changing the way this works?

https://codereview.chromium.org/20220002/diff/191001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/20220002/diff/191001/gyp/freetype.gyp#newcode95
gyp/freetype.gyp:95: '../third_party/externals/freetype/src/type42/type42.c',
Can poppler be configured to not use type42 or winfnt font support?  The only
font types that the PDF code generates is type1 and truetype (sfnt).

https://codereview.chromium.org/20220002/diff/191001/gyp/poppler.gyp
File gyp/poppler.gyp (right):

https://codereview.chromium.org/20220002/diff/191001/gyp/poppler.gyp#newcode38
gyp/poppler.gyp:38: '../third_party/externals/poppler/goo/ImgWriter.cc',
Do you use any of the writers?  Can they be excluded from the build?

https://codereview.chromium.org/20220002/diff/191001/gyp/poppler.gyp#newcode148
gyp/poppler.gyp:148: '$(SDKROOT)/usr/lib/libiconv.dylib',
is iconv needed here?  win only needs it for libpoppler-cpp-gpl

https://codereview.chromium.org/20220002/diff/191001/gyp/poppler.gyp#newcode178
gyp/poppler.gyp:178: '../third_party/externals/poppler/cpp/PNMWriter.cc',
Another writer

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:10: #pragma warning(push)
Don't indent these.

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:62: 255, r, g, b);
Are you dropping the alpha component?

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:62: 255, r, g, b);
nit: line wrap.

[ducky, 2013-08-14 20:43:17]

Yet more fixes...

https://codereview.chromium.org/20220002/diff/191001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/20220002/diff/191001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:73: 'FC_CACHEDIR='"/usr/local/var/cache/fontconfig"'',
On 2013/08/14 15:41:37, vandebo wrote:
> The /usr/local prefix to these variable may be an artifact of you building it
> yourself.

This seems to build (and work) fine with an empty FC_CAHCEDIR and
FONTCONFIG_PATH, so I'm thinking this might be "safer".

https://codereview.chromium.org/20220002/diff/191001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:91: '../third_party/externals/fontconfig/src/fccompat.c',
On 2013/08/14 15:41:37, vandebo wrote:
> Are these two files needed on mac?

The build breaks if they aren't included.

https://codereview.chromium.org/20220002/diff/191001/gyp/freetype.gyp
File gyp/freetype.gyp (left):

https://codereview.chromium.org/20220002/diff/191001/gyp/freetype.gyp#oldcode97
gyp/freetype.gyp:97: '../third_party/freetype/include_overrides',
On 2013/08/14 15:41:37, vandebo wrote:
> Why are you changing the way this works?

Good question. Reverted. I think it was because it wasn't necessary to build,
but since it doesn't break it either, it's back now.

https://codereview.chromium.org/20220002/diff/191001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/20220002/diff/191001/gyp/freetype.gyp#newcode95
gyp/freetype.gyp:95: '../third_party/externals/freetype/src/type42/type42.c',
On 2013/08/14 15:41:37, vandebo wrote:
> Can poppler be configured to not use type42 or winfnt font support?  The only
> font types that the PDF code generates is type1 and truetype (sfnt).

Not in any obvious way. I already disabled as many features as I could during
configure.

https://codereview.chromium.org/20220002/diff/191001/gyp/poppler.gyp
File gyp/poppler.gyp (right):

https://codereview.chromium.org/20220002/diff/191001/gyp/poppler.gyp#newcode38
gyp/poppler.gyp:38: '../third_party/externals/poppler/goo/ImgWriter.cc',
On 2013/08/14 15:41:37, vandebo wrote:
> Do you use any of the writers?  Can they be excluded from the build?

I don't think I use any of the writers, but it breaks the build if I remove it.

https://codereview.chromium.org/20220002/diff/191001/gyp/poppler.gyp#newcode148
gyp/poppler.gyp:148: '$(SDKROOT)/usr/lib/libiconv.dylib',
On 2013/08/14 15:41:37, vandebo wrote:
> is iconv needed here?  win only needs it for libpoppler-cpp-gpl

Turns out no. Might have been an artifact of when I tried to get this to
dynamically link.

https://codereview.chromium.org/20220002/diff/191001/gyp/poppler.gyp#newcode178
gyp/poppler.gyp:178: '../third_party/externals/poppler/cpp/PNMWriter.cc',
On 2013/08/14 15:41:37, vandebo wrote:
> Another writer

I don't think I use any of the writing features, but it breaks the build if I
remove it. I think this is referenced in the image class, which I do use.

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:10: #pragma warning(push)
On 2013/08/14 15:41:37, vandebo wrote:
> Don't indent these.

Done.

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:62: 255, r, g, b);
On 2013/08/14 15:41:37, vandebo wrote:
> nit: line wrap.

Done.

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:62: 255, r, g, b);
On 2013/08/14 15:41:37, vandebo wrote:
> Are you dropping the alpha component?

Yes. I tried adding back in the alpha, but the pictures look quite off if I
don't.

[vandebo (ex-Chrome), 2013-08-15 00:22:13]

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:62: 255, r, g, b);
On 2013/08/14 20:43:18, ducky wrote:
> On 2013/08/14 15:41:37, vandebo wrote:
> > Are you dropping the alpha component?
> 
> Yes. I tried adding back in the alpha, but the pictures look quite off if I
> don't.

This concerns me.  Can you try using this with a pdf that has alpha and see what
the values look like.  It may be a premultiplication vs not issue.

[ducky, 2013-08-15 01:19:11]

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/191001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:62: 255, r, g, b);
On 2013/08/15 00:22:13, vandebo wrote:
> On 2013/08/14 20:43:18, ducky wrote:
> > On 2013/08/14 15:41:37, vandebo wrote:
> > > Are you dropping the alpha component?
> > 
> > Yes. I tried adding back in the alpha, but the pictures look quite off if I
> > don't.
> 
> This concerns me.  Can you try using this with a pdf that has alpha and see
what
> the values look like.  It may be a premultiplication vs not issue.

Looks like I typo'd when unpacking my pixels...
SkDiff shows everything is unchanged now.

[vandebo (ex-Chrome), 2013-08-15 16:12:56]

https://codereview.chromium.org/20220002/diff/207001/gyp/freetype.gyp
File gyp/freetype.gyp (left):

https://codereview.chromium.org/20220002/diff/207001/gyp/freetype.gyp#oldcode86
gyp/freetype.gyp:86: '../third_party/freetype/include_overrides',
Apparently this is the way that skia does local config files - we should follow
this model with the other packages here.

https://codereview.chromium.org/20220002/diff/207001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/20220002/diff/207001/gyp/freetype.gyp#newcode121
gyp/freetype.gyp:121:
'../third_party/externals/freetype/include/freetype/config',
This is going to break color fonts.  Ideally, we should build two different
targets here.  I'm not sure if gyp supports doing that though.

https://codereview.chromium.org/20220002/diff/207001/src/utils/SkPDFRasterize...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/207001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:57: U8CPU a = (unsigned char)rowData[3];
push the casts up to line 55

https://codereview.chromium.org/20220002/diff/207001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:62: SkPMColor out = SkPreMultiplyARGB(a, r, g, b);
nit, skip out:
*bitmapPixels = SkPreMultiplyARGB(a, r, g, b);

[ducky, 2013-08-15 21:38:13]

Hopefully one step closer to ending the never-ending CL...

https://codereview.chromium.org/20220002/diff/207001/gyp/freetype.gyp
File gyp/freetype.gyp (left):

https://codereview.chromium.org/20220002/diff/207001/gyp/freetype.gyp#oldcode86
gyp/freetype.gyp:86: '../third_party/freetype/include_overrides',
On 2013/08/15 16:12:56, vandebo wrote:
> Apparently this is the way that skia does local config files - we should
follow
> this model with the other packages here.

Directory structure already consistent; for the other libraries I think config
is a more appropriate name.

https://codereview.chromium.org/20220002/diff/207001/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/20220002/diff/207001/gyp/freetype.gyp#newcode121
gyp/freetype.gyp:121:
'../third_party/externals/freetype/include/freetype/config',
On 2013/08/15 16:12:56, vandebo wrote:
> This is going to break color fonts.  Ideally, we should build two different
> targets here.  I'm not sure if gyp supports doing that though.

Separated into another build target.

https://codereview.chromium.org/20220002/diff/207001/src/utils/SkPDFRasterize...
File src/utils/SkPDFRasterizer.cpp (right):

https://codereview.chromium.org/20220002/diff/207001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:57: U8CPU a = (unsigned char)rowData[3];
On 2013/08/15 16:12:56, vandebo wrote:
> push the casts up to line 55

rowData changes with each inner loop iteration (rowData incremented by 4 on line
67) - this would break something.

https://codereview.chromium.org/20220002/diff/207001/src/utils/SkPDFRasterize...
src/utils/SkPDFRasterizer.cpp:62: SkPMColor out = SkPreMultiplyARGB(a, r, g, b);
On 2013/08/15 16:12:56, vandebo wrote:
> nit, skip out:
> *bitmapPixels = SkPreMultiplyARGB(a, r, g, b);

Done.

[vandebo (ex-Chrome), 2013-08-15 23:17:13]

https://codereview.chromium.org/20220002/diff/212006/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/20220002/diff/212006/gyp/freetype.gyp#newcode119
gyp/freetype.gyp:119: 'include_dirs': [
Pull the other common things into the gypi:

include_dirs, defines, direct_dependent_settings, conditions

https://codereview.chromium.org/20220002/diff/212006/gyp/freetype.gyp#newcode128
gyp/freetype.gyp:128: 'FT_CONFIG_CONFIG_H=<ftconfig.h>',
Why does the poppler version need these and the old version doesn't?

https://codereview.chromium.org/20220002/diff/212006/gyp/poppler.gyp
File gyp/poppler.gyp (right):

https://codereview.chromium.org/20220002/diff/212006/gyp/poppler.gyp#newcode140
gyp/poppler.gyp:140: 'dependencies': [
nit: pull dependencies into a mac or win section

[ducky, 2013-08-16 02:18:51]

Yet another attempt to end the never-ending CL...

https://codereview.chromium.org/20220002/diff/212006/gyp/freetype.gyp
File gyp/freetype.gyp (right):

https://codereview.chromium.org/20220002/diff/212006/gyp/freetype.gyp#newcode119
gyp/freetype.gyp:119: 'include_dirs': [
On 2013/08/15 23:17:14, vandebo wrote:
> Pull the other common things into the gypi:
> 
> include_dirs, defines, direct_dependent_settings, conditions

Done.

https://codereview.chromium.org/20220002/diff/212006/gyp/freetype.gyp#newcode128
gyp/freetype.gyp:128: 'FT_CONFIG_CONFIG_H=<ftconfig.h>',
On 2013/08/15 23:17:14, vandebo wrote:
> Why does the poppler version need these and the old version doesn't?

Good question. Apparently it doesn't.

https://codereview.chromium.org/20220002/diff/212006/gyp/poppler.gyp
File gyp/poppler.gyp (right):

https://codereview.chromium.org/20220002/diff/212006/gyp/poppler.gyp#newcode140
gyp/poppler.gyp:140: 'dependencies': [
On 2013/08/15 23:17:14, vandebo wrote:
> nit: pull dependencies into a mac or win section

Done.

[vandebo (ex-Chrome), 2013-08-19 23:25:18]

LGTM at long last

[commit-bot: I haz the power, 2013-08-20 01:25:33]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/20220002/237001

[commit-bot: I haz the power, 2013-08-20 01:32:04]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/20220002/237001

[commit-bot: I haz the power, 2013-08-20 03:07:48]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/20220002/237001

[commit-bot: I haz the power, 2013-08-20 03:23:41]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/20220002/232050

[commit-bot: I haz the power, 2013-08-20 03:36:42]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/20220002/237001

[commit-bot: I haz the power, 2013-08-20 16:32:11]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/20220002/237001

[commit-bot: I haz the power, 2013-08-20 16:34:58]

Change committed as 10823