[webkit-reviews] review granted: [Bug 136484] Add support for the initial-letter CSS property to first-letter : [Attachment 237561] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 136484: Add support for the initial-letter CSS property to first-letter
https://bugs.webkit.org/show_bug.cgi?id=136484

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237561&action=review


> Source/WebCore/css/CSSValueKeywords.in:974
> +initial-letter

Do you actually ever use this?

> Source/WebCore/platform/graphics/FontMetrics.h:102
> -
> +    

whoopsy

> Source/WebCore/rendering/RenderBlock.cpp:3484
> +	   while (actualCapHeight > desiredCapHeight) {

Would this ever be slow?

> Source/WebCore/rendering/RenderBlock.cpp:3487
> +	      
newFontDescription.setComputedSize(newFontDescription.computedSize() -1);

Nit: missing space between - and 1

> Source/WebCore/rendering/style/RenderStyle.h:1087
> +    const IntSize& initialLetter() const { return
rareNonInheritedData->m_initialLetter; }
> +    int initialLetterDrop() const { return initialLetter().width(); }
> +    int initialLetterHeight() const { return initialLetter().height(); }

This is a bit weird - maybe initialLetter() should be private, so that people
won't ever think that the IntSize.width() is an actual width.

> Source/WebCore/rendering/style/RenderStyle.h:1620
> +    void setInitialLetter(const IntSize& size) {
SET_VAR(rareNonInheritedData, m_initialLetter, size); }

Again, a bit weird that you need to know what is width and what is height.