Bug 28015 - [Chromium] @font-face is not supported on Linux
Summary: [Chromium] @font-face is not supported on Linux
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-08-05 01:42 PDT by Yusuke Sato
Modified: 2009-08-06 22:39 PDT (History)
2 users (show)

See Also:

Proposed fix for 28015 (7.92 KB, patch)
2009-08-05 02:20 PDT, Yusuke Sato
no flags Details | Formatted Diff | Diff
Proposed fix for 28015 (v2) (7.88 KB, patch)
2009-08-05 23:45 PDT, Yusuke Sato
levin: review-
Details | Formatted Diff | Diff
patch v3 (7.88 KB, patch)
2009-08-06 20:46 PDT, Yusuke Sato
levin: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Sato 2009-08-05 01:42:10 PDT
(copied from http://crbug.com/18490)

Chrome Version: r21365
URLs (if applicable) : http://www.alistapart.com/d/cssatten/poen.html
Behavior in Chrome for Windows (optional): no problem

CSS3 Web font (aka @font-face, dynamic font, or remote font) is not supported on Chromium Linux, even when --enable-remote-fonts command line flag is used.

What steps will reproduce the problem?

1. start chromium for linux with --enable-remote-fonts
2. visit http://www.alistapart.com/d/cssatten/poen.html

What is the expected result?

The page is rendered using web fonts (scrrenshot:http://www.alistapart.com/d/cssatten/poen.png).

What happens instead?

The page is rendered without using web fonts (screenshot: http://www.alistapart.com/d/cssatten/nowf.png).
Comment 1 Yusuke Sato 2009-08-05 02:20:56 PDT
Created attachment 34125 [details]
Proposed fix for 28015

This is Chromium-only change. Can someone review this?
Comment 2 Jan Alonzo 2009-08-05 05:45:12 PDT
(In reply to comment #1)
> Created an attachment (id=34125) [details]
> Proposed fix for 28015
> This is Chromium-only change. Can someone review this?

Just a nit: you should be using PLATFORM(LINUX) instead of defined(__linux__). Someone from Chromium should comment on the rest of the patch.
Comment 3 Yusuke Sato 2009-08-05 23:45:49 PDT
Created attachment 34203 [details]
Proposed fix for 28015 (v2)

Replaced all defined(__linux__)s with PLATFORM(LINUX).
Comment 4 David Levin 2009-08-06 00:57:34 PDT
Comment on attachment 34203 [details]
Proposed fix for 28015 (v2)

These are only style comments but enough adjustments to r- for now.  

Would you ask someone on the linux team (maybe agl@) to code review this for substance (and put their comments into this bug)?
Then please wait to put your patch up for r? until you get their feedback and address it.

And together with their review, I'll feel comfortable with this.

> Index: WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp
> +    return FontPlatformData(m_fontReference,
> +                            size,
> +                            bold && !m_fontReference->isBold(),
> +                            italic && !m_fontReference->isItalic());

There is no line length restriction in WebKit so feel free to unwrap this but only if you think it will read better.

>  #else
>      notImplemented();
>      return FontPlatformData();
> @@ -186,6 +200,51 @@ static String createUniqueFontName()
>  }
>  #endif
> +class RemoteFontStream : public SkStream {
> +public:
> +    explicit RemoteFontStream(RefPtr<SharedBuffer> buffer)

Use PassRefPtr instead of RefPtr for parameters.

> +    virtual bool rewind()
> +    {
> +      offset_ = 0;
> +      return true;

indent by 4 spaces. (I think check-webkit-style would catch this.)

> +private:
> +    RefPtr<SharedBuffer> buffer_;
> +    size_t offset_;

  Use m_ for member variables.  Not variable_.

> +    RemoteFontStream stream(buffer);
> +    SkTypeface* tf = SkTypeface::CreateFromStream(&stream);

Use full words for variable names (not "tf").

> Index: WebCore/platform/graphics/chromium/FontCustomPlatformData.h
>  #define FontCustomPlatformData_h
>  #include <wtf/Noncopyable.h>
> +#include "FontRenderingMode.h"

This include is out of place (check-webkit-style should catch this.)

> +    explicit FontCustomPlatformData(SkTypeface* tf)
> +        : m_fontReference(tf)

Use full words for variable names (not "tf").
Comment 5 Yusuke Sato 2009-08-06 01:26:52 PDT
Thanks for your comment. I've asked Adam to review this.
Comment 6 Adam Langley 2009-08-06 10:54:02 PDT
Comment on attachment 34203 [details]
Proposed fix for 28015 (v2)

I'm not a WebKit reviewer, so I can't r+, but here are my comments:

> +        This is chromium-only change. Support @font-face on Chromium Linux.

Change to: Chromium Linux: add support for @font-face

> +        https://bugs.webkit.org/show_bug.cgi?id=28015

> +    virtual size_t read(void* buffer, size_t size)

The return type of this function is obviously wrong, but I see that it's wrong in Skia, so not your fault.
Comment 7 David Levin 2009-08-06 13:43:10 PDT
Comment on attachment 34203 [details]
Proposed fix for 28015 (v2)

Need a new patch that addresses the comments by myself and agl.
Comment 8 Yusuke Sato 2009-08-06 20:46:18 PDT
Created attachment 34247 [details]
patch v3

Thanks, Adam.
I've addressed all the comments. Please take another look.
Comment 9 Adam Barth 2009-08-06 22:39:52 PDT
Comment on attachment 34247 [details]
patch v3

Clearing review flag on attachment: 34247

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/loader/CachedFont.cpp
	M	WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp
	M	WebCore/platform/graphics/chromium/FontCustomPlatformData.h
Committed r46885
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp
	M	WebCore/platform/graphics/chromium/FontCustomPlatformData.h
	M	WebCore/loader/CachedFont.cpp
r46885 = 618c2493a3020689c5bfa898bd3f97ab75262c58 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
Comment 10 Adam Barth 2009-08-06 22:39:56 PDT
All reviewed patches have been landed.  Closing bug.