| Summary: | Merge ColorSpace and ImageColorSpace enums | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, bdakin, eric, mdelaney7, webkit-ews, webkit.review.bot, zimmermann | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Dirk Schulze
2010-10-19 13:12:21 PDT
Created attachment 71198 [details]
Patch
Comment on attachment 71198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71198&action=review Looks good overall, except for some minor things. But I'd really like to see whether we can avoid code duplication, by not using RetainPtr for the colorspace in ImageBuffer. > WebCore/css/CSSPrimitiveValueMappings.h:2198 > + default: default is not added here on purpose, so the compiler warns if anything is missing. If you need to add sth. add the real case value. > WebCore/platform/graphics/ColorSpace.h:34 > + sRGBColorSpace, > + linearRGBColorSpace I don't like these names, style guide says "Enum members should user InterCaps with an initial capital letter.". So rename to SRGB and LinearRGB.. > WebCore/platform/graphics/cg/ImageBufferCG.cpp:83 > + case DeviceColorSpace: > + m_data.m_colorSpace.adoptCF(CGColorSpaceCreateDeviceRGB()); > + break; Can you investigate, if it's possible to not use a RetainPtr for m_colorSpace. Then we could use the deviceRGBRef() static methods, instead of duplication code. Created attachment 71272 [details]
Patch
Attachment 71272 [details] did not build on qt: Build output: http://queues.webkit.org/results/4516016 Comment on attachment 71272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71272&action=review Other than that, looks good to me! > WebCore/css/CSSPrimitiveValueMappings.h:2199 > + ASSERT_NOT_REACHED(); You should comment here, that linearRgb is not available, through css "-webkit-color-correction". Otherwhise this is a bit confusing. Created attachment 71274 [details]
Patch
Comment on attachment 71274 [details]
Patch
Looks great, r=me.
Please do a leaks run first, as I'm not 100% certain about the removal of RetainPtr for m_colorSpace. Looks fine according to the CoreGraphics docs though, but better be safe than sorry.
I checked. No leaks found. *Phew* ;-) Committed r70143: <http://trac.webkit.org/changeset/70143> http://trac.webkit.org/changeset/70143 might have broken Qt Linux Release The following tests are not passing: http/tests/security/mixedContent/insecure-css-in-main-frame.html |