PDF Device should report non-transformed size for width and height.

PDF Device should report non-transformed size for width and height.
Committed: http://code.google.com/p/skia/source/detail?r=1217

[Steve VanDeBogart, 2011-04-25 22:12:05]

http://codereview.appspot.com/4435074/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4435074/diff/1/include/pdf/SkPDFDevice.h#newcode49
include/pdf/SkPDFDevice.h:49: *  @param contentSize Content size in points.
The comment here needs to be much more verbose: The content size (in points),
will be combined with the initial transform to determine the drawing area (as
reported by the width and height methods).  Anything outside of the drawing area
will be clipped.

http://codereview.appspot.com/4435074/diff/1/include/pdf/SkPDFDevice.h#newcod...
include/pdf/SkPDFDevice.h:143: int fWidth;
I would change this to SkSize fPageSize to make it more clear.

http://codereview.appspot.com/4435074/diff/1/include/pdf/SkPDFDevice.h#newcod...
include/pdf/SkPDFDevice.h:145: int fClipWidth;
fClipX is too generic, there are several clips used through out the SkPDFDevice.
fContentSize too be consistent with the constructor? I would go with an SkSize
here as well.

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp#newcode135
src/pdf/SkPDFDevice.cpp:135: : SkDevice(NULL, makeABitmap(0, 0), false),
These need to be contentSize in order for one of the Debug checks to work
correctly.

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp#newcode139
src/pdf/SkPDFDevice.cpp:139: fClipWidth =
SkScalarRound(SkScalarDiv(contentSize.fWidth,
If initialTransform includes rotation (i.e. a 90 degree rotation), this will
yield the wrong result.  I think you want to use SkMatrix::MapVectors

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFShader.cpp#newcode501
src/pdf/SkPDFShader.cpp:501: SkSize box = SkSize::Make(surfaceBBox.fRight,
surfaceBBox.fBottom);
box -> size?

[Chris Guillory, 2011-04-25 23:12:19]

http://codereview.appspot.com/4435074/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4435074/diff/1/include/pdf/SkPDFDevice.h#newcode49
include/pdf/SkPDFDevice.h:49: *  @param contentSize Content size in points.
On 2011/04/25 22:12:05, Steve VanDeBogart wrote:
> The comment here needs to be much more verbose: The content size (in points),
> will be combined with the initial transform to determine the drawing area (as
> reported by the width and height methods).  Anything outside of the drawing
area
> will be clipped.

Done.

http://codereview.appspot.com/4435074/diff/1/include/pdf/SkPDFDevice.h#newcod...
include/pdf/SkPDFDevice.h:143: int fWidth;
On 2011/04/25 22:12:05, Steve VanDeBogart wrote:
> I would change this to SkSize fPageSize to make it more clear.

Done.

http://codereview.appspot.com/4435074/diff/1/include/pdf/SkPDFDevice.h#newcod...
include/pdf/SkPDFDevice.h:145: int fClipWidth;
On 2011/04/25 22:12:05, Steve VanDeBogart wrote:
> fClipX is too generic, there are several clips used through out the
SkPDFDevice.
> fContentSize too be consistent with the constructor? I would go with an SkSize
> here as well.

Done.

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp#newcode135
src/pdf/SkPDFDevice.cpp:135: : SkDevice(NULL, makeABitmap(0, 0), false),
On 2011/04/25 22:12:05, Steve VanDeBogart wrote:
> These need to be contentSize in order for one of the Debug checks to work
> correctly.
Ah, I changed this to see if I'd hit a DCHECK. Wouldn't this need to be the
unscaled contentSize?

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp#newcode139
src/pdf/SkPDFDevice.cpp:139: fClipWidth =
SkScalarRound(SkScalarDiv(contentSize.fWidth,
On 2011/04/25 22:12:05, Steve VanDeBogart wrote:
> If initialTransform includes rotation (i.e. a 90 degree rotation), this will
> yield the wrong result.  I think you want to use SkMatrix::MapVectors

Done.

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFShader.cpp#newcode501
src/pdf/SkPDFShader.cpp:501: SkSize box = SkSize::Make(surfaceBBox.fRight,
surfaceBBox.fBottom);
On 2011/04/25 22:12:05, Steve VanDeBogart wrote:
> box -> size?

Done.

[Steve VanDeBogart, 2011-04-26 02:20:19]

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp#newcode135
src/pdf/SkPDFDevice.cpp:135: : SkDevice(NULL, makeABitmap(0, 0), false),
On 2011/04/25 23:12:20, Chris Guillory wrote:
> On 2011/04/25 22:12:05, Steve VanDeBogart wrote:
> > These need to be contentSize in order for one of the Debug checks to work
> > correctly.
> Ah, I changed this to see if I'd hit a DCHECK. Wouldn't this need to be the
> unscaled contentSize?

No, it should be the same units as width() and height() - that's the "size" of
the device as far as the rest of Skia is concerned.  The page size and scale
factor needed to get it to 72dpi are internal to the PDF representation.

http://codereview.appspot.com/4435074/diff/5001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4435074/diff/5001/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:146: SkISize fPageSize;
Sorry, seeing this again, I realize that these two fields, which are on
subsequent lines, have different units.  So either their names should reflect
their units, or at the least, there should be comments.  However, I'm not sure
what to call the units of fContentSize.  fPageSize is obviously points, but
fContentSize is in scaled units...

http://codereview.appspot.com/4435074/diff/8001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4435074/diff/8001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:168: fGraphicStack[0].fClip.setRect(0,0,
fPageSize.fWidth, fPageSize.fHeight);
These should be fContentSize

http://codereview.appspot.com/4435074/diff/8001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

http://codereview.appspot.com/4435074/diff/8001/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:501: SkISize size = SkISize::Make(surfaceBBox.fRight,
surfaceBBox.fBottom);
I think this is a bug and should be width() and height() instead of fRight and
fBottom.

[Steve VanDeBogart, 2011-04-26 16:51:30]

I think you also need to update gm/gmmain.cpp

[Chris Guillory, 2011-04-27 02:59:34]

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4435074/diff/1/src/pdf/SkPDFDevice.cpp#newcode135
src/pdf/SkPDFDevice.cpp:135: : SkDevice(NULL, makeABitmap(0, 0), false),
On 2011/04/26 02:20:19, Steve VanDeBogart wrote:
> On 2011/04/25 23:12:20, Chris Guillory wrote:
> > On 2011/04/25 22:12:05, Steve VanDeBogart wrote:
> > > These need to be contentSize in order for one of the Debug checks to work
> > > correctly.
> > Ah, I changed this to see if I'd hit a DCHECK. Wouldn't this need to be the
> > unscaled contentSize?
> 
> No, it should be the same units as width() and height() - that's the "size" of
> the device as far as the rest of Skia is concerned.  The page size and scale
> factor needed to get it to 72dpi are internal to the PDF representation.

Done. Moving this logic into makeABitmap per discussion.

http://codereview.appspot.com/4435074/diff/5001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4435074/diff/5001/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:146: SkISize fPageSize;
On 2011/04/26 02:20:19, Steve VanDeBogart wrote:
> Sorry, seeing this again, I realize that these two fields, which are on
> subsequent lines, have different units.  So either their names should reflect
> their units, or at the least, there should be comments.  However, I'm not sure
> what to call the units of fContentSize.  fPageSize is obviously points, but
> fContentSize is in scaled units...
Removed this member.

http://codereview.appspot.com/4435074/diff/8001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4435074/diff/8001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:168: fGraphicStack[0].fClip.setRect(0,0,
fPageSize.fWidth, fPageSize.fHeight);
On 2011/04/26 02:20:19, Steve VanDeBogart wrote:
> These should be fContentSize

Calling width() and height() on SkDevice.

http://codereview.appspot.com/4435074/diff/8001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

http://codereview.appspot.com/4435074/diff/8001/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:501: SkISize size = SkISize::Make(surfaceBBox.fRight,
surfaceBBox.fBottom);
On 2011/04/26 02:20:19, Steve VanDeBogart wrote:
> I think this is a bug and should be width() and height() instead of fRight and
> fBottom.

Good catch. Done.

[Chris Guillory, 2011-04-27 03:00:16]

On 2011/04/26 16:51:30, Steve VanDeBogart wrote:
> I think you also need to update gm/gmmain.cpp
Updated. I'll also verify build on all platforms prior to submit.

[Steve VanDeBogart, 2011-04-27 18:39:14]

LGTM w/ nits.

http://codereview.appspot.com/4435074/diff/15001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4435074/diff/15001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:127: static inline SkBitmap makeABitmap(const SkISize&
contentSize,
nit: there might be a better name for this function now.

http://codereview.appspot.com/4435074/diff/15001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:169: fGraphicStack[0].fClip.setRect(0, 0, width(),
height());
nit: skia convention is this->method() for virtual method calls.  However, maybe
in a follow up CL we should change width() and height() back to non-virtual.

[reed1, 2011-04-27 18:50:10]

this->method() is good for virtual and non-virtual calls.

I agree that we should try to make width/height non-virtual if possible (but not
in this CL)

[Chris Guillory, 2011-04-27 19:29:37]

http://codereview.appspot.com/4435074/diff/15001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4435074/diff/15001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:127: static inline SkBitmap makeABitmap(const SkISize&
contentSize,
On 2011/04/27 18:39:15, Steve VanDeBogart wrote:
> nit: there might be a better name for this function now.

Done. makeContentBitmap.

http://codereview.appspot.com/4435074/diff/15001/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:169: fGraphicStack[0].fClip.setRect(0, 0, width(),
height());
On 2011/04/27 18:39:15, Steve VanDeBogart wrote:
> nit: skia convention is this->method() for virtual method calls.  However,
maybe
> in a follow up CL we should change width() and height() back to non-virtual.

Done.

[reed1, 2011-04-27 20:19:55]

How does gmmain.cpp compile? Isn't size duplicately defined?

[Chris Guillory, 2011-04-27 20:22:36]

It doesn't. I just encountered this trying to verify the build on linux.
I'll correct it.

On Wed, Apr 27, 2011 at 1:19 PM, <reed@google.com> wrote:

> How does gmmain.cpp compile? Isn't size duplicately defined?
>
>
> http://codereview.appspot.com/4435074/
>