Issue 5076041: Add xps device to skia. (Closed)
|
Created:
13 years, 3 months ago by bungeman Modified:
13 years, 2 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
Patch Set 1 #
Total comments: 14
Patch Set 2 : Only build on Windows. #Patch Set 3 : Disable gm xps output if not supported. #
Total comments: 2
Patch Set 4 : Clean up gyp dependencies. #
Total comments: 57
Patch Set 5 : Address comments. #Patch Set 6 : Fix up includes. #
Total comments: 8
Patch Set 7 : Cleanup and comments. #Patch Set 8 : Don't change gyp_skia, even if I am on 2008. #Patch Set 9 : Missed a matrix in refactoring. #Patch Set 10 : Line length issues. #
Total comments: 73
Patch Set 11 : Functional changes and clean up, will address style issues soon. #Patch Set 12 : How did that sneak in there? Removed random change. #Patch Set 13 : Most style issues cleaned up. Comment at will. #Patch Set 14 : A few style issues that got through. #
Total comments: 67
Patch Set 15 : Comments taken. #
Total comments: 6
Patch Set 16 : Remove all the ref counting. #
Total comments: 4
MessagesTotal messages: 18
This first patch is a mostly working xps device. Known issues: 1. Need to make SkBitSet acceptable for moving out of pdf. 2. Bounder and rasterizer not yet implemented. 3. Inverse fills on rects. 4. Two point and sweep gradients, bitmap gradients not yet implemented. 5. Transfer modes. 6. Need a different font sub-setter. Since xps only does ttf and otf, need to be able to create tt of cff (for cubics) glyph only fonts.
Sign in to reply to this message.
A couple quick comments, I'll look more later. http://codereview.appspot.com/5076041/diff/1/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/1/gyp/xps.gyp#newcode2 gyp/xps.gyp:2: 'includes': [ Should there be something in this file to make it only compile on Windows? http://codereview.appspot.com/5076041/diff/1/include/device/xps/SkXPSDevice.h File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/1/include/device/xps/SkXPSDevice.h... include/device/xps/SkXPSDevice.h:42: virtual bool BeginPortfolio(SkWStream* outputStream); It seems that you can add multiple pages to the single XPS device. You may want to have an SkXPDDocument/Portfolio class that has new device, device done methods so that the device has the size specified by the user. This would also be closer to the pattern used by the PDF backend - not to say that's the best way to do it, but consistency between the backends can make things easier to use. http://codereview.appspot.com/5076041/diff/1/include/device/xps/SkXPSDevice.h... include/device/xps/SkXPSDevice.h:64: , const SkSize& pixelsPerMeter I'm not sure the punctuation before entry applies to another bug initializer lists, at least I can't find any reference in the style guide to indicate so. http://codereview.appspot.com/5076041/diff/1/include/device/xps/SkXPSDevice.h... include/device/xps/SkXPSDevice.h:137: nit: extra line http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp#n... src/device/xps/SkXPSDevice.cpp:179: : SkDevice(make_content_bitmap(10000,10000)) Webkit asks the device for its width and height, so putting constants here will break things. nit: comma space http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp#n... src/device/xps/SkXPSDevice.cpp:180: , fAutoCo() no need to explicitly initialize things with default constructors. i.e. you only need fCurrentPage. http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp#n... src/device/xps/SkXPSDevice.cpp:186: , fTypefaces() open brace goes on this line.
Sign in to reply to this message.
I looked at just the gyp files, as of patch set 3... http://codereview.appspot.com/5076041/diff/16001/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/16001/gyp/xps.gyp#newcode18 gyp/xps.gyp:18: '../include/pdf', # needed to get SkBitSet.h I don't think you need to specify ../include/pdf here... pdf.gyp should provide this to you via direct_dependent_settings.
Sign in to reply to this message.
general comment about calling functions
we_call_functions(this, way, with, their,
parameters, here);
http://codereview.appspot.com/5076041/diff/17009/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (left):
http://codereview.appspot.com/5076041/diff/17009/include/pdf/SkBitSet.h#oldco...
include/pdf/SkBitSet.h:44: /** Export set bits to unsigned int array. (used in
font subsetting)
update comment now that the array is templated
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.cpp
File src/device/xps/SkXPSDevice.cpp (right):
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:39: //Microsoft does not supply a stdint.h
sk_stdint.h declares uint64_t
can that type be used here?
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:64: #define SkScalarToFLOAT(n) SkScalarToFloat(n)
Add dox why we need this FLOAT macro
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:74: static void SkTraceHR(const char* file,
unsigned long line
In general, don't need to prefix a local static function with Sk, since that
sort of gives the impression that you want the name to be globally distinct.
See what you did below for create_id()
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:118:
Please dox all of these acronym macros
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:172: static SkBitmap make_content_bitmap(uint32_t
width, uint32_t height) {
nit: width and height can just be ints, since that is what is declared for
setConfig()
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:172: static SkBitmap make_content_bitmap(uint32_t
width, uint32_t height) {
rename to create_noconfig_bitmap? Not sure, at this point in the code, why its
called "content". Just a thought.
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:178: SkXPSDevice::SkXPSDevice()
We don't mention empty constructors anywhere else in skia (e.g.
fOutputStream()). I think you can leave those off.
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:191: const TypefaceUse* last =
this->fTypefaces.end();
I like making the terminal const, though its really one past the last one, not
the last. I think we use the name 'stop' in other parts of the code that iterate
an array.
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:230: //then have a base canvas do the scale to
physical units.
Don't need this-> qualifier for fields (the 'f' does that). Just use it for
methods.
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:430: static HRESULT subset_typeface(TypefaceUse*
current) {
use uint8_t
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:431: unsigned char *puchFontPackageBuffer;
use uint32_t
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:434:
use uint16_t
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:503: static XPS_COLOR createXpsColor(const
SkColor& skColor) {
SkColor is a POD, so just pass it by value.
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:555: SkScalar totalAB = (endTransformed.fX -
startTransformed.fX)
dox this funny metric of dx + dy
http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c...
src/device/xps/SkXPSDevice.cpp:598: HRESULT
SkXPSDevice::createXpsSolidColorBrush(const SkColor& skColor
use SkAlpha if you like
Sign in to reply to this message.
Sorry if you already know/do this, but in general, acknowledging each comment as done or explaining why you didn't do it will help your reviewers. Also, when you upload a new patch set (and want further comment), another message (usually the one acknowledging/discussing comments) lets them know you're ready for another look. http://codereview.appspot.com/5076041/diff/17009/gm/gmmain.cpp File gm/gmmain.cpp (right): http://codereview.appspot.com/5076041/diff/17009/gm/gmmain.cpp#newcode201 gm/gmmain.cpp:201: static bool write_document(const SkString& path, const SkDynamicMemoryWStream& document) { nit: line length http://codereview.appspot.com/5076041/diff/17009/gm/gmmain.cpp#newcode537 gm/gmmain.cpp:537: #ifdef SK_SUPPORT_PDF SK_SUPPORT_XPS http://codereview.appspot.com/5076041/diff/17009/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/17009/gyp/xps.gyp#newcode33 gyp/xps.gyp:33: },{ #else if 'skia_os != "win"' Why not have the sources inside the conditions, so you don't have to list all the files twice (it's only two right now, but...). http://codereview.appspot.com/5076041/diff/17009/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/17009/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:149: virtual void drawRect(const SkDraw&, const SkRect& r, const SkPaint& paint, bool transformRect); A private method with the same same as a public one is confusing, consider a different name for this one. http://codereview.appspot.com/5076041/diff/17009/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:223: , const SkDraw& d); I generally haven't seen function overloading used in Skia, consider two different names. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:119: #define HR(ex) HR_GENERAL(ex, NULL, _hr) A slightly more verbose name could make these easier to understand. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:141: { Assert that bufferSize is large enough? http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:274: HRM(thumbnailGenerator->GenerateThumbnail( Does this write out the thumbnail to the disk? That won't work in the renderer process in Chrome, it doesn't have permission to create files. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1181: XPS_MATRIX rawXpsMatrix = { Consider pulling this chunk of code into a function, it's used several times. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1388: void SkXPSDevice::drawInversePath(const SkDraw& d This seems like a pretty long method. Maybe pulling some of the bits into helper functions would make it easier to read. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:2049: nit: Many extra lines. Use a single line seperator if you want one. // ************* http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:2076: /* You should probably remove dead code. If you anticipate using it in the near future, add a TODO comment about what needs to be done. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:2375: static void xps_draw_1_glyph(const SkDraw1Glyph& state I usually see all the static helper function at the top of the file, before any method declarations. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:2593: SkXPSDevice::SkXPSDevice(IXpsOMObjectFactory* xpsFactory) Keep the constructors together in the file.
Sign in to reply to this message.
Easier to clean up comments addressed. Significant clean up still needed in the implementation details. http://codereview.appspot.com/5076041/diff/1/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/1/gyp/xps.gyp#newcode2 gyp/xps.gyp:2: 'includes': [ On 2011/09/19 23:41:45, Steve VanDeBogart wrote: > Should there be something in this file to make it only compile on Windows? Yes, silly oversight on my part. http://codereview.appspot.com/5076041/diff/1/include/device/xps/SkXPSDevice.h File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/1/include/device/xps/SkXPSDevice.h... include/device/xps/SkXPSDevice.h:42: virtual bool BeginPortfolio(SkWStream* outputStream); On 2011/09/19 23:41:45, Steve VanDeBogart wrote: > It seems that you can add multiple pages to the single XPS device. You may want > to have an SkXPDDocument/Portfolio class that has new device, device done > methods so that the device has the size specified by the user. This would also > be closer to the pattern used by the PDF backend - not to say that's the best > way to do it, but consistency between the backends can make things easier to > use. We're currently looking into exposing pagination through a Canvas. Having multiple paginated devices is part of this. This may change as this develops. http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp#n... src/device/xps/SkXPSDevice.cpp:179: : SkDevice(make_content_bitmap(10000,10000)) On 2011/09/19 23:41:45, Steve VanDeBogart wrote: > Webkit asks the device for its width and height, so putting constants here will > break things. > nit: comma space I'll have to take a look at how that works. The Chrome print device had height and width specified for each page (not that anyone took advantage of it). We're going to have to be careful about this anyway, as the size of the device may be unrelated to what is wanted for layout. http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp#n... src/device/xps/SkXPSDevice.cpp:180: , fAutoCo() On 2011/09/19 23:41:45, Steve VanDeBogart wrote: > no need to explicitly initialize things with default constructors. i.e. you only > need fCurrentPage. Done http://codereview.appspot.com/5076041/diff/16001/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/16001/gyp/xps.gyp#newcode18 gyp/xps.gyp:18: '../include/pdf', # needed to get SkBitSet.h On 2011/09/20 19:57:33, epoger wrote: > I don't think you need to specify ../include/pdf here... pdf.gyp should provide > this to you via direct_dependent_settings. Cleaned this up, as well as adding images to dependencies and removing from include_dirs. http://codereview.appspot.com/5076041/diff/17009/gm/gmmain.cpp File gm/gmmain.cpp (right): http://codereview.appspot.com/5076041/diff/17009/gm/gmmain.cpp#newcode201 gm/gmmain.cpp:201: static bool write_document(const SkString& path, const SkDynamicMemoryWStream& document) { On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > nit: line length Done. http://codereview.appspot.com/5076041/diff/17009/gm/gmmain.cpp#newcode537 gm/gmmain.cpp:537: #ifdef SK_SUPPORT_PDF On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > SK_SUPPORT_XPS Oops. Done. http://codereview.appspot.com/5076041/diff/17009/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/17009/gyp/xps.gyp#newcode33 gyp/xps.gyp:33: },{ #else if 'skia_os != "win"' On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > Why not have the sources inside the conditions, so you don't have to list all > the files twice (it's only two right now, but...). Because of the way gyp works. All of Skia's gyp files are structured this way (or should be). When gyp creates projects for VisualStudio and XCode, all unconditionally listed files will show up in the project and those which are then excluded will be marked as not built. If a file only appears conditionally, it will not appear in the project at all if the condition is not met. This has resulted in issues where a change is made on one platform and it is not obvious that code for other platforms needs to be updated. We don't really like this state of affairs, but that's why. This file is structured that way for consistency with the other gyp files in Skia. http://codereview.appspot.com/5076041/diff/17009/include/pdf/SkBitSet.h File include/pdf/SkBitSet.h (left): http://codereview.appspot.com/5076041/diff/17009/include/pdf/SkBitSet.h#oldco... include/pdf/SkBitSet.h:44: /** Export set bits to unsigned int array. (used in font subsetting) On 2011/09/21 12:55:53, reed1 wrote: > update comment now that the array is templated I had done this, and accidentally reverted it. Now fixed. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:39: //Microsoft does not supply a stdint.h On 2011/09/21 12:55:53, reed1 wrote: > sk_stdint.h declares uint64_t > > can that type be used here? Added typedef for uintmax_t to sk_stdint. I didn't realize it was pulled in through SkTypes.h. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:64: #define SkScalarToFLOAT(n) SkScalarToFloat(n) On 2011/09/21 12:55:53, reed1 wrote: > Add dox why we need this FLOAT macro Added comment. It isn't strictly required, but makes it obvious that we are converting a scalar to a Windows type (and not just any old float). http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:74: static void SkTraceHR(const char* file, unsigned long line On 2011/09/21 12:55:53, reed1 wrote: > In general, don't need to prefix a local static function with Sk, since that > sort of gives the impression that you want the name to be globally distinct. > > See what you did below for create_id() Left this way because it does need to be pulled out. Created SkHRESULT.cpp for this function. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:118: On 2011/09/21 12:55:53, reed1 wrote: > Please dox all of these acronym macros Pulled out to SkHRESULT.h and doxed. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:119: #define HR(ex) HR_GENERAL(ex, NULL, _hr) On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > A slightly more verbose name could make these easier to understand. This is fairly common. See http://www.google.com/codesearch#search/&q=%22define%20hr(%22&type=cs . This is also used already over 50 times, and will be used a fair bit more. It's specifically for error handling, so one tries to keep it out of the way. This has been moved out to SkHRESULT.h. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:141: { On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > Assert that bufferSize is large enough? Why not? In theory someone could want a partial GUID string, but I'm not sure why. Done. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:172: static SkBitmap make_content_bitmap(uint32_t width, uint32_t height) { On 2011/09/21 12:55:53, reed1 wrote: > nit: width and height can just be ints, since that is what is declared for > setConfig() Done http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:172: static SkBitmap make_content_bitmap(uint32_t width, uint32_t height) { On 2011/09/21 12:55:53, reed1 wrote: > rename to create_noconfig_bitmap? Not sure, at this point in the code, why its > called "content". Just a thought. Renamed to create_fake_bitmap as it is just making device happy, and never actually allocates anything for the pixels. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:178: SkXPSDevice::SkXPSDevice() On 2011/09/21 12:55:53, reed1 wrote: > We don't mention empty constructors anywhere else in skia (e.g. > fOutputStream()). I think you can leave those off. Done http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:191: const TypefaceUse* last = this->fTypefaces.end(); On 2011/09/21 12:55:53, reed1 wrote: > I like making the terminal const, though its really one past the last one, not > the last. I think we use the name 'stop' in other parts of the code that iterate > an array. Changed the three uses to 'stop'. Some places elsewhere also use 'end'. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:274: HRM(thumbnailGenerator->GenerateThumbnail( On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > Does this write out the thumbnail to the disk? That won't work in the renderer > process in Chrome, it doesn't have permission to create files. No, it just writes the thumbnail to 'image'. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:430: static HRESULT subset_typeface(TypefaceUse* current) { On 2011/09/21 12:55:53, reed1 wrote: > use uint8_t Added comment that these types are what the declaration of CreateFontPackage wants. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:431: unsigned char *puchFontPackageBuffer; On 2011/09/21 12:55:53, reed1 wrote: > use uint32_t Added comment that these types are what the declaration of CreateFontPackage wants. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:434: On 2011/09/21 12:55:53, reed1 wrote: > use uint16_t Added comment that these types are what the declaration of CreateFontPackage wants. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:503: static XPS_COLOR createXpsColor(const SkColor& skColor) { On 2011/09/21 12:55:53, reed1 wrote: > SkColor is a POD, so just pass it by value. Done. Also changed in other places where used. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:555: SkScalar totalAB = (endTransformed.fX - startTransformed.fX) On 2011/09/21 12:55:53, reed1 wrote: > dox this funny metric of dx + dy Commented and changed names to make this more obvious. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:598: HRESULT SkXPSDevice::createXpsSolidColorBrush(const SkColor& skColor On 2011/09/21 12:55:53, reed1 wrote: > use SkAlpha if you like Done. Was unaware of this typedef. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1181: XPS_MATRIX rawXpsMatrix = { On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > Consider pulling this chunk of code into a function, it's used several times. Yes, everything down here need significant clean up. This is a particularly glaring case. Will be done. http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1388: void SkXPSDevice::drawInversePath(const SkDraw& d On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > This seems like a pretty long method. Maybe pulling some of the bits into > helper functions would make it easier to read. A good bit of this is duplicating what drawPath does / already did. This method is still at proof of concept stage and needs heavy clean up. (I was rather excited to get it working at all, and the standard XPS viewer won't show the result correctly, but it does print well.) http://codereview.appspot.com/5076041/diff/17009/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:2375: static void xps_draw_1_glyph(const SkDraw1Glyph& state On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > I usually see all the static helper function at the top of the file, before any > method declarations. Yes, but this is highly tied to the text drawing below. This is actually modeled on (including method placement) how the gpu device does this. Most likely this (text handling) will be pulled out into another file.
Sign in to reply to this message.
http://codereview.appspot.com/5076041/diff/17011/include/core/SkMath.h File include/core/SkMath.h (left): http://codereview.appspot.com/5076041/diff/17011/include/core/SkMath.h#oldcode13 include/core/SkMath.h:13: #include "SkTypes.h" Lets put these in a XPS header for now (e.g. src/xps/SkXPSMath.h) http://codereview.appspot.com/5076041/diff/17011/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/17011/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:41: For each of these new APIs... - document what they do - why are they cased differently from other skia methods? - why are they virtual? http://codereview.appspot.com/5076041/diff/17011/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:78: /* If this (and other) methods are commented out because you haven't implemented them yet, perhaps that is better document in the .cpp. e.g. void SkXPSDevice::clear(SkColor color) { // TODO: override this for XPS this->INHERITED::clear(color); } http://codereview.appspot.com/5076041/diff/17011/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:123: private: Do private functions every need SK_API?
Sign in to reply to this message.
Still need to clean up rect and path methods. The rest of the code is now a bit easier on the eyes. http://codereview.appspot.com/5076041/diff/17011/include/core/SkMath.h File include/core/SkMath.h (left): http://codereview.appspot.com/5076041/diff/17011/include/core/SkMath.h#oldcode13 include/core/SkMath.h:13: #include "SkTypes.h" On 2011/09/22 16:23:29, reed1 wrote: > Lets put these in a XPS header for now (e.g. src/xps/SkXPSMath.h) Moved to SkConstexprMath.h in xps include directory. http://codereview.appspot.com/5076041/diff/17011/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/17011/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:41: On 2011/09/22 16:23:29, reed1 wrote: > For each of these new APIs... > - document what they do Will do. > - why are they cased differently from other skia methods? Fixed. > - why are they virtual? The idea is that they will be overriding an interface. http://codereview.appspot.com/5076041/diff/17011/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:78: /* On 2011/09/22 16:23:29, reed1 wrote: > If this (and other) methods are commented out because you haven't implemented > them yet, perhaps that is better document in the .cpp. > > e.g. > void SkXPSDevice::clear(SkColor color) { > // TODO: override this for XPS > this->INHERITED::clear(color); > } Done. http://codereview.appspot.com/5076041/diff/17011/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:123: private: On 2011/09/22 16:23:29, reed1 wrote: > Do private functions every need SK_API? Oops, no. Copy-pasta from other constructor. Fixed.
Sign in to reply to this message.
Are you still working on addressing comment, or is it ready for review? I started reviewing it again, but stopped because I wasn't sure. http://codereview.appspot.com/5076041/diff/17009/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/17009/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:149: virtual void drawRect(const SkDraw&, const SkRect& r, const SkPaint& paint, bool transformRect); On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > A private method with the same same as a public one is confusing, consider a > different name for this one. You haven't addressed this. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp File gm/gmmain.cpp (right): http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode291 gm/gmmain.cpp:291: , SkIntToScalar(size.height())); , on prev line http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode292 gm/gmmain.cpp:292: static const float inchesPerMeter = 10000.0 / 254.0; Maybe these constants should be part of the XPS device, since they will often be needed when XPS is used. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode293 gm/gmmain.cpp:293: static const float upm = 72*inchesPerMeter; spaces around *'s http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode295 gm/gmmain.cpp:295: , SkFloatToScalar(upm)); , on prev line http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode296 gm/gmmain.cpp:296: static const float ppm = 200*inchesPerMeter; spaces around *'s http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode298 gm/gmmain.cpp:298: , SkFloatToScalar(ppm)); , on prev line http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode323 gm/gmmain.cpp:323: (gRec.fBackend == kPDF_Backend && CAN_IMAGE_PDF)) Open brace on this line. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode398 gm/gmmain.cpp:398: gRec.fBackend == kRaster_Backend || indept up to "readPath" in prev line. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode400 gm/gmmain.cpp:400: (gRec.fBackend == kPDF_Backend && CAN_IMAGE_PDF))) Open brace on this line. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode475 gm/gmmain.cpp:475: "", *bitmap, &document, NULL); indet into gm on prev line http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp#newcode13 gyp/xps.gyp:13: 'pdf.gyp:pdf', # needed to get SkBitSet What's needed to move SkBitSet to core? http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp#newcode16 gyp/xps.gyp:16: '../include/device/xps', I just now noticed the */device/ structure. Presumably all of the backends will move into subdirectories device? http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp#newcode39 gyp/xps.gyp:39: '../include/device/xps/SkXPSDevice.h', These are already out of sync... Can you use wildcards to exclude everything in */device/xps/* http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkConstex... File include/device/xps/SkConstexprMath.h (right): http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkConstex... include/device/xps/SkConstexprMath.h:10: Does this need include guards? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:11: #include "SkTypes.h" Alphabetical with the rest below? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:42: virtual bool beginPortfolio(SkWStream* outputStream); More generic names might be better if you want this interface to become part of SkDevice. begin/endDocument, begin/endPage ? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:64: , const SkSize& pixelsPerMeter , on prev line x6 http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:78: extra line http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:96: bool pathIsMutable); add blank line? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:113: const SkPaint& paint); add blank line? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:138: extra line http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:139: HRESULT SkXPSDevice::initXpsDocumentWriter(IXpsOMImageResource* image); SkXPSDevice:: isn't needed [repeated to the end of the file] http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:143: , IXpsOMPage** page); , on prev line [repeated to the end of the file] http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:166: , const SkShader::TileMode (&xy)[2] & usually touches the type, not the name http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:174: , IXpsOMMatrixTransform* xpsMatrixToUse const ref? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:199: const SkPoint (&points)[4] & usually touches the type, not the name http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:208: IXpsOMObjectFactory* xpsFactory const ref? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:209: , IXpsOMCanvas* page conf ref? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:210: , IXpsOMFontResource* font const ref? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:212: , XPS_GLYPH_INDEX* xpsGlyphs const ref? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:214: , XPS_POINT *origin const ref? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:218: , const SkPaint* paintPtr const ref instead of const ptr? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:219: , const SkDraw& d); draw is usually the first arg
Sign in to reply to this message.
Most of Steve's comments addressed, or will be in 14 (changes already made locally). http://codereview.appspot.com/5076041/diff/17009/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/17009/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:149: virtual void drawRect(const SkDraw&, const SkRect& r, const SkPaint& paint, bool transformRect); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > > A private method with the same same as a public one is confusing, consider a > > different name for this one. > > You haven't addressed this. Now non-virtual internalDrawRect. http://codereview.appspot.com/5076041/diff/17009/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:223: , const SkDraw& d); On 2011/09/21 17:40:15, Steve VanDeBogart wrote: > I generally haven't seen function overloading used in Skia, consider two > different names. Now clip and clipToPath. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp File gm/gmmain.cpp (right): http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode291 gm/gmmain.cpp:291: , SkIntToScalar(size.height())); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > , on prev line Done. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode292 gm/gmmain.cpp:292: static const float inchesPerMeter = 10000.0 / 254.0; On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > Maybe these constants should be part of the XPS device, since they will often be > needed when XPS is used. I don't think these should be part of the device, however, having a header of shared physical constants and conversions somewhere would be nice. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode293 gm/gmmain.cpp:293: static const float upm = 72*inchesPerMeter; On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > spaces around *'s Done. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode295 gm/gmmain.cpp:295: , SkFloatToScalar(upm)); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > , on prev line Done. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode296 gm/gmmain.cpp:296: static const float ppm = 200*inchesPerMeter; On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > spaces around *'s Done. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode298 gm/gmmain.cpp:298: , SkFloatToScalar(ppm)); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > , on prev line Done. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode323 gm/gmmain.cpp:323: (gRec.fBackend == kPDF_Backend && CAN_IMAGE_PDF)) On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > Open brace on this line. Will be done. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode398 gm/gmmain.cpp:398: gRec.fBackend == kRaster_Backend || On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > indept up to "readPath" in prev line. Done. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode400 gm/gmmain.cpp:400: (gRec.fBackend == kPDF_Backend && CAN_IMAGE_PDF))) On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > Open brace on this line. Done. http://codereview.appspot.com/5076041/diff/24014/gm/gmmain.cpp#newcode475 gm/gmmain.cpp:475: "", *bitmap, &document, NULL); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > indet into gm on prev line Was this way previously, but a little clean-up can't hurt. Done. http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp#newcode16 gyp/xps.gyp:16: '../include/device/xps', On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > I just now noticed the */device/ structure. Presumably all of the backends will > move into subdirectories device? It would be nice, to prevent device directories from cluttering the top level. I'm not expecting this to happen too quickly, but it seems like a nudge in the right direction. http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp#newcode39 gyp/xps.gyp:39: '../include/device/xps/SkXPSDevice.h', On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > These are already out of sync... Can you use wildcards to exclude everything in > */device/xps/* These are, in fact, still in sync. There is no need to exclude SkConstexprMath.h simply because we're not on Windows. This is actually a flag that it should probably go somewhere else, though this may need to happen later. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkConstex... File include/device/xps/SkConstexprMath.h (right): http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkConstex... include/device/xps/SkConstexprMath.h:10: On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > Does this need include guards? Done. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:11: #include "SkTypes.h" On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > Alphabetical with the rest below? SkTypes.h is used in Skia to include stdint.h and windows.h. After the number of issues I went through to discover this, I think SkTypes.h should just be named SkIncludeMeFirst.h. If this isn't first, things get frustrating the same way as not including windows.h first. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:42: virtual bool beginPortfolio(SkWStream* outputStream); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > More generic names might be better if you want this interface to become part of > SkDevice. begin/endDocument, begin/endPage ? Do you mean more specific? A portfolio of canvases can mean a video, a slide show, a book, or an unrelated collection of artwork. A document of pages implies an ordered collection of identically sized surfaces with a single content broken more or less arbitrarily at fixed intervals. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:64: , const SkSize& pixelsPerMeter On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > , on prev line x6 Done. Lots of done. When I'm working on stuff myself I prefer separators at the beginning of a line, terminators at the end. Regex-ed away. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:78: On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > extra line Done. I believe these have all been cleaned up. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:96: bool pathIsMutable); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > add blank line? Done. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:113: const SkPaint& paint); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > add blank line? Done. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:138: On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > extra line Done. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:139: HRESULT SkXPSDevice::initXpsDocumentWriter(IXpsOMImageResource* image); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > SkXPSDevice:: isn't needed [repeated to the end of the file] Done. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:143: , IXpsOMPage** page); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > , on prev line [repeated to the end of the file] Done. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:166: , const SkShader::TileMode (&xy)[2] On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > & usually touches the type, not the name This is standard c++ syntax for passing an array by reference. It doesn't work without the parens. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:174: , IXpsOMMatrixTransform* xpsMatrixToUse On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > const ref? When in Rome... Because COM is designed around runtime binding, no consts are ever used in declarations, since there is no enforcement. As a result, if I mark any of these as const then the first thing I will have to do is cast away the const in every method to do anything with them. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:199: const SkPoint (&points)[4] On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > & usually touches the type, not the name See above, this is how to pass array by reference. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:208: IXpsOMObjectFactory* xpsFactory On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > const ref? See above rant on COM. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:209: , IXpsOMCanvas* page On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > conf ref? See above rant on COM. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:210: , IXpsOMFontResource* font On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > const ref? See above rant on COM. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:212: , XPS_GLYPH_INDEX* xpsGlyphs On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > const ref? See above rant on COM. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:214: , XPS_POINT *origin On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > const ref? See above rant on COM. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:218: , const SkPaint* paintPtr On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > const ref instead of const ptr? Will be done. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:219: , const SkDraw& d); On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > draw is usually the first arg Done.
Sign in to reply to this message.
I hope you take these comments, but generally LGTM http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp#n... src/device/xps/SkXPSDevice.cpp:179: : SkDevice(make_content_bitmap(10000,10000)) On 2011/09/21 20:09:28, bungeman wrote: > On 2011/09/19 23:41:45, Steve VanDeBogart wrote: > > Webkit asks the device for its width and height, so putting constants here > will > > break things. > > nit: comma space > > I'll have to take a look at how that works. The Chrome print device had height > and width specified for each page (not that anyone took advantage of it). We're > going to have to be careful about this anyway, as the size of the device may be > unrelated to what is wanted for layout. Did you get a chance to look at this? http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp File gyp/xps.gyp (right): http://codereview.appspot.com/5076041/diff/24014/gyp/xps.gyp#newcode39 gyp/xps.gyp:39: '../include/device/xps/SkXPSDevice.h', On 2011/10/04 16:18:27, bungeman wrote: > On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > > These are already out of sync... Can you use wildcards to exclude everything > in > > */device/xps/* > > These are, in fact, still in sync. There is no need to exclude SkConstexprMath.h > simply because we're not on Windows. This is actually a flag that it should > probably go somewhere else, though this may need to happen later. While there is no need to exclude it, there is also no reason to include it either. Right now the only consumer is the code in device/xps, which will only be useful on Windows. If there's a consumer outside of XPS, then it should probably be moved into core or util. I still think a wildcard in the exclude would be less fragile. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:11: #include "SkTypes.h" On 2011/10/04 16:18:27, bungeman wrote: > On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > > Alphabetical with the rest below? > > SkTypes.h is used in Skia to include stdint.h and windows.h. After the number of > issues I went through to discover this, I think SkTypes.h should just be named > SkIncludeMeFirst.h. If this isn't first, things get frustrating the same way as > not including windows.h first. Any header that needs definitions from SkTypes.h should include it, so there shouldn't be any problem putting it in alphabetical order with the rest of the includes. If doing that does give you a problem, then there's probably a missing include somewhere. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:42: virtual bool beginPortfolio(SkWStream* outputStream); On 2011/10/04 16:18:27, bungeman wrote: > On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > > More generic names might be better if you want this interface to become part > of > > SkDevice. begin/endDocument, begin/endPage ? > > Do you mean more specific? A portfolio of canvases can mean a video, a slide > show, a book, or an unrelated collection of artwork. A document of pages implies > an ordered collection of identically sized surfaces with a single content broken > more or less arbitrarily at fixed intervals. My suggestion was based on the assumption that portfolio is a specific term used in XPS. If it is, then I prefer Document over Portfolio because Document is a more generic term. I prefer page over canvas because canvas has a technical meaning in Skia. i.e. why doesn't beginCanvas return an SkCanvas? http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkConstex... File include/device/xps/SkConstexprMath.h (right): http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkConstex... include/device/xps/SkConstexprMath.h:9: #define SkConstexprMath_DEFINED This does seem like it should go in /core... http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkConstex... include/device/xps/SkConstexprMath.h:12: #include <limits.h> Skia style is under specified, so the following comment at your option: Google/Chrome style says to include system headers first (in one alphabetical block), followed by a blank line than then project headers in alphabetical order. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:28: struct TypefaceUse { Put this in the private part of SkXPSDevice, there's no reason to publicly expose it. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:64: virtual bool beginCanvas( nit: It looks like the arguments would fit if you put the first one on line 64 and lined up the rest underneath them, but again under specified Skia style. Here and below. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:76: virtual uint32_t getDeviceCapabilities() { SK_OVERRIDE http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:80: virtual void clear(SkColor color); SK_OVERRIDE http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:82: virtual bool readPixels(const SkIRect& srcRect, SkBitmap* bitmap) { SK_OVERRIDE http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:86: virtual void drawPaint(const SkDraw&, const SkPaint& paint); SK_OVERRIDE http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:167: SkTDArray<TypefaceUse> fTypefaces; nit/cleanup: Now that core has SkTArray, consider using that along with a definition of TypefaceUse that manages its own memory. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:182: const SkPaint& paint, nit: paint is usually the last argument to drawing method http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:250: const SkPaint& paintPtr); nit: paintPtr -> paint ? http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:254: bool stroke, bool fill, const SkPath& path); Should these be BOOL? http://codereview.appspot.com/5076041/diff/40001/include/utils/win/SkHRESULT.h File include/utils/win/SkHRESULT.h (right): http://codereview.appspot.com/5076041/diff/40001/include/utils/win/SkHRESULT.... include/utils/win/SkHRESULT.h:14: , HRESULT hr, const char* msg); nit: comma http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:43: //Windows defines a FLOAT type, nit: // comments in Skia seem to have a space before the first word. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:67: buffer, nit: if no argument is longer than the space to the end of the line, then put the first argument on the same line as the method call and line up arguments with it. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:85: ) == -1) { nit: put on previous line. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:100: , fCurrentPage(0) { nit: comma http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:121: ), nit: prev line [several occurrences in this file] http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:123: ); nit: prev line [several occurrences in this file] http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:136: const SkRect* mediaBox, If you aren't going to use these arguments, maybe remove them from the definition, at least until you do start to use them. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:271: static const float targetUnitsPerMeter = 96 * inchesPerMeter; nit: make 96 a constant? http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:463: //Manhattan distance between transformed start and end. I'm not following what this function is doing. Do you just want to scale stopOffsets up by the magnitude of the transformed start to end vector? http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:592: static const XPS_TILE_MODE N = XPS_TILE_MODE_NONE; This kind of pollutes the namespace (at least in this file). I assume you did this because using the full names in the table made it not fit well? consider XTM_* or something else to make it a bit more unique. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:672: nit: extra line http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:674: const SkScalar BIG = SkIntToScalar(1000); //SK_ScalarMax; This may not be sufficient in some cases. You can use the page bounds and the transform to ensure that it is big enough. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1006: if (SkShader::kNone_GradientType == gradientType) { The control structure for gradients is kind of confusing. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1123: || paint.getMaskFilter() boolean ops are generally at the end of the prev line http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1190: //TODO: override this for XPS consider SkASSERT(false) for these or some other way to indicate to users that they've hit an not-implemented method. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1192: void SkXPSDevice::drawPoints(const SkDraw& d, SkCanvas::PointMode mode, Just omit this, since it just calls the parent... unless you intend to do something with it in the future, in which case, add a TODO? http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1202: void SkXPSDevice::drawTextOnPath(const SkDraw& d, const void* text, size_t len, ditto http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1265: HRV(this->shadePath(shadedPath.get(), nit: fits on one line http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1457: //Note: this is not quite right when there is nothing solid in the Interesting... I can probably use this trick in PDF too. Could you improve it by drawing white clipped to the path just before drawing the old content? http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1764: void SkXPSDevice::drawPath(const SkDraw& d, This method is pretty big, pulling some of the content out into sub-methods probably wouldn't hurt. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1864: filter, //just to compute how much to draw. nit: two spaces before // comment. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:2549: : SkDevice(make_fake_bitmap(10000, 10000)) make this a shared constant http://codereview.appspot.com/5076041/diff/40001/src/utils/win/SkHRESULT.cpp File src/utils/win/SkHRESULT.cpp (right): http://codereview.appspot.com/5076041/diff/40001/src/utils/win/SkHRESULT.cpp#... src/utils/win/SkHRESULT.cpp:14: { nit: prev line
Sign in to reply to this message.
Addressed most of the outstanding comments and a few other things found along the way. http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp#n... src/device/xps/SkXPSDevice.cpp:179: : SkDevice(make_content_bitmap(10000,10000)) On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > On 2011/09/21 20:09:28, bungeman wrote: > > On 2011/09/19 23:41:45, Steve VanDeBogart wrote: > > > Webkit asks the device for its width and height, so putting constants here > > will > > > break things. > > > nit: comma space > > > > I'll have to take a look at how that works. The Chrome print device had > height > > and width specified for each page (not that anyone took advantage of it). > We're > > going to have to be careful about this anyway, as the size of the device may > be > > unrelated to what is wanted for layout. > > Did you get a chance to look at this? After some code spelunking, I'm fairly sure WebKit doesn't care what we set this to. The choke point is at src\chrome\renderer\print_web_view_helper_win.cc : 201 float webkit_scale_factor = frame->printPage(page_number, canvas.get()); The value in the device is ignored prior to this (it is pushed out through StartPageForVectorCanvas). After this point, in WebFrameImpl::printPage, the canvas is wrapped in a GraphicsContext for the remainder of the render. GraphicsContext is write only, and all of the size information comes from the WebFrame. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:11: #include "SkTypes.h" On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > On 2011/10/04 16:18:27, bungeman wrote: > > On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > > > Alphabetical with the rest below? > > > > SkTypes.h is used in Skia to include stdint.h and windows.h. After the number > of > > issues I went through to discover this, I think SkTypes.h should just be named > > SkIncludeMeFirst.h. If this isn't first, things get frustrating the same way > as > > not including windows.h first. > > Any header that needs definitions from SkTypes.h should include it, so there > shouldn't be any problem putting it in alphabetical order with the rest of the > includes. If doing that does give you a problem, then there's probably a > missing include somewhere. Yes, but the issue is that *this* file needs Windows.h before its own includes of other windows headers. SkTypes.h includes Windows.h with WIN32_LEAN_AND_MEAN, but then undefines this if it defines it. As a result, it is really easy to include Windows.h once through SkTypes.h and once again in the file which needs it, but with different settings for WIN32_LEAN_AND_MEAN, leading to much confusion. I agree there is a missing include somewhere, and that somewhere is all the Windows headers which don't include Windows.h. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:42: virtual bool beginPortfolio(SkWStream* outputStream); On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > On 2011/10/04 16:18:27, bungeman wrote: > > On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > > > More generic names might be better if you want this interface to become part > > of > > > SkDevice. begin/endDocument, begin/endPage ? > > > > Do you mean more specific? A portfolio of canvases can mean a video, a slide > > show, a book, or an unrelated collection of artwork. A document of pages > implies > > an ordered collection of identically sized surfaces with a single content > broken > > more or less arbitrarily at fixed intervals. > > My suggestion was based on the assumption that portfolio is a specific term used > in XPS. If it is, then I prefer Document over Portfolio because Document is a > more generic term. > > I prefer page over canvas because canvas has a technical meaning in Skia. i.e. > why doesn't beginCanvas return an SkCanvas? I don't think the term portfolio and XPS have ever shown up together in a document before this. Interestingly, after an informal poll at trivia night, the portfolio/canvas vs document/page seems to be split along age lines. Those who have been around longer tend to think of 'document' as something you put in a file drawer, and so think of 'portfolio' as being more generic. Those who are younger seem to think of 'document' in terms of web pages which can sing and dance and so they have a more vague idea about what a 'document' is. As a result the younger set thinks 'document' is more general and, aside from the artsy ones, don't know much about portfolios. I like portfolios as they're literally "a thing to carry sheets for writing upon" (the thing) whereas documents are "a teaching or instruction" (the content). This is not unlike the window/document split in the browser itself. If you're objecting more strongly to canvas as being confusing (though also suggestive) perhaps portfolio/sheet would work better. English has a lot of words, so I'm sure one could argue the color of this bike shed for quite some time. Also, this is a long comment. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:28: struct TypefaceUse { On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > Put this in the private part of SkXPSDevice, there's no reason to publicly > expose it. Done. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:76: virtual uint32_t getDeviceCapabilities() { On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > SK_OVERRIDE Done. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:80: virtual void clear(SkColor color); On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > SK_OVERRIDE Done. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:82: virtual bool readPixels(const SkIRect& srcRect, SkBitmap* bitmap) { On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > SK_OVERRIDE Done. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:86: virtual void drawPaint(const SkDraw&, const SkPaint& paint); On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > SK_OVERRIDE Done. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:167: SkTDArray<TypefaceUse> fTypefaces; On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit/cleanup: Now that core has SkTArray, consider using that along with a > definition of TypefaceUse that manages its own memory. Done. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:182: const SkPaint& paint, On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: paint is usually the last argument to drawing method Done. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:250: const SkPaint& paintPtr); On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: paintPtr -> paint ? Done. Oops, missed that when updating implementation. http://codereview.appspot.com/5076041/diff/40001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:254: bool stroke, bool fill, const SkPath& path); On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > Should these be BOOL? Done. Yes, in fact, they should. http://codereview.appspot.com/5076041/diff/40001/include/utils/win/SkHRESULT.h File include/utils/win/SkHRESULT.h (right): http://codereview.appspot.com/5076041/diff/40001/include/utils/win/SkHRESULT.... include/utils/win/SkHRESULT.h:14: , HRESULT hr, const char* msg); On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: comma Done. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:67: buffer, On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: if no argument is longer than the space to the end of the line, then put > the first argument on the same line as the method call and line up arguments > with it. Refactored this so the 'if' conditional isn't so long. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:85: ) == -1) { On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: put on previous line. Now all on one line. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:100: , fCurrentPage(0) { On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: comma This is how the skia style guide says to do initializer lists. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:121: ), On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: prev line [several occurrences in this file] Done. I like blocks to look like blocks, but I suppose this lets more be seen on a screen. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:123: ); On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: prev line [several occurrences in this file] Done. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:136: const SkRect* mediaBox, On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > If you aren't going to use these arguments, maybe remove them from the > definition, at least until you do start to use them. I actually have some code saved off to start using these (setting up a default print ticket) but this code review is already too big. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:271: static const float targetUnitsPerMeter = 96 * inchesPerMeter; On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: make 96 a constant? Done. It is a constant, there it is :) I managed to use it exactly once, right here, and no one else needs to know. However, I now dub it xpsDPI. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:463: //Manhattan distance between transformed start and end. On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > I'm not following what this function is doing. Do you just want to scale > stopOffsets up by the magnitude of the transformed start to end vector? Yes, but because this is perspective, it isn't linear. I could write the sqrt version, but it seemed easier to let the matrix do the mult version for me. This function is only called when faking a perspective gradient. The stops will end up being correct, but the gradient itself will not be quite right. A more advanced version would figure out when things are getting out of step above a certain delta and add stops. In non-perspective, an xps transform handles the affine transform and this whole thing is skipped. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:592: static const XPS_TILE_MODE N = XPS_TILE_MODE_NONE; On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > This kind of pollutes the namespace (at least in this file). I assume you did > this because using the full names in the table made it not fit well? consider > XTM_* or something else to make it a bit more unique. Done. Good idea. Yes, with the full names this table is really wide. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:672: On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: extra line Done. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:674: const SkScalar BIG = SkIntToScalar(1000); //SK_ScalarMax; On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > This may not be sufficient in some cases. You can use the page bounds and the > transform to ensure that it is big enough. I put a TODO in for this. I would just make this a huge number, but it turns out this makes rasterizers do strange things. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1006: if (SkShader::kNone_GradientType == gradientType) { On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > The control structure for gradients is kind of confusing. Yes, at the very least the gradient and bitmap parts need to be split into their own methods. I'll end up being forced to do the split when implementing more of these. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1123: || paint.getMaskFilter() On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > boolean ops are generally at the end of the prev line Done. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1190: //TODO: override this for XPS On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > consider SkASSERT(false) for these or some other way to indicate to users that > they've hit an not-implemented method. Added SkDEBUGF. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1192: void SkXPSDevice::drawPoints(const SkDraw& d, SkCanvas::PointMode mode, On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > Just omit this, since it just calls the parent... unless you intend to do > something with it in the future, in which case, add a TODO? This now calls the correct method on the passed in SkDraw. The super class wasn't doing exactly what I wanted anyway. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1202: void SkXPSDevice::drawTextOnPath(const SkDraw& d, const void* text, size_t len, On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > ditto See above on drawPoints. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1265: HRV(this->shadePath(shadedPath.get(), On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: fits on one line Done. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1457: //Note: this is not quite right when there is nothing solid in the On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > Interesting... I can probably use this trick in PDF too. Could you improve it > by drawing white clipped to the path just before drawing the old content? The trick is to know what it's going to be composited with, this could be in a layer. Also note that this ends up with n^2 layers (where n is the number of times this is used) so one has to trust that the rasterizer which ends up being used was well written. http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1764: void SkXPSDevice::drawPath(const SkDraw& d, On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > This method is pretty big, pulling some of the content out into sub-methods > probably wouldn't hurt. Hmmm... maybe, shadePath, applyMask, and convertToPpm have all already been pulled out of this. 254 lines is still a little long I suppose. The raster bits could probably be pulled out (rasterizer and maskfilter). http://codereview.appspot.com/5076041/diff/40001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:1864: filter, //just to compute how much to draw. On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: two spaces before // comment. Done. http://codereview.appspot.com/5076041/diff/40001/src/utils/win/SkHRESULT.cpp File src/utils/win/SkHRESULT.cpp (right): http://codereview.appspot.com/5076041/diff/40001/src/utils/win/SkHRESULT.cpp#... src/utils/win/SkHRESULT.cpp:14: { On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > nit: prev line Done.
Sign in to reply to this message.
http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/1/src/device/xps/SkXPSDevice.cpp#n... src/device/xps/SkXPSDevice.cpp:179: : SkDevice(make_content_bitmap(10000,10000)) On 2011/10/06 16:48:38, bungeman wrote: > On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > > On 2011/09/21 20:09:28, bungeman wrote: > > > On 2011/09/19 23:41:45, Steve VanDeBogart wrote: > > > > Webkit asks the device for its width and height, so putting constants here > > > will > > > > break things. > > > > nit: comma space > > > > > > I'll have to take a look at how that works. The Chrome print device had > > height > > > and width specified for each page (not that anyone took advantage of it). > > We're > > > going to have to be careful about this anyway, as the size of the device may > > be > > > unrelated to what is wanted for layout. > > > > Did you get a chance to look at this? > > After some code spelunking, I'm fairly sure WebKit doesn't care what we set this > to. The choke point is at > > src\chrome\renderer\print_web_view_helper_win.cc : 201 > float webkit_scale_factor = frame->printPage(page_number, canvas.get()); > > The value in the device is ignored prior to this (it is pushed out through > StartPageForVectorCanvas). After this point, in WebFrameImpl::printPage, the > canvas is wrapped in a GraphicsContext for the remainder of the render. > GraphicsContext is write only, and all of the size information comes from the > WebFrame. It was a problem at some point for PDF. I doubt it has changed, but as long as you're aware of the potential problem, that's good enough for me. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:11: #include "SkTypes.h" On 2011/10/06 16:48:38, bungeman wrote: > On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > > On 2011/10/04 16:18:27, bungeman wrote: > > > On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > > > > Alphabetical with the rest below? > > > > > > SkTypes.h is used in Skia to include stdint.h and windows.h. After the > number > > of > > > issues I went through to discover this, I think SkTypes.h should just be > named > > > SkIncludeMeFirst.h. If this isn't first, things get frustrating the same way > > as > > > not including windows.h first. > > > > Any header that needs definitions from SkTypes.h should include it, so there > > shouldn't be any problem putting it in alphabetical order with the rest of the > > includes. If doing that does give you a problem, then there's probably a > > missing include somewhere. > > Yes, but the issue is that *this* file needs Windows.h before its own includes > of other windows headers. SkTypes.h includes Windows.h with WIN32_LEAN_AND_MEAN, > but then undefines this if it defines it. As a result, it is really easy to > include Windows.h once through SkTypes.h and once again in the file which needs > it, but with different settings for WIN32_LEAN_AND_MEAN, leading to much > confusion. > > I agree there is a missing include somewhere, and that somewhere is all the > Windows headers which don't include Windows.h. I don't really do Windows, so I'll take your word for it. Would it make sense to have SkWindows.h that include windows.h with the appropriate defines, and then have the rest of the Skia headers include that instead? http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:42: virtual bool beginPortfolio(SkWStream* outputStream); On 2011/10/06 16:48:38, bungeman wrote: > On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > > On 2011/10/04 16:18:27, bungeman wrote: > > > On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > > > > More generic names might be better if you want this interface to become > part > > > of > > > > SkDevice. begin/endDocument, begin/endPage ? > > > > > > Do you mean more specific? A portfolio of canvases can mean a video, a slide > > > show, a book, or an unrelated collection of artwork. A document of pages > > implies > > > an ordered collection of identically sized surfaces with a single content > > broken > > > more or less arbitrarily at fixed intervals. > > > > My suggestion was based on the assumption that portfolio is a specific term > used > > in XPS. If it is, then I prefer Document over Portfolio because Document is a > > more generic term. > > > > I prefer page over canvas because canvas has a technical meaning in Skia. i.e. > > why doesn't beginCanvas return an SkCanvas? > > I don't think the term portfolio and XPS have ever shown up together in a > document before this. > > Interestingly, after an informal poll at trivia night, the portfolio/canvas vs > document/page seems to be split along age lines. Those who have been around > longer tend to think of 'document' as something you put in a file drawer, and so > think of 'portfolio' as being more generic. Those who are younger seem to think > of 'document' in terms of web pages which can sing and dance and so they have a > more vague idea about what a 'document' is. As a result the younger set thinks > 'document' is more general and, aside from the artsy ones, don't know much about > portfolios. > > I like portfolios as they're literally "a thing to carry sheets for writing > upon" (the thing) whereas documents are "a teaching or instruction" (the > content). This is not unlike the window/document split in the browser itself. > > If you're objecting more strongly to canvas as being confusing (though also > suggestive) perhaps portfolio/sheet would work better. English has a lot of > words, so I'm sure one could argue the color of this bike shed for quite some > time. Also, this is a long comment. Sorry, bad assumption on my part re portfolio. If you're willing, sheet sounds better than canvas. http://codereview.appspot.com/5076041/diff/48001/include/pdf/SkBitSet.h File include/pdf/SkBitSet.h (right): http://codereview.appspot.com/5076041/diff/48001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:17: class SkBitSet : public SkRefCnt { Does this require changing the other consumers of SkBitSet? http://codereview.appspot.com/5076041/diff/48001/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/48001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:123: SkXPSDevice::TypefaceUse::TypefaceUse(const TypefaceUse& that) Is there a caller for this, I couldn't find it? If reasonable, I'd rather not make SkBitSet ref counted and I think this is what caused you to make that change. http://codereview.appspot.com/5076041/diff/48001/src/utils/win/SkHRESULT.cpp File src/utils/win/SkHRESULT.cpp (right): http://codereview.appspot.com/5076041/diff/48001/src/utils/win/SkHRESULT.cpp#... src/utils/win/SkHRESULT.cpp:18: FormatMessageA( nit: style here
Sign in to reply to this message.
I changed SkTArray (r2433 and r2434) so that I could remove the unneeded ref counting. http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/24014/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:42: virtual bool beginPortfolio(SkWStream* outputStream); On 2011/10/06 20:32:58, Steve VanDeBogart wrote: > On 2011/10/06 16:48:38, bungeman wrote: > > On 2011/10/04 23:33:13, Steve VanDeBogart wrote: > > > On 2011/10/04 16:18:27, bungeman wrote: > > > > On 2011/09/26 20:46:57, Steve VanDeBogart wrote: > > > > > More generic names might be better if you want this interface to become > > part > > > > of > > > > > SkDevice. begin/endDocument, begin/endPage ? > > > > > > > > Do you mean more specific? A portfolio of canvases can mean a video, a > slide > > > > show, a book, or an unrelated collection of artwork. A document of pages > > > implies > > > > an ordered collection of identically sized surfaces with a single content > > > broken > > > > more or less arbitrarily at fixed intervals. > > > > > > My suggestion was based on the assumption that portfolio is a specific term > > used > > > in XPS. If it is, then I prefer Document over Portfolio because Document is > a > > > more generic term. > > > > > > I prefer page over canvas because canvas has a technical meaning in Skia. > i.e. > > > why doesn't beginCanvas return an SkCanvas? > > > > I don't think the term portfolio and XPS have ever shown up together in a > > document before this. > > > > Interestingly, after an informal poll at trivia night, the portfolio/canvas vs > > document/page seems to be split along age lines. Those who have been around > > longer tend to think of 'document' as something you put in a file drawer, and > so > > think of 'portfolio' as being more generic. Those who are younger seem to > think > > of 'document' in terms of web pages which can sing and dance and so they have > a > > more vague idea about what a 'document' is. As a result the younger set thinks > > 'document' is more general and, aside from the artsy ones, don't know much > about > > portfolios. > > > > I like portfolios as they're literally "a thing to carry sheets for writing > > upon" (the thing) whereas documents are "a teaching or instruction" (the > > content). This is not unlike the window/document split in the browser itself. > > > > If you're objecting more strongly to canvas as being confusing (though also > > suggestive) perhaps portfolio/sheet would work better. English has a lot of > > words, so I'm sure one could argue the color of this bike shed for quite some > > time. Also, this is a long comment. > > Sorry, bad assumption on my part re portfolio. If you're willing, sheet sounds > better than canvas. Sheet it is. http://codereview.appspot.com/5076041/diff/48001/include/pdf/SkBitSet.h File include/pdf/SkBitSet.h (right): http://codereview.appspot.com/5076041/diff/48001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:17: class SkBitSet : public SkRefCnt { On 2011/10/06 20:32:58, Steve VanDeBogart wrote: > Does this require changing the other consumers of SkBitSet? I don't think it did, but it's gone now. http://codereview.appspot.com/5076041/diff/48001/src/device/xps/SkXPSDevice.cpp File src/device/xps/SkXPSDevice.cpp (right): http://codereview.appspot.com/5076041/diff/48001/src/device/xps/SkXPSDevice.c... src/device/xps/SkXPSDevice.cpp:123: SkXPSDevice::TypefaceUse::TypefaceUse(const TypefaceUse& that) On 2011/10/06 20:32:58, Steve VanDeBogart wrote: > Is there a caller for this, I couldn't find it? If reasonable, I'd rather not > make SkBitSet ref counted and I think this is what caused you to make that > change. This was added in order to appease SkTArray. I have since changed SkTArray and removed this. http://codereview.appspot.com/5076041/diff/48001/src/utils/win/SkHRESULT.cpp File src/utils/win/SkHRESULT.cpp (right): http://codereview.appspot.com/5076041/diff/48001/src/utils/win/SkHRESULT.cpp#... src/utils/win/SkHRESULT.cpp:18: FormatMessageA( On 2011/10/06 20:32:58, Steve VanDeBogart wrote: > nit: style here Done.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/5076041/diff/61001/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/61001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:150: class TypefaceUse : ::SkNoncopyable { nit: :: isn't generally used in Skia. nit: : public SkNoncopyable http://codereview.appspot.com/5076041/diff/61001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:155: SkBitSet* glyphsUsed; nit: It doesn't look like you need this to be a pointer, just make it a member?
Sign in to reply to this message.
Some excuses. http://codereview.appspot.com/5076041/diff/61001/include/device/xps/SkXPSDevi... File include/device/xps/SkXPSDevice.h (right): http://codereview.appspot.com/5076041/diff/61001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:150: class TypefaceUse : ::SkNoncopyable { On 2011/10/07 22:09:19, Steve VanDeBogart wrote: > nit: :: isn't generally used in Skia. > nit: : public SkNoncopyable I'm still not sure what the rule is here, but I have to do this to get vc++ to compile this. I get an error without the :: because SkXpsDevice -> SkDevice -> SkRefCnt -> SkNoncopyable and that last '->' is private. I can't say I 100% understand why yet. http://codereview.appspot.com/5076041/diff/61001/include/device/xps/SkXPSDevi... include/device/xps/SkXPSDevice.h:155: SkBitSet* glyphsUsed; On 2011/10/07 22:09:19, Steve VanDeBogart wrote: > nit: It doesn't look like you need this to be a pointer, just make it a member? I need to know the number of bits before creating one. I query the font cache for the number of glyphs.
Sign in to reply to this message.
Fair enough - LGTM
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
