[webkit-reviews] review granted: [Bug 130168] RenderTextControl::hasValidAvgCharWidth doesn't detect System Font : [Attachment 226570] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 130168: RenderTextControl::hasValidAvgCharWidth doesn't detect System Font
https://bugs.webkit.org/show_bug.cgi?id=130168

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

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


> Source/WebCore/ChangeLog:24
> +	   * platform/graphics/Font.cpp:
> +	   (WebCore::Font::hasValidAvgCharWidth):
> +	   (WebCore::Font::fastAvgCharWidth):
> +	   * platform/graphics/Font.h:
> +	   * rendering/RenderTextControl.cpp:
> +	   (WebCore::RenderTextControl::getAvgCharWidth):
> +	   (WebCore::RenderTextControl::computeIntrinsicLogicalWidths):
> +	   * rendering/RenderTextControl.h:
> +	   * rendering/RenderTextControlMultiLine.cpp:
> +	   (WebCore::RenderTextControlMultiLine::getAvgCharWidth):
> +	   * rendering/RenderTextControlMultiLine.h:
> +	   * rendering/RenderTextControlSingleLine.cpp:
> +	   (WebCore::RenderTextControlSingleLine::getAvgCharWidth):
> +	  
(WebCore::RenderTextControlSingleLine::preferredContentLogicalWidth):
> +	   * rendering/RenderTextControlSingleLine.h:

You should provide some per-file descriptions!

> Source/WebCore/platform/graphics/Font.cpp:501
> +	   width = roundf(primaryFont()->avgCharWidth()); // FIXME:
primaryFont() might not correspond to firstFamily()

. at end of comment

> Source/WebCore/platform/graphics/Font.h:157
> +    bool fastAvgCharWidth(float &width) const; // returns true on success

I wish the name was more clear. This looks like a getter, but is actually
something more tricky. Maybe computeFastAvgCharWidth()? (even though there is
no computation)

The other places where we do things like this seem to all be in the Audio code
(mostly from a single author).

Also, let's take this opportunity to expand Avg to Average