[webkit-reviews] review granted: [Bug 76508] [Chromium] Random characters got rendered as empty boxes or with incorrect glyphs even when a font is present : [Attachment 123062] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has granted Kazuhiro Inaba
<kinaba at chromium.org>'s request for review:
Bug 76508: [Chromium] Random characters got rendered as empty boxes or with
incorrect glyphs even when a font is present
https://bugs.webkit.org/show_bug.cgi?id=76508

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123062&action=review


r+. But there are some style issues.

> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:59

> +static bool getGlyphIndices(HFONT font,
> +			       HDC dc,
> +			       const UChar* buffer,
> +			       unsigned length,
> +			       WORD* glyphBuffer,
> +			       DWORD flag) {

You don't need to wrap these lines.

We had better rename 'buffer' to 'characters', 'length'  to 'charactersLength'
because this function has two buffer arguments.

>
Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:148
> +    if (!getGlyphIndices(fontData->platformData().hfont(),
> +			    dc, buffer, length, localGlyphBuffer,
GGI_MARK_NONEXISTING_GLYPHS)) {

You don't need to wrap these lines.

>
Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:150
> +	   SelectObject(dc, oldFont);
> +	   ReleaseDC(0, dc);

We had better introduce scoped objects like base/win/scoped_dc.h of Chromium.
You don't need to add them in this patch.