[Webkit-unassigned] [Bug 192151] [FreeType] Add initial implementation of variation fonts

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=192151

Adrian Perez <aperez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aperez at igalia.com

--- Comment #15 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 356597
  --> https://bugs.webkit.org/attachment.cgi?id=356597
Patch

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

Without being an expert myself when it comes to FreeType, this
is looking very good!

>> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:46
>> +#include FT_MULTIPLE_MASTERS_H
> 
> Is this a header file? I'm confused.

Yes, this is correct. FreeType is one of the few projects which makes
use of include expansions from macros. Look:

  % grep -r 'define FT_MULTIPLE' /usr/include/freetype2/
  /usr/include/freetype2/freetype/config/ftheader.h:#define FT_MULTIPLE_MASTERS_H  <freetype/ftmm.h>

(:

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:60
>  }

Given that the destructor is empty, this one here could be removed to
let the C++ compiler generate a default destructor automatically. If you
want to be explicit, you can write this in the struct/class definition:

  struct FontPlatformData {
     // ...
     ~FontPlatformData() = default;
  };

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:212
> +    if (m_pattern != other.m_pattern && !FcPatternEqual(m_pattern.get(), other.m_pattern.get()))

This can be a bit puzzling: The comment above says that FcPatternEqual() does NOT
accept NULL pointers, but now that the checks before calling it are removed, there
is no explanation telling why is is known that the m_pattern.get() calls would
always return a non-NULL pointer.

Maybe it would be a good thing to add a note in the comment telling about this,
or even a couple of ASSERTs as you have done in other parts of the code.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181206/13b350a4/attachment.html>