[PDF] Fix some scalar/int assumptions.

[PDF] Fix some scalar/int assumptions.

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

[Steve VanDeBogart, 2012-01-05 00:11:05]

This doesn't fix all the PDF fixed gm's, but it's a good start.

[epoger, 2012-01-05 14:30:07]

L mostly G T M, but one major concern and a few suggestions.

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

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFDevice.cpp#newcode286
src/pdf/SkPDFDevice.cpp:286: const SkIPoint& translation) {
In the long term, I think it would be better to take SkPoint than SkIPoint as a
parameter so we don't have to worry about the later step.  If you agree, maybe
just add a TODO...

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFDevice.cpp#newcode485
src/pdf/SkPDFDevice.cpp:485: static inline SkBitmap makeContentBitmap(const
SkISize& contentSize,
[similar to my point above about SkIPoint]

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFDevice.cpp#newcode507
src/pdf/SkPDFDevice.cpp:507: SkPDFDevice::SkPDFDevice(const SkISize& pageSize,
const SkISize& contentSize,
[and again]

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

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:203: new
SkPDFScalar(SkScalarDiv(fPaint.getAlpha(), 0xFF));
I don't think this is correct... interpretation of 0xFF will vary depending on
whether SK_SCALAR_IS_FLOAT.  Right?

[Steve VanDeBogart, 2012-01-05 17:47:58]

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

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFDevice.cpp#newcode286
src/pdf/SkPDFDevice.cpp:286: const SkIPoint& translation) {
On 2012/01/05 14:30:08, epoger wrote:
> In the long term, I think it would be better to take SkPoint than SkIPoint as
a
> parameter so we don't have to worry about the later step.  If you agree, maybe
> just add a TODO...

Sounds good, fixed.

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFDevice.cpp#newcode485
src/pdf/SkPDFDevice.cpp:485: static inline SkBitmap makeContentBitmap(const
SkISize& contentSize,
On 2012/01/05 14:30:08, epoger wrote:
> [similar to my point above about SkIPoint]

To change this requires changing the SkPDFDevice constructor, which would be a
two sided commit to Chrome.  Added a todo.

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFDevice.cpp#newcode507
src/pdf/SkPDFDevice.cpp:507: SkPDFDevice::SkPDFDevice(const SkISize& pageSize,
const SkISize& contentSize,
On 2012/01/05 14:30:08, epoger wrote:
> [and again]

Added todo.

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

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:203: new
SkPDFScalar(SkScalarDiv(fPaint.getAlpha(), 0xFF));
On 2012/01/05 14:30:08, epoger wrote:
> I don't think this is correct... interpretation of 0xFF will vary depending on
> whether SK_SCALAR_IS_FLOAT.  Right?

But the numerator's interpretation changes in the same way, i.e. 192/255 vs
(192/65536)/(255/65536).  The old code theoretically did the same thing, but I
can't figure out why it wasn't working.  This works.

[reed1, 2012-01-05 18:14:04]

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

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:203: new
SkPDFScalar(SkScalarDiv(fPaint.getAlpha(), 0xFF));
On 2012/01/05 17:47:58, Steve VanDeBogart wrote:
> On 2012/01/05 14:30:08, epoger wrote:
> > I don't think this is correct... interpretation of 0xFF will vary depending
on
> > whether SK_SCALAR_IS_FLOAT.  Right?
> 
> But the numerator's interpretation changes in the same way, i.e. 192/255 vs
> (192/65536)/(255/65536).  The old code theoretically did the same thing, but I
> can't figure out why it wasn't working.  This works.

SkScalarDiv(int, int) will always do the right thing.

[reed1, 2012-01-05 18:17:32]

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

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFDevice.cpp#newcode286
src/pdf/SkPDFDevice.cpp:286: const SkIPoint& translation) {
On 2012/01/05 17:47:58, Steve VanDeBogart wrote:
> On 2012/01/05 14:30:08, epoger wrote:
> > In the long term, I think it would be better to take SkPoint than SkIPoint
as
> a
> > parameter so we don't have to worry about the later step.  If you agree,
maybe
> > just add a TODO...
> 
> Sounds good, fixed.

Not sure I agree. If the translate is being applied to devices and their clips,
then integers is semantically correct (that's all we ever have in SkRegion).
However, I don't feel strongly either way as this function is private to the
impl.

[epoger, 2012-01-05 18:37:35]

LGTM

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

http://codereview.appspot.com/5516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:203: new
SkPDFScalar(SkScalarDiv(fPaint.getAlpha(), 0xFF));
On 2012/01/05 18:14:04, reed1 wrote:
> On 2012/01/05 17:47:58, Steve VanDeBogart wrote:
> > On 2012/01/05 14:30:08, epoger wrote:
> > > I don't think this is correct... interpretation of 0xFF will vary
depending
> on
> > > whether SK_SCALAR_IS_FLOAT.  Right?
> > 
> > But the numerator's interpretation changes in the same way, i.e. 192/255 vs
> > (192/65536)/(255/65536).  The old code theoretically did the same thing, but
I
> > can't figure out why it wasn't working.  This works.
> 
> SkScalarDiv(int, int) will always do the right thing.

Ah, I see.  I assumed that fPaint.getAlpha() returned SkScalar, but it
doesn't... it returns an integer.  And since both the numerator and denominator
are integers, SkScalarDiv works fine.

I think it would be slightly clearer as follows, but I'm happy either way:
  new SkPDFScalar(SkIntToScalar(fPaint.getAlpha()) / 0xFF)