Bug 152957 – Get text drawing working with display lists

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Text drawing is stubbed out; need to make it work.

[zalan]

Created attachment 268809 [details]
Patch

[zalan]

Created attachment 268812 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 268812 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:698
> +    fontCascade.drawGlyphs(*this, font, buffer, from, numGlyphs, point, fontCascade.fontDescription().fontSmoothing());

Why is this change? You are making FontCascade::drawGlyphs a static functio , removing the const qualifier and passing a member of a member of FontCascade itself. At the same time all the other functions in FontCascade like drawText() and drawEmphasisMarks() are not static. Should not we be consistent here?

[Build Bot]

Comment on attachment 268812 [details]
Patch

Attachment 268812 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/684022

New failing tests:
fast/picture/image-picture-loads-1x.html

[Build Bot]

Created attachment 268819 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[zalan]

Comment on attachment 268812 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:698
>> +    fontCascade.drawGlyphs(*this, font, buffer, from, numGlyphs, point, fontCascade.fontDescription().fontSmoothing());
> 
> Why is this change? You are making FontCascade::drawGlyphs a static functio , removing the const qualifier and passing a member of a member of FontCascade itself. At the same time all the other functions in FontCascade like drawText() and drawEmphasisMarks() are not static. Should not we be consistent here?

I am not sure what you mean by consistency. drawText() and drawEmphasisMarks() are both higher level drawing functions. They generate a glyphBuffer and call drawGlyphs() with it.

[Said Abou-Hallawa]

Comment on attachment 268812 [details]
Patch

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

>>> Source/WebCore/platform/graphics/GraphicsContext.cpp:698
>>> +    fontCascade.drawGlyphs(*this, font, buffer, from, numGlyphs, point, fontCascade.fontDescription().fontSmoothing());
>> 
>> Why is this change? You are making FontCascade::drawGlyphs a static functio , removing the const qualifier and passing a member of a member of FontCascade itself. At the same time all the other functions in FontCascade like drawText() and drawEmphasisMarks() are not static. Should not we be consistent here?
> 
> I am not sure what you mean by consistency. drawText() and drawEmphasisMarks() are both higher level drawing functions. They generate a glyphBuffer and call drawGlyphs() with it.

I meant the signatures of these functions look similar:

WEBCORE_EXPORT float drawText(GraphicsContext&, const TextRun&, const FloatPoint&, int from = 0, int to = -1, CustomFontNotReadyAction = DoNotPaintIfFontNotReady) const;
void drawGlyphs(GraphicsContext&, const Font&, const GlyphBuffer&, int from, int numGlyphs, const FloatPoint&) const;
void drawEmphasisMarks(GraphicsContext&, const TextRun&, const AtomicString& mark, const FloatPoint&, int from = 0, int to = -1) const;

All of them are non static and const. So why did you pick one of them"drawGlyphs" and make it static? And to overcome the problem of using a member of the class "FontCascade::fontDescription()" you are now passing it every time drawGlyphs() is called.

[zalan]

(In reply to comment #5)
> Created attachment 268819 [details]
> Archive of layout-test-results from ews103 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-ews.
> Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

This failure seems unrelated.

[zalan]

Created attachment 268844 [details]
Patch

[zalan]

probe EWS again.

[Simon Fraser (smfr)]

Comment on attachment 268844 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:-689
> -    if (isRecording()) {
> -        // FIXME: Text drawing with display lists TBD.
> -        return 0;
> -    }
> -

I think this needs a comment to say why it doesn't do anything when recording. I assume font.drawText() calls back into GraphicsContext::drawGlyphs? Would be nice to name these so it's obvious which is higher and lower level.

[zalan]

Committed r194966: <http://trac.webkit.org/changeset/194966>