[PDF] Change the way flip origin is done.

[PDF] Change the way flip origin is done.

The PDF shader matrix is dependent on flip origin or not, so we need to set it at device creation time.

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

[reed1, 2011-02-24 17:53:10]

http://codereview.appspot.com/4216046/diff/1005/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (left):

http://codereview.appspot.com/4216046/diff/1005/include/pdf/SkPDFDevice.h#old...
include/pdf/SkPDFDevice.h:50: SkPDFDevice(int width, int height);
Can we use an enum instead of a bool? We do think for the GPU, which has a
similar issue. e.g....

enum Orientation or PageOrigin {
    kTopLeft_Origin,
    kBottomLeft_origin,
   // or some other names
};

I just find bools hard to interpret at the call sites. Skia needs to adopt this
more as well.

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

http://codereview.appspot.com/4216046/diff/1005/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:124: return SkNEW_ARGS(SkPDFDevice, (width, height,
!isForLayer));
using an enum for your origin orientation will make this call site much clearer.

http://codereview.appspot.com/4216046/diff/1005/src/pdf/SkPDFFormXObject.cpp
File src/pdf/SkPDFFormXObject.cpp (right):

http://codereview.appspot.com/4216046/diff/1005/src/pdf/SkPDFFormXObject.cpp#...
src/pdf/SkPDFFormXObject.cpp:31: SkString content = device->content();
Unrelated to this CL, but are we getting a cheap copy of that string, or are we
making a deep copy? If so, perhaps the signature could be changed to (which
matches most of skia)

void content(SkString* str) const {....

[Steve VanDeBogart, 2011-02-24 21:43:11]

http://codereview.appspot.com/4216046/diff/1005/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (left):

http://codereview.appspot.com/4216046/diff/1005/include/pdf/SkPDFDevice.h#old...
include/pdf/SkPDFDevice.h:50: SkPDFDevice(int width, int height);
On 2011/02/24 17:53:10, reed1 wrote:
> Can we use an enum instead of a bool? We do think for the GPU, which has a
> similar issue. e.g....
> 
> enum Orientation or PageOrigin {
>     kTopLeft_Origin,
>     kBottomLeft_origin,
>    // or some other names
> };
> 
> I just find bools hard to interpret at the call sites. Skia needs to adopt
this
> more as well.

I changed to an enum, but the enum can't indicate where the origin is.  It can
only indicate if we want it flipped or not.

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

http://codereview.appspot.com/4216046/diff/1005/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:124: return SkNEW_ARGS(SkPDFDevice, (width, height,
!isForLayer));
On 2011/02/24 17:53:10, reed1 wrote:
> using an enum for your origin orientation will make this call site much
clearer.

Yup, cleaner now.

http://codereview.appspot.com/4216046/diff/1005/src/pdf/SkPDFFormXObject.cpp
File src/pdf/SkPDFFormXObject.cpp (right):

http://codereview.appspot.com/4216046/diff/1005/src/pdf/SkPDFFormXObject.cpp#...
src/pdf/SkPDFFormXObject.cpp:31: SkString content = device->content();
On 2011/02/24 17:53:10, reed1 wrote:
> Unrelated to this CL, but are we getting a cheap copy of that string, or are
we
> making a deep copy? If so, perhaps the signature could be changed to (which
> matches most of skia)
> 
> void content(SkString* str) const {....
> 

We make a deep copy so that we could continue appending to the page.  (The
assignment on return should be cheap though.)  However, all the call sites then
copy the result into an SkMemoryStream.  I'll send you a CL in a moment to
remove the extra copy.

[reed1, 2011-02-24 22:20:38]

LGTM

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

http://codereview.appspot.com/4216046/diff/7001/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:50: enum OriginTransform {
These names are fine. The skia naming pattern happens to be...

enum OriginTransform {
    kFlip_OriginTransform,
    kNoFlip_OriginTransform
};

[Steve VanDeBogart, 2011-02-24 23:14:34]

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

http://codereview.appspot.com/4216046/diff/7001/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:50: enum OriginTransform {
On 2011/02/24 22:20:39, reed1 wrote:
> These names are fine. The skia naming pattern happens to be...
> 
> enum OriginTransform {
>     kFlip_OriginTransform,
>     kNoFlip_OriginTransform
> };

Done.