Underline bounds cannot be queried before underline itself is drawn
Created attachment 215134 [details] Patch
<rdar://problem/15316301>
Comment on attachment 215134 [details] Patch Attachment 215134 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/10348238
Comment on attachment 215134 [details] Patch Attachment 215134 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/10348245
Comment on attachment 215134 [details] Patch Attachment 215134 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/10288251
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.
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
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.
Created attachment 215189 [details] Patch
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.
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
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.
**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.
Created attachment 215203 [details] Patch
Created attachment 215205 [details] Patch
Created attachment 215223 [details] Patch
Comment on attachment 215223 [details] Patch Attachment 215223 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/11948047
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?
Created attachment 215433 [details] Patch
Comment on attachment 215433 [details] Patch Attachment 215433 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/17068179
Nevermind; I misread it. The Windows build calls drawLineForText in a place I didn't expect.
Created attachment 215457 [details] Patch
Comment on attachment 215457 [details] Patch Clearing flags on attachment: 215457 Committed r158243: <http://trac.webkit.org/changeset/158243>
All reviewed patches have been landed. Closing bug.
This patch moved code that was specific to CG into cross-platform code. Do we know that all the other platforms want this code?
Re-opened since this is blocked by bug 123520
Created attachment 215561 [details] Patch
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
Created attachment 215668 [details] Patch
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.
Comment on attachment 215668 [details] Patch Clearing flags on attachment: 215668 Committed r158392: <http://trac.webkit.org/changeset/158392>