[webkit-reviews] review granted: [Bug 54452] Optimize Color::serialized() : [Attachment 82442] Proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Feb 15 09:55:58 PST 2011
Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 54452: Optimize Color::serialized()
https://bugs.webkit.org/show_bug.cgi?id=54452
Attachment 82442: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=82442&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=82442&action=review
Looks good.
> Source/WebCore/platform/graphics/Color.cpp:183
> +static inline void appendHexNumber(Vector<UChar>& vector, unsigned char
number)
In newer code we’ve been using uint8_t for this more and more. Maybe we should
make that a standard thing?
> Source/WebCore/platform/graphics/Color.cpp:185
> + static const char hexDigits[17] = "0123456789abcdef";
Any particular reason to use lowercase hex here? I normally prefer uppercase
when we have a choice. I guess the old code was doing lowercase.
> Source/WebCore/platform/graphics/Color.cpp:191
> + size_t vectorSize = vector.size();
> + vector.grow(vectorSize + 2);
> +
> + vector[vectorSize] = hexDigits[number >> 4];
> + vector[vectorSize + 1] = hexDigits[number & 0xF];
Is this more efficient than calling append twice?
> Source/WebCore/platform/graphics/Color.cpp:203
> + result.reserveInitialCapacity(8);
Why 8 and not 7?
> Source/WebCore/platform/graphics/Color.cpp:207
> + appendHexNumber(result, static_cast<unsigned char>(red()));
> + appendHexNumber(result, static_cast<unsigned char>(green()));
> + appendHexNumber(result, static_cast<unsigned char>(blue()));
Are these casts needed? Doesn’t the type conversion happen without a warning
even without the cast?
> Source/WebCore/platform/graphics/Color.cpp:208
> + return String::adopt(result);
Strings use memory more efficiently if they are created with the text in them,
because then they use a single block. When we adopt a vector we do extra work
that costs both space and also a bit of time.
For this code path, we could just use String::createUninitialized since we know
the exact length ahead of time.
We could do the same in the other code path for 0 alpha.
More information about the webkit-reviews
mailing list