Bug 123310 – Underline bounds cannot be queried before underline itself is drawn

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Underline bounds cannot be queried before underline itself is drawn

[Myles C. Maxfield]

Created attachment 215134 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/15316301>

[EFL EWS Bot]

Comment on attachment 215134 [details]
Patch

Attachment 215134 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/10348238

[kov's GTK+ EWS bot]

Comment on attachment 215134 [details]
Patch

Attachment 215134 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/10348245

[Build Bot]

Comment on attachment 215134 [details]
Patch

Attachment 215134 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/10288251

[Sam Weinig]

Comment on attachment 215134 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:69
> +static FloatRect computeBoundsForUnderline(GraphicsContext &context, const FloatPoint& point, float width, bool printing, bool &shouldAntialias)

The & in "GraphicsContext &context" and "bool &shouldAntialias" are on the wrong sides.

> Source/WebCore/rendering/InlineTextBox.cpp:71
> +    // This function should be side-effect free

This comment doesn't really add anything.

> Source/WebCore/rendering/InlineTextBox.cpp:78
> +    float thickness = max(context.strokeThickness(), 0.5f);

We have started using an explicit std::max.

> Source/WebCore/rendering/InlineTextBox.cpp:84
> +        float adjustedThickness = max(thickness, 1.0f);

Again, std::max please.

> Source/WebCore/rendering/InlineTextBox.cpp:103
> +static void drawLineForText(GraphicsContext &context, const FloatPoint& point, float width, bool printing)

The & in "GraphicsContext &context" is on the wrong sides.

> Source/WebCore/rendering/InlineTextBox.cpp:105
> +    bool shouldAntialias;

This can move down to after the if-statement.

[EFL EWS Bot]

Comment on attachment 215134 [details]
Patch

Attachment 215134 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/11918060

[Myles C. Maxfield]

Comment on attachment 215134 [details]
Patch

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

>> Source/WebCore/rendering/InlineTextBox.cpp:69
>> +static FloatRect computeBoundsForUnderline(GraphicsContext &context, const FloatPoint& point, float width, bool printing, bool &shouldAntialias)
> 
> The & in "GraphicsContext &context" and "bool &shouldAntialias" are on the wrong sides.

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:71
>> +    // This function should be side-effect free
> 
> This comment doesn't really add anything.

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:78
>> +    float thickness = max(context.strokeThickness(), 0.5f);
> 
> We have started using an explicit std::max.

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:84
>> +        float adjustedThickness = max(thickness, 1.0f);
> 
> Again, std::max please.

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:103
>> +static void drawLineForText(GraphicsContext &context, const FloatPoint& point, float width, bool printing)
> 
> The & in "GraphicsContext &context" is on the wrong sides.

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:105
>> +    bool shouldAntialias;
> 
> This can move down to after the if-statement.

Done.

[Myles C. Maxfield]

Created attachment 215189 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 215189 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:69
> +static FloatRect computeBoundsForUnderline(GraphicsContext& context, const FloatPoint& point, float width, bool printing, bool& shouldAntialias)

I think "point" should be called "start" or "origin". "width" should be "length". Is it up or down from the origin?

> Source/WebCore/rendering/InlineTextBox.cpp:73
> +    float thickness = std::max(context.strokeThickness(), 0.5f);

But that bug added a 0.5 thickness only when printing. I think the comment has bitrotted.

> Source/WebCore/rendering/InlineTextBox.cpp:79
> +    if (!printing && !context.paintingDisabled() && context.getCTM(GraphicsContext::DefinitelyIncludeDeviceScale).preservesAxisAlignment()) {

It would be better to early return here.

[EFL EWS Bot]

Comment on attachment 215189 [details]
Patch

Attachment 215189 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/10828022

[Myles C. Maxfield]

Comment on attachment 215189 [details]
Patch

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

>> Source/WebCore/rendering/InlineTextBox.cpp:69
>> +static FloatRect computeBoundsForUnderline(GraphicsContext& context, const FloatPoint& point, float width, bool printing, bool& shouldAntialias)
> 
> I think "point" should be called "start" or "origin". "width" should be "length". Is it up or down from the origin?

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:73
>> +    float thickness = std::max(context.strokeThickness(), 0.5f);
> 
> But that bug added a 0.5 thickness only when printing. I think the comment has bitrotted.

I've traced this back to r14112 (from 2006), entitled "Move WebTextRenderer into WebCore" but can't seem to figure out where WebTextRenderer came from. My guess is that, when printing, the 0.5 comes from that bug, and when not printing, setting the minimum to 0.5 makes sure that when we round to pixel boundaries, the underline disappear (assuming an identity CTM). I think the best thing to do is to just delete the comment. Thoughts?

>> Source/WebCore/rendering/InlineTextBox.cpp:79
>> +    if (!printing && !context.paintingDisabled() && context.getCTM(GraphicsContext::DefinitelyIncludeDeviceScale).preservesAxisAlignment()) {
> 
> It would be better to early return here.

Done.

[Myles C. Maxfield]

**the underline doesn't disappear

(In reply to comment #12)
> (From update of attachment 215189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215189&action=review
> 
> >> Source/WebCore/rendering/InlineTextBox.cpp:69
> >> +static FloatRect computeBoundsForUnderline(GraphicsContext& context, const FloatPoint& point, float width, bool printing, bool& shouldAntialias)
> > 
> > I think "point" should be called "start" or "origin". "width" should be "length". Is it up or down from the origin?
> 
> Done.
> 
> >> Source/WebCore/rendering/InlineTextBox.cpp:73
> >> +    float thickness = std::max(context.strokeThickness(), 0.5f);
> > 
> > But that bug added a 0.5 thickness only when printing. I think the comment has bitrotted.
> 
> I've traced this back to r14112 (from 2006), entitled "Move WebTextRenderer into WebCore" but can't seem to figure out where WebTextRenderer came from. My guess is that, when printing, the 0.5 comes from that bug, and when not printing, setting the minimum to 0.5 makes sure that when we round to pixel boundaries, the underline disappear (assuming an identity CTM). I think the best thing to do is to just delete the comment. Thoughts?
> 
> >> Source/WebCore/rendering/InlineTextBox.cpp:79
> >> +    if (!printing && !context.paintingDisabled() && context.getCTM(GraphicsContext::DefinitelyIncludeDeviceScale).preservesAxisAlignment()) {
> > 
> > It would be better to early return here.
> 
> Done.

[Myles C. Maxfield]

Created attachment 215203 [details]
Patch

[Myles C. Maxfield]

Created attachment 215205 [details]
Patch

[Myles C. Maxfield]

Created attachment 215223 [details]
Patch

[Build Bot]

Comment on attachment 215223 [details]
Patch

Attachment 215223 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/11948047

[Myles C. Maxfield]

The win port is failing to build the Netscape plugin, which is completely irrelevant to this patch. Is there something I can do about this?

[Myles C. Maxfield]

Created attachment 215433 [details]
Patch

[Build Bot]

Comment on attachment 215433 [details]
Patch

Attachment 215433 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/17068179

[Myles C. Maxfield]

Nevermind; I misread it. The Windows build calls drawLineForText in a place I didn't expect.

[Myles C. Maxfield]

Created attachment 215457 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 215457 [details]
Patch

Clearing flags on attachment: 215457

Committed r158243: <http://trac.webkit.org/changeset/158243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This patch moved code that was specific to CG into cross-platform code. Do we know that all the other platforms want this code?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 123520

[Myles C. Maxfield]

Created attachment 215561 [details]
Patch

[Tim Horton]

Comment on attachment 215561 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1261
> +    if (lineRect.height() < initialBounds.height() * 2.0) {

We tend not to use f or .0

[Myles C. Maxfield]

Created attachment 215668 [details]
Patch

[Dean Jackson]

Comment on attachment 215668 [details]
Patch

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

> Source/WebCore/ChangeLog:32
> +        * platform/graphics/GraphicsContext.h: Signature of new computeLineBoundsForText
> +        function
> +        * platform/graphics/blackberry/PathBlackBerry.cpp:
> +        (WebCore::GraphicsContext::computeLineBoundsForText): Implement new function
> +        * platform/graphics/cairo/GraphicsContextCairo.cpp:
> +        (WebCore::GraphicsContext::computeLineBoundsForText): Ditto
> +        (WebCore::GraphicsContext::drawLineForText): Use computeLineBoundsForText
> +        * platform/graphics/cg/GraphicsContextCG.cpp:
> +        (WebCore::computeLineBoundsAndAntialiasingModeForText): Static function that
> +        performs the actual bounds computation
> +        (WebCore::GraphicsContext::computeLineBoundsForText): Calls
> +        computeLineBoundsAndAntialiasingModeForText
> +        (WebCore::GraphicsContext::drawLineForText): Use new function
> +        * platform/graphics/wince/GraphicsContextWinCE.cpp:
> +        (WebCore::GraphicsContext::computeLineBoundsForText): Implement new function

Super nit: end comments with periods.

[WebKit Commit Bot]

Comment on attachment 215668 [details]
Patch

Clearing flags on attachment: 215668

Committed r158392: <http://trac.webkit.org/changeset/158392>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.