[webkit-reviews] review granted: [Bug 47226] Cache CGColor as we do NSColor : [Attachment 69866] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 47226: Cache CGColor as we do NSColor
https://bugs.webkit.org/show_bug.cgi?id=47226

Attachment 69866: Patch
https://bugs.webkit.org/attachment.cgi?id=69866&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 This fixes performance problems on certain web pages that use

I assume you can't mention an example. Can a manual test be made?

+			developmentRegion = English;

Please don't land this.


-    CGColorRef debugColor = createCGColor(Color(255, 0, 0, 204));
-    m_rootLayer->setBackgroundColor(debugColor);
-    CGColorRelease(debugColor);
+    m_rootLayer->setBackgroundColor(cachedCGColor(Color(255, 0, 0, 204),
DeviceColorSpace));

Even though it's inside ifdef NDEBUG, the "debugColor" name seemed helpful. And
I don't think that we should be caching it.


     if (m_owner->showDebugBorders()) {
-	 CGColorRef borderColor = createCGColor(Color(128, 0, 128, 180));
-	 CACFLayerSetBorderColor(newLayer.get(), borderColor);
-	 CGColorRelease(borderColor);
+	 CACFLayerSetBorderColor(newLayer.get(), cachedCGColor(Color(128, 0,
128, 180), DeviceColorSpace));

Ditto.

I don't know a lo about CGColor, but since this just applies the same technique
that we used with NSColors, r=me.

I'm wondering why we can't store the platform color inside Color, and have to
iterate over the vector in a disconnected cache. Are there so many Color
objects that it's a memory use issue? Or are they transient, so storing the
CGColor inside does no good?