[webkit-reviews] review denied: [Bug 28015] [Chromium] @font-face is not supported on Linux : [Attachment 34203] Proposed fix for 28015 (v2)

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Yusuke Sato
<yusukes at chromium.org>'s request for review:
Bug 28015: [Chromium] @font-face is not supported on Linux
https://bugs.webkit.org/show_bug.cgi?id=28015

Attachment 34203: Proposed fix for 28015 (v2)
https://bugs.webkit.org/attachment.cgi?id=34203&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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").