Bug 174813 - [WinCairo] Implement Font::platformBoundsForGlyph
Summary: [WinCairo] Implement Font::platformBoundsForGlyph
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-25 00:51 PDT by Fujii Hironori
Modified: 2017-08-01 18:09 PDT (History)
7 users (show)

See Also:


Attachments
Wrongly placed text-emphasis (fast/text/emphasis.html) (30.94 KB, image/png)
2017-07-26 19:21 PDT, Fujii Hironori
no flags Details
Patch (1.97 KB, patch)
2017-07-26 20:54 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (2.04 KB, patch)
2017-07-30 17:57 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WinCairo port's result of fast/text/emphasis.html after this change (30.86 KB, image/png)
2017-07-31 22:49 PDT, Fujii Hironori
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2017-07-25 00:51:13 PDT
(Forked from Bug 82798)

Font::platformBoundsForGlyph in Source/WebCore/platform/graphics/win/SimpleFontDataCairoWin.cpp is not implemented yet.

> FloatRect Font::platformBoundsForGlyph(Glyph glyph) const
> {
>     if (m_platformData.useGDI())
>         return boundsForGDIGlyph(glyph);
>     //FIXME: Implement this
>     return FloatRect();
> }
Comment 1 Fujii Hironori 2017-07-26 19:21:35 PDT
Created attachment 316505 [details]
Wrongly placed text-emphasis (fast/text/emphasis.html)

text-emphasis should be placed at horizontally center of the character.
Comment 2 Fujii Hironori 2017-07-26 20:54:32 PDT
Created attachment 316514 [details]
Patch
Comment 3 Alex Christensen 2017-07-28 08:30:00 PDT
Comment on attachment 316514 [details]
Patch

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

> Source/WebCore/platform/graphics/win/SimpleFontDataCairoWin.cpp:114
> +    SaveDC(dc);

Would it be more efficient to use GetCurrentObject and GetGraphicsMode instead of saving the entire dc?
Comment 4 Fujii Hironori 2017-07-30 17:01:03 PDT
Thank you for the review.

I'm reading the spec of cairo_win32_scaled_font_select_font and the source code.
https://www.cairographics.org/manual/cairo-Win32-Fonts.html#cairo-win32-scaled-font-select-font

cairo_win32_scaled_font_select_font changes not only current font, but also the graphics mode, the world transform and the map mode.
And, the spec recommends to use SaveDC() and RestoreDC(). I think it's safer to use SaveDC() and RestoreDC().

I realized I should call cairo_win32_scaled_font_done_font. (even though, this function does nothing actually)
I'll revise the patch.
Comment 5 Fujii Hironori 2017-07-30 17:57:55 PDT
Created attachment 316753 [details]
Patch
Comment 6 Alex Christensen 2017-07-31 14:21:48 PDT
Comment on attachment 316753 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: fast/text/emphasis.html

This usually indicates that you added a new test.  Do you mean this test failed with WinCairo before and not after?
Comment 7 Fujii Hironori 2017-07-31 22:49:30 PDT
Created attachment 316834 [details]
WinCairo port's result of fast/text/emphasis.html after this change

Still fails after this change.
Mostly because the expectation are created for AppleWin port.
I think it's too early for WinCairo port to add expectations because WinCairo BuildBot is not ready for layout tests.

OK, I'll create a test for this change.
Comment 8 Alex Christensen 2017-08-01 17:39:20 PDT
Comment on attachment 316753 [details]
Patch

Let's just land this.
Comment 9 WebKit Commit Bot 2017-08-01 18:08:52 PDT
Comment on attachment 316753 [details]
Patch

Clearing flags on attachment: 316753

Committed r220117: <http://trac.webkit.org/changeset/220117>
Comment 10 WebKit Commit Bot 2017-08-01 18:08:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-08-01 18:09:35 PDT
<rdar://problem/33667541>