Bug 47922 - Merge ColorSpace and ImageColorSpace enums
Summary: Merge ColorSpace and ImageColorSpace enums
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-10-19 13:12 PDT by Dirk Schulze
Modified: 2010-10-20 06:40 PDT (History)
7 users (show)

See Also:

Patch (24.89 KB, patch)
2010-10-19 13:29 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (124.74 KB, patch)
2010-10-20 04:30 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (127.97 KB, patch)
2010-10-20 05:09 PDT, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2010-10-19 13:12:21 PDT
I think we should merge the ColorSpace and ImageColorSpace enumerations. This mean adding linearRGB to ColorSpace and will be necessary for implementing color-interpolation as well as color-interpolation-filters on SVG.
Comment 1 Dirk Schulze 2010-10-19 13:29:31 PDT
Created attachment 71198 [details]
Comment 2 Nikolas Zimmermann 2010-10-20 01:40:22 PDT
Comment on attachment 71198 [details]

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.
Comment 3 Dirk Schulze 2010-10-20 04:30:01 PDT
Created attachment 71272 [details]
Comment 4 Early Warning System Bot 2010-10-20 04:48:21 PDT
Attachment 71272 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4516016
Comment 5 Nikolas Zimmermann 2010-10-20 05:00:30 PDT
Comment on attachment 71272 [details]

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.
Comment 6 Dirk Schulze 2010-10-20 05:09:17 PDT
Created attachment 71274 [details]
Comment 7 Nikolas Zimmermann 2010-10-20 06:04:38 PDT
Comment on attachment 71274 [details]

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.
Comment 8 Dirk Schulze 2010-10-20 06:07:24 PDT
I checked. No leaks found. *Phew* ;-)
Comment 9 Dirk Schulze 2010-10-20 06:10:29 PDT
Committed r70143: <http://trac.webkit.org/changeset/70143>
Comment 10 WebKit Review Bot 2010-10-20 06:40:39 PDT
http://trac.webkit.org/changeset/70143 might have broken Qt Linux Release
The following tests are not passing: