Bug 212918 – Extended Color: Switch ColorMac.mm's nsColor() function over to using TinyLRUCache

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Extended Color: Switch ColorMac.mm's nsColor() function over to using TinyLRUCache

[Sam Weinig]

Created attachment 401366 [details]
Patch

[Darin Adler]

Comment on attachment 401366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401366&action=review

> Source/WebCore/platform/graphics/Color.h:192
> +    bool isExtended() const { return !(m_colorData.simpleColorAndFlags & invalidSimpleColor); }

Not sure there is a benefit to this, but I think this can be constexpr.

> Source/WebCore/platform/graphics/Color.h:195
> +    const SimpleColor asSimple() const;

We might even be able to make this a constexpr if we are willing to move it to the header.

> Source/WebCore/platform/graphics/mac/ColorMac.mm:41
> +    return [NSColor colorWithCGColor:cachedCGColor(color)];

Is there a problem with populating both tiny cache each time? I guess not.

[Darin Adler]

Comment on attachment 401366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401366&action=review

>> Source/WebCore/platform/graphics/Color.h:192
>> +    bool isExtended() const { return !(m_colorData.simpleColorAndFlags & invalidSimpleColor); }
> 
> Not sure there is a benefit to this, but I think this can be constexpr.

Really tempting to also add isSimple().

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 401366 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401366&action=review
> 
> > Source/WebCore/platform/graphics/Color.h:192
> > +    bool isExtended() const { return !(m_colorData.simpleColorAndFlags & invalidSimpleColor); }
> 
> Not sure there is a benefit to this, but I think this can be constexpr.
> 

Currently, you can't make a constexpr Color at all, if I remember correctly due to the way the m_colorData union is initialized (though I might be misremembering this).

[Sam Weinig]

Created attachment 401397 [details]
Patch

[Darin Adler]

Comment on attachment 401397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401397&action=review

> Source/WebCore/platform/graphics/mac/ColorMac.mm:142
> +    ASSERT(color.isExtended() || color.asSimple().value());

Do we need to write this assertion? Will TinyLRUCache automatically assert the same thing for us?

> Source/WebCore/platform/graphics/mac/ColorMac.mm:145
> +    return cache.get().get(color).get();

"get, get, get"

Better than "kvetch, kvetch, kvetch" I guess.

[Sam Weinig]

(In reply to Darin Adler from comment #6)
> Comment on attachment 401397 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401397&action=review
> 
> > Source/WebCore/platform/graphics/mac/ColorMac.mm:142
> > +    ASSERT(color.isExtended() || color.asSimple().value());
> 
> Do we need to write this assertion? Will TinyLRUCache automatically assert
> the same thing for us?

Yeah, it will, by virtue of calling cachedCGColor() which is where I copied this assert from. I am actually not sure it is necessary in either place anymore.

> 
> > Source/WebCore/platform/graphics/mac/ColorMac.mm:145
> > +    return cache.get().get(color).get();
> 
> "get, get, get"
> 
> Better than "kvetch, kvetch, kvetch" I guess.

[Sam Weinig]

Created attachment 401406 [details]
Patch

[EWS]

Committed r262790: <https://trac.webkit.org/changeset/262790>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401406 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64162829>