[Chromium] @font-face is not supported on Linux

[Yusuke Sato]

(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).

[Yusuke Sato]

Created attachment 34125 [details]
Proposed fix for 28015

This is Chromium-only change. Can someone review this?

[Jan Alonzo]

(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.

[Yusuke Sato]

Created attachment 34203 [details]
Proposed fix for 28015 (v2)

Thanks!
Replaced all defined(__linux__)s with PLATFORM(LINUX).

[David Levin]

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
>  
> +#if PLATFORM(LINUX)
> +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_.

> +#elif PLATFORM(LINUX)
> +    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.)

> +#elif PLATFORM(LINUX)
> +    explicit FontCustomPlatformData(SkTypeface* tf)
> +        : m_fontReference(tf)

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

[Yusuke Sato]

Thanks for your comment. I've asked Adam to review this.

[Adam Langley]

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.

[David Levin]

Comment on attachment 34203 [details]
Proposed fix for 28015 (v2)

Need a new patch that addresses the comments by myself and agl.

[Yusuke Sato]

Created attachment 34247 [details]
patch v3

Thanks, Adam.
I've addressed all the comments. Please take another look.

[Adam Barth]

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
http://trac.webkit.org/changeset/46885

[Adam Barth]

All reviewed patches have been landed.  Closing bug.