Bug 162712 – [Win][Direct2D] Add D2D Font handling code

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

This patch adds Direct2D/DirectWrite implementations of the Font routines currently handled by Cairo or CoreGraphics on Windows. These exist in parallel to the existing libraries.

This patch simply adds the new files. It does not incorporate them into the build.

[Brent Fulgham]

Created attachment 290168 [details]
Patch

[WebKit Commit Bot]

Attachment 290168 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/win/FontDirect2D.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

(In reply to comment #2)
> Attachment 290168 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/graphics/win/FontDirect2D.cpp:34:  Found
> header this file implements after other header. Should be: config.h, primary
> header, blank line, and then alphabetically sorted.  [build/include_order]
> [4]
> Total errors found: 1 in 11 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

check-webkit-style is confused because 'FontDirect2D.cpp' should probably be called 'FontCascadeDirect2D.cpp'

[Brent Fulgham]

Created attachment 290172 [details]
Patch

[Alex Christensen]

Comment on attachment 290172 [details]
Patch

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

These new files aren't added to PlatformWin.cmake.  They should probably be added so we can at least see if they compile, unless they're so incomplete that they don't link.

> Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp:47
> +const int syntheticObliqueAngle = 14;

14 degrees?  Where did this come from?

> Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp:56
> +    case Antialiased: {

Scope doesn't look needed here.

> Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp:84
> +    D2D1_MATRIX_3X2_F matrix;
> +    context->GetTransform(&matrix);

Can this fail, leaving matrix uninitialized?  The other context functions look like they might be able to fail, too.

> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:56
> +    , m_orientation(Horizontal)

This should use c++ style initialization in the header
m_ orientation { Horizontal }

> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:61
> +    , m_isColorBitmapFont(false)

ditto

> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:55
> +    UChar localeName[256] = { };
> +    int localeLength = GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), 255);

LOCALE_NAME_MAX_LENGTH

> Source/WebCore/platform/graphics/win/SimpleFontDataDirect2D.cpp:182
> +    RELEASE_ASSERT(font);

fontFace?

[Brent Fulgham]

Comment on attachment 290172 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp:47
>> +const int syntheticObliqueAngle = 14;
> 
> 14 degrees?  Where did this come from?

It's the value from FontCGWin.cpp. I'll combine them.

>> Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp:56
>> +    case Antialiased: {
> 
> Scope doesn't look needed here.

OK.

>> Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp:84
>> +    context->GetTransform(&matrix);
> 
> Can this fail, leaving matrix uninitialized?  The other context functions look like they might be able to fail, too.

GetTransform/SetTransform don't return error state, so I don't think so.

Likewise the D2D1::Matrix3x2F::Skew(..) (and related) functions just return matrix values. There is no error state, since they are probably just performing simple calculations.

>> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:56
>> +    , m_orientation(Horizontal)
> 
> This should use c++ style initialization in the header
> m_ orientation { Horizontal }

Will fix.

>> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:61
>> +    , m_isColorBitmapFont(false)
> 
> ditto

ditto.

>> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:55
>> +    int localeLength = GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), 255);
> 
> LOCALE_NAME_MAX_LENGTH

Whoops!

>> Source/WebCore/platform/graphics/win/SimpleFontDataDirect2D.cpp:182
>> +    RELEASE_ASSERT(font);
> 
> fontFace?

Whoops! Thanks.

[Brent Fulgham]

Created attachment 290180 [details]
Patch

[Alex Christensen]

Comment on attachment 290180 [details]
Patch

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

> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:7
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; eithe
> + * version 2 of the License, or (at your option) any later version.

This looks like the wrong license.

> Source/WebCore/platform/graphics/win/SimpleFontDataDirect2D.cpp:15
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission. 

I think the newest license has 2 clauses here.

> Source/WebCore/platform/graphics/win/SimpleFontDataDirect2D.cpp:226
> +    RELEASE_ASSERT(SUCCEEDED(hr));

There is heavy use of RELEASE_ASSERT in this patch.  Do we really want to crash in all these cases?

> Source/WebCore/platform/graphics/win/TextAnalyzerHelper.cpp:118
> +    return S_OK;

Do we really want to return S_OK for all these unimplemented functions?

> Source/WebCore/platform/graphics/win/TextAnalyzerHelper.h:62
> +    DWRITE_SCRIPT_ANALYSIS m_analysis;

Should this also have an initializer?

[Brent Fulgham]

(In reply to comment #5)
> Comment on attachment 290172 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290172&action=review
> 
> These new files aren't added to PlatformWin.cmake.  They should probably be
> added so we can at least see if they compile, unless they're so incomplete
> that they don't link.

Let's talk tomorrow. These won't compile without new GraphicsContext code, so not much point building this yet.

I have a follow-up patch where we can get things building, but it's going to be an either/or build with the Windows CoreGraphics or WinCairo builds.

[Brent Fulgham]

Comment on attachment 290180 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:7
>> + * version 2 of the License, or (at your option) any later version.
> 
> This looks like the wrong license.

Ok. I'll revise that.

>> Source/WebCore/platform/graphics/win/SimpleFontDataDirect2D.cpp:226
>> +    RELEASE_ASSERT(SUCCEEDED(hr));
> 
> There is heavy use of RELEASE_ASSERT in this patch.  Do we really want to crash in all these cases?

I think it's warranted for now. This code isn't being built or used by anyone but me, and I would like to know immediately if one of these fails. We can back off on this once we get tests running.

>> Source/WebCore/platform/graphics/win/TextAnalyzerHelper.cpp:118
>> +    return S_OK;
> 
> Do we really want to return S_OK for all these unimplemented functions?

They aren't even called in the analyze scripts case I'm using. I'll switch to E_UNIMPL or whatever that flag is.

>> Source/WebCore/platform/graphics/win/TextAnalyzerHelper.h:62
>> +    DWRITE_SCRIPT_ANALYSIS m_analysis;
> 
> Should this also have an initializer?

I'll add one.

[Brent Fulgham]

Created attachment 290205 [details]
Patch

[Alex Christensen]

Comment on attachment 290205 [details]
Patch

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

r=me once these comments are addressed.

> Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp:97
> +    for (unsigned i = 0; i < numGlyphs; ++i) {
> +        auto advance = glyphBuffer.advances(i + from);
> +        if (!advance)
> +            continue;

This seems like it could go out of bounds if we didn't properly check numGlyphs.  It's definitely outside the scope of this patch, but using iterators would be safer.

> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:44
>  using namespace std;

Could we get rid of this, too?

> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:37
> +using namespace std::min;

This isn't used.  And std::min isn't a namespace.

> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:55
> +        m_isSystemFont = !wcscmp(faceName, L"Lucida Grande");

I see this is done a few other places, but do we really want to hard code in the name of the system font?

> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:2
> + * Copyright (C) 2006 Apple Inc.  All rights reserved.

2016 here and in many other headers.

> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:54
> +    localeName[localeLength] = 0;

I prefer '\0' to indicate the type.

> Source/WebCore/platform/graphics/win/SimpleFontDataDirect2D.cpp:50
> +using namespace std;

can we not?

> Source/WebCore/platform/graphics/win/TextAnalyzerHelper.cpp:40
> +    m_analysis = { };

Why isn't this with the others?

[Brent Fulgham]

Comment on attachment 290205 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp:97
>> +            continue;
> 
> This seems like it could go out of bounds if we didn't properly check numGlyphs.  It's definitely outside the scope of this patch, but using iterators would be safer.

I'll add a check that 'i+from' is not greater than the size of the advances member.

This is probably needed in FontCG as well, but I'll do that as a separate patch.

>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:44
>>  using namespace std;
> 
> Could we get rid of this, too?

Done.

>> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:37
>> +using namespace std::min;
> 
> This isn't used.  And std::min isn't a namespace.

Done.

>> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:55
>> +        m_isSystemFont = !wcscmp(faceName, L"Lucida Grande");
> 
> I see this is done a few other places, but do we really want to hard code in the name of the system font?

Especially when system font on Vista and newer (I think) is "Segoe UI". This should probably be done with some kind of system check, and probably in FontPlatformDataWin.cpp and shared across the Windows ports.

I'll do a separate bug for that.

>> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:2
>> + * Copyright (C) 2006 Apple Inc.  All rights reserved.
> 
> 2016 here and in many other headers.

Fixed.

>> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:54
>> +    localeName[localeLength] = 0;
> 
> I prefer '\0' to indicate the type.

OK!

>> Source/WebCore/platform/graphics/win/SimpleFontDataDirect2D.cpp:50
>> +using namespace std;
> 
> can we not?

Sure!

>> Source/WebCore/platform/graphics/win/TextAnalyzerHelper.cpp:40
>> +    m_analysis = { };
> 
> Why isn't this with the others?

Fixed!

[Brent Fulgham]

Created attachment 290216 [details]
Patch for landing

[Brent Fulgham]

Comment on attachment 290216 [details]
Patch for landing

Re-adding r+ from Alex's review. New patch for landing to make sure the bots are happy before committing.

[WebKit Commit Bot]

Comment on attachment 290216 [details]
Patch for landing

Clearing flags on attachment: 290216

Committed r206597: <http://trac.webkit.org/changeset/206597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.