Bug 97696 – [Chromium][Win] Remove ensureFontLoaded from PlatformSupport

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Mark Pilgrim (Google)]

[Chromium][Win] Remove ensureFontLoaded from PlatformSupport

[Mark Pilgrim (Google)]

Created attachment 165828 [details]
Patch

[Adam Barth]

Comment on attachment 165828 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp:44
>  

No need for this blank line.

[WebKit Review Bot]

Comment on attachment 165828 [details]
Patch

Clearing flags on attachment: 165828

Committed r129673: <http://trac.webkit.org/changeset/129673>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 97723

[Mark Pilgrim (Google)]

Created attachment 175004 [details]
Patch

[James Robinson]

Comment on attachment 175004 [details]
Patch

What was the build breakage last time and how does this avoid it?

[Mark Pilgrim (Google)]

The previous patch missed all the calling references in WebCore/platform/chromium/. (Still not sure why that passed EWS, but anyway... this patch includes them.)

[Adam Barth]

Comment on attachment 175004 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/SimpleFontDataChromiumWin.cpp:-64
> -        if (PlatformSupport::ensureFontLoaded(m_platformData.hfont())) {

I wonder if we should provide this helper function in WebCore somewhere rather than inlining it everywhere.

[James Robinson]

(In reply to comment #8)
> The previous patch missed all the calling references in WebCore/platform/chromium/. (Still not sure why that passed EWS, but anyway... this patch includes them.)

We don't have any EWS coverage for the chromium-win build.

[Mark Pilgrim (Google)]

Is this ready for an r+ or r-?

[Adam Barth]

Comment on attachment 175004 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/SimpleFontDataChromiumWin.cpp:-64
>> -        if (PlatformSupport::ensureFontLoaded(m_platformData.hfont())) {
> 
> I wonder if we should provide this helper function in WebCore somewhere rather than inlining it everywhere.

IMHO, we should add this helper function.  Otherwise, we're making all these call sites much more verbose.

[Mark Pilgrim (Google)]

Created attachment 176516 [details]
WIP Patch

[Mark Pilgrim (Google)]

Created attachment 176564 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 176564 [details]
Patch

Fixed a typo. Ready for review now.

[Adam Barth]

Comment on attachment 176564 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:178
> -                if (PlatformSupport::ensureFontLoaded(hfont())) {
> +                if (FontPlatformData::ensureFontLoaded(hfont())) {

No need for the FontPlatformData:: prefix here.  We're already inside FontPlatformData.

[WebKit Review Bot]

Comment on attachment 176564 [details]
Patch

Clearing flags on attachment: 176564

Committed r136057: <http://trac.webkit.org/changeset/136057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.