Bug 67131 - [Qt] Add support for webfonts
Summary: [Qt] Add support for webfonts
Status: RESOLVED DUPLICATE of bug 78001
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pierre Rossi
URL:
Keywords:
Depends on: 55036 63467
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-29 09:13 PDT by Pierre Rossi
Modified: 2012-02-07 14:17 PST (History)
2 users (show)

See Also:


Attachments
Patch (4.76 KB, patch)
2011-08-29 10:01 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff
Patch (6.16 KB, patch)
2011-10-31 06:31 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Rossi 2011-08-29 09:13:27 PDT
[Qt] Add support for webfonts
Comment 1 Pierre Rossi 2011-08-29 10:01:55 PDT
Created attachment 105497 [details]
Patch
Comment 2 Pierre Rossi 2011-08-29 10:04:48 PDT
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.
Comment 3 Andreas Kling 2011-08-29 10:08:52 PDT
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?
Comment 4 Alexis Menard (darktears) 2011-08-29 15:34:37 PDT
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.
Comment 5 Pierre Rossi 2011-10-31 06:31:39 PDT
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 ?
Comment 6 Simon Hausmann 2011-10-31 06:53:58 PDT
Does it make sense at all to keep the QFontDatabase::addApplicationFonts based "implementation" around?
Comment 7 Jarred Nicholls 2011-10-31 07:00:18 PDT
(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 ?
Comment 8 Simon Hausmann 2011-10-31 07:19:37 PDT
(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.
Comment 9 Pierre Rossi 2011-10-31 07:40:36 PDT
(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.
Comment 10 Simon Hausmann 2012-01-11 07:35:45 PST
Comment on attachment 113045 [details]
Patch

Clearing review on this patch, since it's all being rewritten at the moment :) (looking very promising!)
Comment 11 Jarred Nicholls 2012-01-11 07:40:06 PST
(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
Comment 12 Pierre Rossi 2012-02-07 14:17:07 PST

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