Issue 4657087: Changed Skia backend code to add support to print in the margins (Closed)
|
Created:
13 years, 5 months ago by aayushkumar Modified:
13 years, 5 months ago CC:
skia-review_googlegroups.com, kmadhusu_chromium.org, bungeman, aayushkumar_google.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionModified to add support to print in the margins of pdf documents
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 28
Patch Set 4 : '' #
Total comments: 23
Patch Set 5 : Made stylistic changes based on Chris and Reed's feedback #Patch Set 6 : Removed redundant brackets #
Total comments: 6
Patch Set 7 : Added braces and fixed style nits #
Total comments: 4
Patch Set 8 : Changed param type and added documentation #MessagesTotal messages: 18
Looking good! I have mostly style comments. I'm not seeing why we need to modify SkDevice.h/cpp. Can all the changes go into SkPDFDevice.h/cpp? http://codereview.appspot.com/4657087/diff/5001/include/core/SkDevice.h File include/core/SkDevice.h (right): http://codereview.appspot.com/4657087/diff/5001/include/core/SkDevice.h#newco... include/core/SkDevice.h:235: MARGIN_DA, The enum values should be named kMargin_DrawingArea and kContent_DrawingArea. According to the style guidelines for skia. http://code.google.com/p/skia/wiki/CodingStyleGuidelines http://codereview.appspot.com/4657087/diff/5001/include/core/SkDevice.h#newco... include/core/SkDevice.h:236: CONTENT_DA, Nit: Can we have kContent_DrawingArea come before kMargin_DrawingArea? http://codereview.appspot.com/4657087/diff/5001/include/core/SkDevice.h#newco... include/core/SkDevice.h:239: virtual void setupDrawingArea(DrawingArea area); Nit: Can we name this function setDrawingArea? http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:145: SK_API void copyContentToDataDrawingArea( Seems like this function should be named copyContentEntriesToData. Also it looks like it should be in the private section of this class. Additionally I don't think it needs the SK_API prefix since it doesn't appear to be part of the public interface of this class. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:147: SkDynamicMemoryWStream *data) const; Place the '*' next to the type. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:147: SkDynamicMemoryWStream *data) const; Nit: This can fit on the line above. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:172: Optional: Don't think we need a newline here. There fields are all related. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:175: DrawingArea drawingArea; drawingArea -> fDrawingArea http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:957: void SkPDFDevice::setupDrawingArea(DrawingArea area) { Nit: Can we name the argument drawingArea here an in the header? http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1083: SkDynamicMemoryWStream *data) const { Nit: This can fit on the above line. http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1106: SkPDFDevice::copyContentToDataDrawingArea(fMarginContentEntries.get(), &data); Are you planning on adding additional logic here to clip and alter the initialTransform? If so please add a // TODO(aayushkumar): http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1247: ContentEntry** contentEntry; contentEntry -> lastContentEntry http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1248: SkTScopedPtr<ContentEntry> *contentEntries; Place the '*' next to the type. http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1250: if (drawingArea == CONTENT_DA) { To clean this up I think we should add private accessor functions to this class: getContentEntries() const getLastContentEntry() const We can replace previous usage of fContentEntries and fLastContentEntry with these calls.
Sign in to reply to this message.
What is the background on why we need a new virtual on device? Is it of any interest to raster or gpu?
Sign in to reply to this message.
adding Ben
Sign in to reply to this message.
On 2011/07/08 21:16:18, reed1 wrote: > What is the background on why we need a new virtual on device? Is it of any > interest to raster or gpu? I don't think we need a new virtual on SkDevice. I've requested that the changes be limited to SkPDFDevice.
Sign in to reply to this message.
Removed changes from SkDevice On 2011/07/08 21:22:04, Chris Guillory wrote: > On 2011/07/08 21:16:18, reed1 wrote: > > What is the background on why we need a new virtual on device? Is it of any > > interest to raster or gpu? > I don't think we need a new virtual on SkDevice. I've requested that the changes > be limited to SkPDFDevice.
Sign in to reply to this message.
http://codereview.appspot.com/4657087/diff/5001/include/core/SkDevice.h File include/core/SkDevice.h (right): http://codereview.appspot.com/4657087/diff/5001/include/core/SkDevice.h#newco... include/core/SkDevice.h:235: MARGIN_DA, On 2011/07/08 21:11:54, Chris Guillory wrote: > The enum values should be named kMargin_DrawingArea and kContent_DrawingArea. > According to the style guidelines for skia. > http://code.google.com/p/skia/wiki/CodingStyleGuidelines Done. http://codereview.appspot.com/4657087/diff/5001/include/core/SkDevice.h#newco... include/core/SkDevice.h:236: CONTENT_DA, On 2011/07/08 21:11:54, Chris Guillory wrote: > Nit: Can we have kContent_DrawingArea come before kMargin_DrawingArea? Done. http://codereview.appspot.com/4657087/diff/5001/include/core/SkDevice.h#newco... include/core/SkDevice.h:239: virtual void setupDrawingArea(DrawingArea area); On 2011/07/08 21:11:54, Chris Guillory wrote: > Nit: Can we name this function setDrawingArea? Done. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:145: SK_API void copyContentToDataDrawingArea( On 2011/07/08 21:11:54, Chris Guillory wrote: > Seems like this function should be named copyContentEntriesToData. Also it looks > like it should be in the private section of this class. Additionally I don't > think it needs the SK_API prefix since it doesn't appear to be part of the > public interface of this class. Done. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:147: SkDynamicMemoryWStream *data) const; On 2011/07/08 21:11:54, Chris Guillory wrote: > Nit: This can fit on the line above. Done. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:147: SkDynamicMemoryWStream *data) const; On 2011/07/08 21:11:54, Chris Guillory wrote: > Place the '*' next to the type. Done. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:172: On 2011/07/08 21:11:54, Chris Guillory wrote: > Optional: Don't think we need a newline here. There fields are all related. Done. http://codereview.appspot.com/4657087/diff/5001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:175: DrawingArea drawingArea; On 2011/07/08 21:11:54, Chris Guillory wrote: > drawingArea -> fDrawingArea Done. http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:957: void SkPDFDevice::setupDrawingArea(DrawingArea area) { On 2011/07/08 21:11:54, Chris Guillory wrote: > Nit: Can we name the argument drawingArea here an in the header? Done. http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1083: SkDynamicMemoryWStream *data) const { On 2011/07/08 21:11:54, Chris Guillory wrote: > Nit: This can fit on the above line. Done. http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1106: SkPDFDevice::copyContentToDataDrawingArea(fMarginContentEntries.get(), &data); Added a TODO On 2011/07/08 21:11:54, Chris Guillory wrote: > Are you planning on adding additional logic here to clip and alter the > initialTransform? If so please add a > // TODO(aayushkumar): Done. http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1247: ContentEntry** contentEntry; On 2011/07/08 21:11:54, Chris Guillory wrote: > contentEntry -> lastContentEntry Done. http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1248: SkTScopedPtr<ContentEntry> *contentEntries; On 2011/07/08 21:11:54, Chris Guillory wrote: > Place the '*' next to the type. Done. http://codereview.appspot.com/4657087/diff/5001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1250: if (drawingArea == CONTENT_DA) { On 2011/07/08 21:11:54, Chris Guillory wrote: > To clean this up I think we should add private accessor functions to this class: > getContentEntries() const > getLastContentEntry() const > > We can replace previous usage of fContentEntries and fLastContentEntry with > these calls. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:114: kContent_DrawingArea, Optional: Comments here. e.g. // Drawing area for the page content. // Drawing area for the page margins. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:118: virtual void setDrawingArea(DrawingArea drawingArea); There should be a comment for this function. Something like this should suffice: Sets the drawing area for the device. Subsequent draw calls are directed to the specific drawing area. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:177: // Accessor and setter functions based on the current drawingArea drawingArea -> DrawingArea. Also add a period at the end of the sentence. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:179: SkTScopedPtr<ContentEntry>* getContentEntries(); Nit: Can you put getContentEntries() before getLastContentEntry(). http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:179: SkTScopedPtr<ContentEntry>* getContentEntries(); I think I'd be better if this returned a reference. SkTScopedPtr<ContentEntry>& http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:238: SkDynamicMemoryWStream* data) const; Nit: Indent to ContentEntry* above. http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1272: ContentEntry* lastContentEntry = getLastContentEntry(); This local isn't needed here. Please move this line to right before this line: if (!lastContentEntry) http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1274: Nit: Remove this newline. http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1314: SkTScopedPtr<ContentEntry>* contentEntries = getContentEntries(); Can you move this line above the SkASSERT and keep both SkASSERTs together? http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1370: if (!contentEntries->get() || The if condition should fit on one line when contentEntries is a reference.
Sign in to reply to this message.
http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:117: +1 needs comment/documentation (as does the enum values) why is it virtual?
Sign in to reply to this message.
http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:114: kContent_DrawingArea, On 2011/07/09 01:23:39, Chris Guillory wrote: > Optional: Comments here. e.g. > // Drawing area for the page content. > // Drawing area for the page margins. Done. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:117: Removed virtual. Sorry, this was an artifact of of when I had the setDrawingArea in the SkDevice file. On 2011/07/11 12:20:32, reed1 wrote: > +1 needs comment/documentation (as does the enum values) > why is it virtual? http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:117: On 2011/07/11 12:20:32, reed1 wrote: > +1 needs comment/documentation (as does the enum values) > why is it virtual? Done. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:118: virtual void setDrawingArea(DrawingArea drawingArea); On 2011/07/09 01:23:39, Chris Guillory wrote: > There should be a comment for this function. Something like this should suffice: > Sets the drawing area for the device. Subsequent draw calls are directed to the > specific drawing area. Done. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:177: // Accessor and setter functions based on the current drawingArea On 2011/07/09 01:23:39, Chris Guillory wrote: > drawingArea -> DrawingArea. Also add a period at the end of the sentence. Done. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:179: SkTScopedPtr<ContentEntry>* getContentEntries(); On 2011/07/09 01:23:39, Chris Guillory wrote: > Nit: Can you put getContentEntries() before getLastContentEntry(). Done. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:179: SkTScopedPtr<ContentEntry>* getContentEntries(); On 2011/07/09 01:23:39, Chris Guillory wrote: > I think I'd be better if this returned a reference. SkTScopedPtr<ContentEntry>& Done. http://codereview.appspot.com/4657087/diff/4002/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:238: SkDynamicMemoryWStream* data) const; On 2011/07/09 01:23:39, Chris Guillory wrote: > Nit: Indent to ContentEntry* above. Done. http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1272: ContentEntry* lastContentEntry = getLastContentEntry(); On 2011/07/09 01:23:39, Chris Guillory wrote: > This local isn't needed here. Please move this line to right before this line: > if (!lastContentEntry) Done. http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1274: On 2011/07/09 01:23:39, Chris Guillory wrote: > Nit: Remove this newline. Done. http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1314: SkTScopedPtr<ContentEntry>* contentEntries = getContentEntries(); On 2011/07/09 01:23:39, Chris Guillory wrote: > Can you move this line above the SkASSERT and keep both SkASSERTs together? Done. http://codereview.appspot.com/4657087/diff/4002/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1370: if (!contentEntries->get() || On 2011/07/09 01:23:39, Chris Guillory wrote: > The if condition should fit on one line when contentEntries is a reference. Done.
Sign in to reply to this message.
LGTM, just two nits. http://codereview.appspot.com/4657087/diff/12002/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/12002/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:114: kContent_DrawingArea, // Drawing area for the page content Nit: Add a period at the end of these comments. http://codereview.appspot.com/4657087/diff/12002/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:240: /** Helper method to copyContentToData. Nit: to -> for
Sign in to reply to this message.
http://codereview.appspot.com/4657087/diff/12002/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4657087/diff/12002/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:958: if (fDrawingArea == kContent_DrawingArea) Per skia code guidelines braces are always used. This is different from Chrome. http://code.google.com/p/skia/wiki/CodingStyleGuidelines
Sign in to reply to this message.
http://codereview.appspot.com/4657087/diff/12002/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/12002/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:114: kContent_DrawingArea, // Drawing area for the page content On 2011/07/11 23:16:11, Chris Guillory wrote: > Nit: Add a period at the end of these comments. Done. http://codereview.appspot.com/4657087/diff/12002/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:240: /** Helper method to copyContentToData. On 2011/07/11 23:16:11, Chris Guillory wrote: > Nit: to -> for Done. http://codereview.appspot.com/4657087/diff/12002/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4657087/diff/12002/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:958: if (fDrawingArea == kContent_DrawingArea) On 2011/07/11 23:18:17, Chris Guillory wrote: > Per skia code guidelines braces are always used. This is different from Chrome. > http://code.google.com/p/skia/wiki/CodingStyleGuidelines Done.
Sign in to reply to this message.
I can commit this if needed. Let's let Mike have a chance for another look.
Sign in to reply to this message.
Yeah, that'll be great. Thanks! :) On Mon, Jul 11, 2011 at 4:54 PM, <ctguil@chromium.org> wrote: > I can commit this if needed. Let's let Mike have a chance for another > look. > > > http://codereview.appspot.com/**4657087/<http://codereview.appspot.com/4657087/> >
Sign in to reply to this message.
Just a documentation question and a parameter type. LGTM after those are addressed. http://codereview.appspot.com/4657087/diff/18001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/18001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:119: * directed to the specific drawing area (margin or content area). dox: What is the default? Where is drawing directed if I never make this call? http://codereview.appspot.com/4657087/diff/18001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:244: void copyContentEntriesToData(ContentEntry* entry, I think we can just pass SkWStream* here. No need to name the subclass I think.
Sign in to reply to this message.
http://codereview.appspot.com/4657087/diff/18001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4657087/diff/18001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:119: * directed to the specific drawing area (margin or content area). On 2011/07/12 12:31:27, reed1 wrote: > dox: What is the default? Where is drawing directed if I never make this call? Done. http://codereview.appspot.com/4657087/diff/18001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:244: void copyContentEntriesToData(ContentEntry* entry, On 2011/07/12 12:31:27, reed1 wrote: > I think we can just pass SkWStream* here. No need to name the subclass I think. Done.
Sign in to reply to this message.
I reformatted some of the comments and added a TODO in SkPDFDevice::setDrawingArea. Committed as revision 1843. You can close this issue now.
Sign in to reply to this message.
|
