Bug 67131 – [Qt] Add support for webfonts

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Pierre Rossi]

[Qt] Add support for webfonts

[Pierre Rossi]

Created attachment 105497 [details]
Patch

[Pierre Rossi]

Comment on attachment 105497 [details]
Patch

Just in case someone has and opinion on the contents mainly. The tests are too much of a mess to figure out until we fix them first for 55036, which this patch depends on anyway.

[Andreas Kling]

Comment on attachment 105497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105497&action=review

> Source/WebCore/platform/graphics/qt/FontPlatformData.h:122
> +        if (m_data->rawFont.isValid())
> +            m_data->font.setPixelSize(qRound(m_data->rawFont.pixelSize()));

This feels very hackish. I realize that we're already in this situation with the current frankenfonts implementation, but how can we get out of it? Who are the clients of FontPlatformData::font(), and can they be switched over to backend-agnostic (QFont vs QRawFont) logic somehow?

[Alexis Menard (darktears)]

Comment on attachment 105497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105497&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Problem here.

> Source/WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:60
> +    const QByteArray ba(buffer->data(), buffer->size());

please use different name than ba.

[Pierre Rossi]

Created attachment 113045 [details]
Patch

Actually, regarding the layout tests, a few of the fonts-related ones I looked into could be unskipped by now. Could be because of 55036, or some other fixes, I guess we should just do that in a separate patch ?

[Simon Hausmann]

Does it make sense at all to keep the QFontDatabase::addApplicationFonts based "implementation" around?

[Jarred Nicholls]

(In reply to comment #6)
> Does it make sense at all to keep the QFontDatabase::addApplicationFonts based "implementation" around?

If trunk only works "correctly" with QRawFont and pre-QRawFont is no longer being supported[*], the old implementation might as well be trashed.

[*] Is trunk still going to be supported for <=4.7.4 ?

[Simon Hausmann]

(In reply to comment #7)
> (In reply to comment #6)
> > Does it make sense at all to keep the QFontDatabase::addApplicationFonts based "implementation" around?
> 
> If trunk only works "correctly" with QRawFont and pre-QRawFont is no longer being supported[*], the old implementation might as well be trashed.

That's exactly what I was thinking, too. I mean, if pre-QRawFont doesn't work properly (_if_ that is), then IMHO we should ditch it.
 
> [*] Is trunk still going to be supported for <=4.7.4 ?

While we support Qt 4.7, we should compiling with all patch releases of 4.7. Once 4.8 is out it would be nice to drop 4.7 support.

[Pierre Rossi]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Does it make sense at all to keep the QFontDatabase::addApplicationFonts based "implementation" around?
> > 
> > If trunk only works "correctly" with QRawFont and pre-QRawFont is no longer being supported[*], the old implementation might as well be trashed.
> 
> That's exactly what I was thinking, too. I mean, if pre-QRawFont doesn't work properly (_if_ that is), then IMHO we should ditch it.

The addApplicationFonts implementation does work AFAICT, but I'm afraid it's not exactly the best approach in our case.

> 
> > [*] Is trunk still going to be supported for <=4.7.4 ?
> 
> While we support Qt 4.7, we should compiling with all patch releases of 4.7. Once 4.8 is out it would be nice to drop 4.7 support.

Agreed, I think 4.7 support was the only reason for the HAVE(RAWFONT) guard, potentially something to drop after 4.8 is out.

[Simon Hausmann]

Comment on attachment 113045 [details]
Patch

Clearing review on this patch, since it's all being rewritten at the moment :) (looking very promising!)

[Jarred Nicholls]

(In reply to comment #10)
> (From update of attachment 113045 [details])
> Clearing review on this patch, since it's all being rewritten at the moment :) (looking very promising!)

Schweeeeeet

[Pierre Rossi]

*** This bug has been marked as a duplicate of bug 78001 ***