Bug 188099

Created attachment 345931 [details]
Test patch

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


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

Comment on attachment 345931 [details]
Test patch

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

> Source/WebCore/platform/graphics/FontCascade.cpp:1437
> +    float initialYPosition = point.y();

Does the Windows Glyph concept not track or deal with initial advance? I wonder if it could just always be set to a height of zero so there is no difference in the code here?

Comment on attachment 345931 [details]
Test patch

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

>> Source/WebCore/platform/graphics/FontCascade.cpp:1437
>> +    float initialYPosition = point.y();
> 
> Does the Windows Glyph concept not track or deal with initial advance? I wonder if it could just always be set to a height of zero so there is no difference in the code here?

Windows does deal with initial advances.

Comment on attachment 345931 [details]
Test patch

Attachment 345931 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8677106

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

Created attachment 345961 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Created attachment 345983 [details]
Patch

<rdar://problem/42682699>

Created attachment 345988 [details]
Patch

Created attachment 345995 [details]
Patch

Created attachment 346007 [details]
Patch

Comment on attachment 346007 [details]
Patch

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

> Source/WebCore/platform/graphics/ComplexTextController.cpp:49
> +    WTF_MAKE_FAST_ALLOCATED;

Seems like this is unneeded, since we won‘t ever allocate one.

> Source/WebCore/platform/graphics/FontCascade.cpp:1436
> +    FloatPoint startPoint(point.x() + glyphBuffer.initialAdvance().width(), point.y() + glyphBuffer.initialAdvance().height());

This can be written like this:

    FloatPoint startPoint { point + glyphBuffer.initialAdvance() };

Or any variant using { }, (), or =, and either FloatPoint or auto. But no reason to do the x and y separately.

Comment on attachment 346007 [details]
Patch

Attachment 346007 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8690617

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

Created attachment 346027 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

*** Bug 188168 has been marked as a duplicate of this bug. ***

Created attachment 346184 [details]
Patch

Comment on attachment 346007 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCascade.cpp:1436
>> +    FloatPoint startPoint(point.x() + glyphBuffer.initialAdvance().width(), point.y() + glyphBuffer.initialAdvance().height());
> 
> This can be written like this:
> 
>     FloatPoint startPoint { point + glyphBuffer.initialAdvance() };
> 
> Or any variant using { }, (), or =, and either FloatPoint or auto. But no reason to do the x and y separately.

Looks like Windows can't handle that :(

Committed r234448: <https://trac.webkit.org/changeset/234448>

Comment on attachment 346007 [details]
Patch

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

>>> Source/WebCore/platform/graphics/FontCascade.cpp:1436
>>> +    FloatPoint startPoint(point.x() + glyphBuffer.initialAdvance().width(), point.y() + glyphBuffer.initialAdvance().height());
>> 
>> This can be written like this:
>> 
>>     FloatPoint startPoint { point + glyphBuffer.initialAdvance() };
>> 
>> Or any variant using { }, (), or =, and either FloatPoint or auto. But no reason to do the x and y separately.
> 
> Looks like Windows can't handle that :(

I’d like to know more. Can you show me which version you tried and how Windows failed?

(In reply to Darin Adler from comment #19)
> Comment on attachment 346007 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346007&action=review
> 
> >>> Source/WebCore/platform/graphics/FontCascade.cpp:1436
> >>> +    FloatPoint startPoint(point.x() + glyphBuffer.initialAdvance().width(), point.y() + glyphBuffer.initialAdvance().height());
> >> 
> >> This can be written like this:
> >> 
> >>     FloatPoint startPoint { point + glyphBuffer.initialAdvance() };
> >> 
> >> Or any variant using { }, (), or =, and either FloatPoint or auto. But no reason to do the x and y separately.
> > 
> > Looks like Windows can't handle that :(
> 
> I’d like to know more. Can you show me which version you tried and how
> Windows failed?

FloatPoint startPoint { point + glyphBuffer.initialAdvance() };

https://webkit-queues.webkit.org/patch/346184/win-ews

c:\cygwin\home\buildbot\webkit\source\webcore\platform\graphics\fontcascade.cpp(1436): error C2678: binary '+': no operator found which takes a left-hand operand of type 'WebCore::FloatPoint' (or there is no acceptable conversion) (compiling source file C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource330.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

Yes, I see what the issue is. I thought that initialAdvance() returned a FloatSize. But even though this is Windows, we have USE(CG) true, so GlyphBufferAdvance is not a FloatSize, it's a struct derived from CGSize instead. The comment says:

// CG uses CGSize instead of FloatSize so that the result of advances()
// can be passed directly to CGContextShowGlyphsWithAdvances in FontMac.mm

That comment points to some things that are not quite right. For one thing, there is no call to CGContextShowGlyphsWithAdvances any more so the comment is not correct. I am not at all sure that using CGSize instead of FloatSize here is a good idea. And I’m even more unsure that using a struct derived from CGSize with added functions is a good style choice.

So I think this is something worth cleaning up eventually.

And of course FontMac.mm is not used on Windows.

I see the code taking advantage of this now.

It’s Font::applyTransforms, and it’s inside #if PLATFORM(COCOA), not #if USE(CG).