Add an SKP to PDF rendered. test_pdfs.py will be hooked up in buildbot testing later.

Add an SKP to PDF rendered. test_pdfs.py will be hooked up in buildbot testing later.
Committed: https://code.google.com/p/skia/source/detail?r=5880

[epoger, 2012-10-05 20:02:05]

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.cpp
File tools/PdfRenderer.cpp (right):

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.cpp#newcode37
tools/PdfRenderer.cpp:37: SkISize pageSize = SkISize::Make(width, height);
changed indent here

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.cpp#newcode45
tools/PdfRenderer.cpp:45: SkDELETE(fPDFDevice);
does something bad happen if fPDFDevice was never set up before this is called?

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.h
File tools/PdfRenderer.h (right):

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.h#newcode7
tools/PdfRenderer.h:7: 
Somewhere in here, please add a couple of lines of comments describing what this
does.

https://codereview.appspot.com/6610056/diff/2001/tools/test_pdfs.py
File tools/test_pdfs.py (right):

https://codereview.appspot.com/6610056/diff/2001/tools/test_pdfs.py#newcode24
tools/test_pdfs.py:24: by input with the images in expectedDir. Note, files in
directoriers are
I'm confused.  Is each instance of "input" allowed to be EITHER an SkPicture
file, OR a directory within which the tool will read out all .skp files?

Do directory names need to end with .skp?

[edisonn, 2012-10-09 12:49:08]

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.cpp
File tools/PdfRenderer.cpp (right):

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.cpp#newcode37
tools/PdfRenderer.cpp:37: SkISize pageSize = SkISize::Make(width, height);
On 2012/10/05 20:02:05, epoger wrote:
> changed indent here

Done.

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.cpp#newcode45
tools/PdfRenderer.cpp:45: SkDELETE(fPDFDevice);
delete(null) should be just fine, but since we call SkDelete, there might be
some magic which could mess around ,if not now in the future.
On 2012/10/05 20:02:05, epoger wrote:
> does something bad happen if fPDFDevice was never set up before this is
called?

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.h
File tools/PdfRenderer.h (right):

https://codereview.appspot.com/6610056/diff/2001/tools/PdfRenderer.h#newcode7
tools/PdfRenderer.h:7: 
On 2012/10/05 20:02:05, epoger wrote:
> Somewhere in here, please add a couple of lines of comments describing what
this
> does.

Done.

https://codereview.appspot.com/6610056/diff/2001/tools/test_pdfs.py
File tools/test_pdfs.py (right):

https://codereview.appspot.com/6610056/diff/2001/tools/test_pdfs.py#newcode24
tools/test_pdfs.py:24: by input with the images in expectedDir. Note, files in
directoriers are
Works in the same way as test_pictures.py, from which I copied the pattern. Test
functions should be consistent. 

On 2012/10/05 20:02:05, epoger wrote:
> I'm confused.  Is each instance of "input" allowed to be EITHER an SkPicture
> file, OR a directory within which the tool will read out all .skp files?
Correct
> 
> Do directory names need to end with .skp?
No

[epoger, 2012-10-10 14:31:35]

LGTM

I have some formatting/documentation suggestions, but I approve this CL
regardless.

https://codereview.appspot.com/6610056/diff/2001/tools/test_pdfs.py
File tools/test_pdfs.py (right):

https://codereview.appspot.com/6610056/diff/2001/tools/test_pdfs.py#newcode24
tools/test_pdfs.py:24: by input with the images in expectedDir. Note, files in
directoriers are
On 2012/10/09 12:49:08, edisonn wrote:
> Works in the same way as test_pictures.py, from which I copied the pattern.
Test
> functions should be consistent. 

In that case, I would advocate for changing the description to be clearer (I
probably won't be the only one who finds this confusing).  Maybe something like
this?


Takes input SkPicture files and renders them as PDF files, and then compares
those
resulting PDF files against PDF files found in expectedDir.

Each instance of "input" can be either a file (name must end in .skp), or a
directory
(in which case this script will process all .skp files within the directory).

https://codereview.appspot.com/6610056/diff/10001/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.appspot.com/6610056/diff/10001/gyp/tools.gyp#newcode161
gyp/tools.gyp:161: 'target_name': 'pdf_renderer',
nit: this is indented 1 space instead of 2

https://codereview.appspot.com/6610056/diff/10001/gyp/tools.gyp#newcode171
gyp/tools.gyp:171: 'core.gyp:core',
indent is off here too

https://codereview.appspot.com/6610056/diff/10001/tools/test_pdfs.py
File tools/test_pdfs.py (right):

https://codereview.appspot.com/6610056/diff/10001/tools/test_pdfs.py#newcode36
tools/test_pdfs.py:36: help = ("specify the location to output the rendered
files."
Indentation is funny in here.  Our code will be easier to maintain if we follow
these style guides:

1. http://www.python.org/dev/peps/pep-0008/#indentation
2. apply any overrides from
http://www.corp.google.com/eng/doc/pyguide.xml#Indentation

(except in exceptional cases)

[edisonn, 2012-10-10 15:06:28]

https://codereview.appspot.com/6610056/diff/2001/tools/test_pdfs.py
File tools/test_pdfs.py (right):

https://codereview.appspot.com/6610056/diff/2001/tools/test_pdfs.py#newcode24
tools/test_pdfs.py:24: by input with the images in expectedDir. Note, files in
directoriers are
done; updated test_picures.py too in another cl
On 2012/10/10 14:31:36, epoger wrote:
> On 2012/10/09 12:49:08, edisonn wrote:
> > Works in the same way as test_pictures.py, from which I copied the pattern.
> Test
> > functions should be consistent. 
> 
> In that case, I would advocate for changing the description to be clearer (I
> probably won't be the only one who finds this confusing).  Maybe something
like
> this?
> 
> 
> Takes input SkPicture files and renders them as PDF files, and then compares
> those
> resulting PDF files against PDF files found in expectedDir.
> 
> Each instance of "input" can be either a file (name must end in .skp), or a
> directory
> (in which case this script will process all .skp files within the directory).

https://codereview.appspot.com/6610056/diff/10001/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.appspot.com/6610056/diff/10001/gyp/tools.gyp#newcode161
gyp/tools.gyp:161: 'target_name': 'pdf_renderer',
On 2012/10/10 14:31:36, epoger wrote:
> nit: this is indented 1 space instead of 2

Done.

https://codereview.appspot.com/6610056/diff/10001/gyp/tools.gyp#newcode171
gyp/tools.gyp:171: 'core.gyp:core',
On 2012/10/10 14:31:36, epoger wrote:
> indent is off here too

Done.

https://codereview.appspot.com/6610056/diff/10001/tools/test_pdfs.py
File tools/test_pdfs.py (right):

https://codereview.appspot.com/6610056/diff/10001/tools/test_pdfs.py#newcode36
tools/test_pdfs.py:36: help = ("specify the location to output the rendered
files."
On 2012/10/10 14:31:36, epoger wrote:
> Indentation is funny in here.  Our code will be easier to maintain if we
follow
> these style guides:
> 
> 1. http://www.python.org/dev/peps/pep-0008/#indentation
> 2. apply any overrides from
> http://www.corp.google.com/eng/doc/pyguide.xml#Indentation
> 
> (except in exceptional cases)

Done.

[epoger, 2012-10-10 15:13:20]

LGTM

Thanks for the clean-up!