Bug 27663 - FontPlatformData for Qt requires mapping from FontWeight to QFont::Weight
Summary: FontPlatformData for Qt requires mapping from FontWeight to QFont::Weight
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-24 12:25 PDT by Mike Fenton
Modified: 2009-07-27 14:32 PDT (History)
1 user (show)

See Also:


Attachments
Path to add toQFontWeight functionality. (3.60 KB, patch)
2009-07-24 12:32 PDT, Mike Fenton
manyoso: review-
Details | Formatted Diff | Diff
Revised Patch (4.20 KB, patch)
2009-07-27 11:07 PDT, Mike Fenton
manyoso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Fenton 2009-07-24 12:25:57 PDT
Based on FIXME's found in graphics/qt/FontPlatformDataQt.cpp and graphics/qt/FontCacheQt.cpp an update is required to map FontWeight to QFont::Weight values.
Comment 1 Mike Fenton 2009-07-24 12:32:44 PDT
Created attachment 33461 [details]
Path to add toQFontWeight functionality.
Comment 2 Adam Treat 2009-07-24 12:59:49 PDT
Comment on attachment 33461 [details]
Path to add toQFontWeight functionality.

> +    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
Comment 3 Mike Fenton 2009-07-27 11:07:04 PDT
Created attachment 33556 [details]
Revised Patch

Patch has been updated.

Based on feedback, toQFontWeight now returns values directly within intermediary variable.

Switch bold calculation to >Normal from >=Bold to ensure logic matches calculation provided by QFont.bold().

This does not break any tests.

m_bold variable is required by FontPlatformData and can not be removed without affecting other platforms.
Comment 4 Adam Treat 2009-07-27 11:29:46 PDT
Comment on attachment 33556 [details]
Revised Patch

Looks good to me.
Comment 5 Adam Treat 2009-07-27 14:32:12 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/qt/FontCacheQt.cpp
	M	WebCore/platform/graphics/qt/FontPlatformData.h
	M	WebCore/platform/graphics/qt/FontPlatformDataQt.cpp
Committed r46428
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/qt/FontCacheQt.cpp
	M	WebCore/platform/graphics/qt/FontPlatformData.h
	M	WebCore/platform/graphics/qt/FontPlatformDataQt.cpp
r46428 = 4e8d024866454361b6a4ba28eff659a91cc1517e (git-svn)
No changes between current HEAD and refs/remotes/git-svn
Resetting to the latest refs/remotes/git-svn
http://trac.webkit.org/changeset/46428