[webkit-reviews] review denied: [Bug 27663] FontPlatformData for Qt requires mapping from FontWeight to QFont::Weight : [Attachment 33461] Path to add toQFontWeight functionality.

[bugzilla-daemon at webkit.org]

Adam Treat <treat at kde.org> has denied Mike Fenton
<mike.fenton at torchmobile.com>'s request for review:
Bug 27663: FontPlatformData for Qt requires mapping from FontWeight to
QFont::Weight
https://bugs.webkit.org/show_bug.cgi?id=27663

Attachment 33461: Path to add toQFontWeight functionality.
https://bugs.webkit.org/attachment.cgi?id=33461&action=review

------- Additional Comments from Adam Treat <treat at kde.org>
> +    static inline QFont::Weight toQFontWeight(FontWeight fontWeight)
> +    {
> +	   QFont::Weight weight;

Unnecessary.

> +	   switch (fontWeight) {
> +	   case FontWeight100:
> +	   case FontWeight200:
> +	       weight = QFont::Light;
> +	       break;

return QFont::Light;

> +	   case FontWeight600:
> +	       weight = QFont::DemiBold;

return QFont::DemiBold;

And so on...

> -    if (description.weight() >= FontWeight600) {
> -	   m_font.setWeight(QFont::Bold);
> -	   m_bold = true;
> -    } else
> -	   m_font.setWeight(QFont::Normal);
> +
> +    m_font.setWeight(toQFontWeight(description.weight()));
> +    m_bold = m_font.bold();

Seems unnecessary to keep 'm_bold' with what you are doing.  m_font.bold()
stores the state.

However, this appears to be a behavior change.	Are you sure it is correct?

How did you go about determining the proper mapping?

Did you run the layout tests before and after to see if anything changed?

Cheers,
Adam