[webkit-reviews] review canceled: [Bug 50619] [GTK] Glyphs in vertical text tests are rotated 90 degrees clockwise : [Attachment 206976] Patch

[bugzilla-daemon at webkit.org]

Denis Nomiyama (dnomi) <d.nomiyama at samsung.com> has canceled Denis Nomiyama
(dnomi) <d.nomiyama at samsung.com>'s request for review:
Bug 50619: [GTK] Glyphs in vertical text tests are rotated 90 degrees clockwise
https://bugs.webkit.org/show_bug.cgi?id=50619

Attachment 206976: Patch
https://bugs.webkit.org/attachment.cgi?id=206976&action=review

------- Additional Comments from Denis Nomiyama (dnomi)
<d.nomiyama at samsung.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=206976&action=review


I found few improvements to this patch, and hopefully it will help to review
it.
They are described below and I will soon upload a new patch with them.
I will also rebase the patch to the latest code.

> Source/WebCore/platform/graphics/FontCache.h:152
> +#if PLATFORM(GTK)
> +    typedef uint32_t FontFileKey;
> +#else
>      typedef AtomicString FontFileKey;
> +#endif

As Martin mentioned before, this change is not convenient since GTK will be the
only port with a different type for FontFileKey.
I will revert this change and will improve the definition of
HashMap<FontFileKey,...> in FontCache.cpp.
The current implementation of this HashMap assumes that FontFileKey is an
integer and it doesn't allow FontFileKey to be an AtomicString (I got a
compilation error).
I think this problem happened after the Chromium/Skia code clean up. The
Chromium port used to define FontFileKey as an integer, which was given by
Skia.

> Source/WebCore/platform/graphics/freetype/FontPlatformData.h:93
> +    FontCache::FontFileKey uniqueID() const { return m_fontID; };

uniqueID()/m_fontID are only used once in FontPlatformDataFreeType.cpp, and
this can be removed/optimised.

> Source/WebCore/platform/graphics/freetype/FontPlatformData.h:132
> +    FontCache::FontFileKey m_fontID;

Ditto.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:45
> +static FontCache::FontFileKey lastFontID = 0;

This is ugly. Instead of generating ID's for FontFileKey, I would like to
propose hash() instead. hash() also gives an unique value based on
m_scaledFont.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:367
> +static uint32_t reverseByteOrder(uint32_t tableTag)

This function is only called once. Perhaps this can be optimised.