Add DPI stettings to SkDocument::CreatePDF(). Tests will be added in a future cl, once DPI will be …

Add DPI stettings to SkDocument::CreatePDF(). Tests will be added in a future cl, once DPI will be used in SkPDFDevice

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

[reed1, 2013-10-21 13:10:24]

https://codereview.chromium.org/32233003/diff/30001/include/core/SkDocument.h
File include/core/SkDocument.h (right):

https://codereview.chromium.org/32233003/diff/30001/include/core/SkDocument.h...
include/core/SkDocument.h:41: *    as JPEG (DCT).
What does DPI mean to the client when they draw into this Document? Does it
affect the coordinates they should pass in? Does it affect internal
optimizations? Does it indicated something about the presumed target when the
Document gets "played" back?

[edisonn, 2013-10-21 14:40:58]

https://codereview.chromium.org/32233003/diff/30001/include/core/SkDocument.h
File include/core/SkDocument.h (right):

https://codereview.chromium.org/32233003/diff/30001/include/core/SkDocument.h...
include/core/SkDocument.h:41: *    as JPEG (DCT).
On 2013/10/21 13:10:24, reed1 wrote:
> What does DPI mean to the client when they draw into this Document? Does it
> affect the coordinates they should pass in?
No
 Does it affect internal
> optimizations? 
it could, the images might be smaller or bigger, we might change the way we
encode the image (no ecoding, zip, jpeg), but I am not sure I should put this in
the comment, as the encoder controlls this
Does it indicated something about the presumed target when the
> Document gets "played" back?

Not much, but if a ridiculous high DPI is used, the play back will be slow or
will fail because memory can't be allocated

added comment

[reed1, 2013-10-21 15:13:07]

lgtm -- but wait for other reviewers

[vandebo (ex-Chrome), 2013-10-21 16:28:35]

LGTM

https://codereview.chromium.org/32233003/diff/90001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/32233003/diff/90001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:211: fRasterDpi = rasterDpi;
nit: you could move the 0 check down to this method and remove
SK_ScalarDefaultRasterDPI from the public interface.

[edisonn, 2013-10-21 16:36:33]

https://codereview.chromium.org/32233003/diff/90001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/32233003/diff/90001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:211: fRasterDpi = rasterDpi;
On 2013/10/21 16:28:35, vandebo wrote:
> nit: you could move the 0 check down to this method and remove
> SK_ScalarDefaultRasterDPI from the public interface.

IMHO it is way easier for a dev to find out the default value by inspecting a
default parameter, rather than having to dig in the code.

[vandebo (ex-Chrome), 2013-10-21 16:43:26]

On 2013/10/21 16:36:33, edisonn wrote:
> https://codereview.chromium.org/32233003/diff/90001/include/pdf/SkPDFDevice.h
> File include/pdf/SkPDFDevice.h (right):
> 
>
https://codereview.chromium.org/32233003/diff/90001/include/pdf/SkPDFDevice.h...
> include/pdf/SkPDFDevice.h:211: fRasterDpi = rasterDpi;
> On 2013/10/21 16:28:35, vandebo wrote:
> > nit: you could move the 0 check down to this method and remove
> > SK_ScalarDefaultRasterDPI from the public interface.
> 
> IMHO it is way easier for a dev to find out the default value by inspecting a
> default parameter, rather than having to dig in the code.

You could still document in the header, but it needn't be exposed - code wise. 
But which ever LGTM

[commit-bot: I haz the power, 2013-10-21 16:50:37]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/edisonn@google.com/32233003/90001

[commit-bot: I haz the power, 2013-10-21 17:14:40]

Change committed as 11886