[webkit-reviews] review denied: [Bug 51456] Introduce FontMetrics abstraction : [Attachment 77512] Patch v10

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 51456: Introduce FontMetrics abstraction
https://bugs.webkit.org/show_bug.cgi?id=51456

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77512&action=review

I'm not sure why a struct is violating the style guidelines. Sometime a struct
is the better choice. But in this case I agree to Niko, because this code makes
use of various functions (with more than one line) and this should really be in
a header.

I'd like to see another run r-, but not because of the header itself.

> WebCore/platform/graphics/FontMetrics.h:19
> +/*
> +    Copyright (C) Research In Motion Limited 2010. All rights reserved.
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.
> +
> +    This library is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +    Library General Public License for more details.
> +
> +    You should have received a copy of the GNU Library General Public
License
> +    aint with this library; see the file COPYING.LIB.  If not, write to
> +    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +    Boston, MA 02110-1301, USA.
> +*/
> +

Can you start every line with a space and a star? Also why do you copy&paste
wrong licenses? aint? What is aint? Do you want to save memory? ;-)

> WebCore/platform/graphics/FontMetrics.h:112
> +private:

Do you think this 'private:' is helpful? I don't.

> WebCore/platform/graphics/SimpleFontData.cpp:85
> +    int xHeight = static_cast<int>(svgFontFaceElement->xHeight() * scale);
> +    int ascent = static_cast<int>(svgFontFaceElement->ascent() * scale);
> +    int descent = static_cast<int>(svgFontFaceElement->descent() * scale);

why not floorf?

> WebCore/platform/graphics/SimpleFontData.cpp:86
> +    int lineGap = 0.1f * size;

This doesn't cause a compiler warning?