Summary: | [css3-text] Add platform support for "wavy" text decoration style | ||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bruno Abinader (history only) <bruno.abinader> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, abecsi, benjamin, buildbot, cmarcelo, dbates, dglazkov, eric, esprehn+autocc, gtk-ews, gyuyoung.kim, lamarque, macpherson, menard, ojan.autocc, peter+ews, rakuco, rego+ews, rniwa, syoichi, tmpsantos, vestbo, webkit-ews, webkit.review.bot, xan.lopez, zan | ||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
URL: | http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-style-property | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 90958, 93507, 93863, 108571, 109926 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 58491, 100546, 93509, 94110, 94111, 94112, 94114 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Bruno Abinader (history only)
2012-08-01 06:08:02 PDT
Two new platform specific bugs have been created so far: [Qt] Add support for text decoration "wavy" style https://bugs.webkit.org/show_bug.cgi?id=93507 [Skia] Add support for text decoration "wavy" and "double" styles https://bugs.webkit.org/show_bug.cgi?id=93509 Four new platform specific bugs have been added: [cairo] Add support for text decoration "wavy" style https://bugs.webkit.org/show_bug.cgi?id=94110 [wx] Add support for text decoration "wavy" style https://bugs.webkit.org/show_bug.cgi?id=94111 [cg] Add support for text decoration "wavy" style https://bugs.webkit.org/show_bug.cgi?id=94112 [wince] Add support for text decoration "wavy" style https://bugs.webkit.org/show_bug.cgi?id=94114 Created attachment 186394 [details]
Patch
Proposed patch. It works for all platforms.
Created attachment 186397 [details]
Patch (EWS only version)
Same patch with css3-text and css3-conditional-rules enabled.
Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16377169 Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16373256 Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16377173 Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16376214 Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16365378 Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16370291 Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16376238 Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16378204 Comment on attachment 186397 [details] Patch (EWS only version) Attachment 186397 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16368329 Created attachment 186694 [details]
Patch (EWS only version)
Attempt to fix build with css3-conditional-rules enabled.
Comment on attachment 186694 [details] Patch (EWS only version) Attachment 186694 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16379762 Comment on attachment 186694 [details] Patch (EWS only version) Attachment 186694 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16370892 Comment on attachment 186694 [details] Patch (EWS only version) Attachment 186694 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16373822 Comment on attachment 186694 [details] Patch (EWS only version) Attachment 186694 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16372934 Created attachment 186707 [details]
Patch (EWS only version)
Attempt to fix build with css3-conditional-rules enabled.
Comment on attachment 186694 [details] Patch (EWS only version) Attachment 186694 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16368866 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16374892 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16386009 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16367968 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16369967 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16386020 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16373873 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16368895 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16387019 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16388019 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16376952 Comment on attachment 186707 [details] Patch (EWS only version) Attachment 186707 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16387078 Comment on attachment 186394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186394&action=review It looks promising but this needs to become much cleaner. > Source/WebCore/ChangeLog:10 > + platforms. This patch also obsoletes bugs 94110, 94111, 94112, 94114 > + and 108571. This "This patch also obsoletes bugs 94110, 94111, 94112, 94114 and 108571." is unnecessary. What you should do instead is explain the change. Both the "Why?" and "What/How?" of the patch. > Source/WebCore/ChangeLog:14 > + Tests are in > + fast/css3-text/css3-text-decoration/text-decoration-style.html already, > + just need to rebaseline them (see bug 100546). Then when not already rebaseline them for your platform in this patch? > Source/WebCore/rendering/InlineTextBox.cpp:961 > +void InlineTextBox::createWavyPath(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2, const float strokeThickness, Path& path) This is either misnamed or the function does not do enough. From a function named createWavyPath(), the only return you would expect is a Path. Here you modify the path passed as a parameter, and modify the context. As an alternative, why not 'strokeWavyTextDecoration(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2)'? +don't const the input float; > Source/WebCore/rendering/InlineTextBox.cpp:965 > + bool isVerticalLine = (p1.x() == p2.x()); You should compute this after adjustLineToPixelBoundaries(). This cold could be moved just before the if(). > Source/WebCore/rendering/InlineTextBox.cpp:971 > + short signal = -1; Why a short? What is this anyway? "Signal" is a strange name in this context. > Source/WebCore/rendering/InlineTextBox.cpp:994 > + x1 = x2 = p1.x(); > + > + // Make sure (x1, y1) < (x2, y2) > + if (p1.y() < p2.y()) { > + y1 = p1.y(); > + y2 = p2.y(); > + } else { > + y1 = p2.y(); > + y2 = p1.y(); > + } > + > + path.moveTo(FloatPoint(x1 + signal * step, y1)); > + float y = y1 + 2 * step; > + > + while (y <= y2) { > + signal = -signal; > + path.addLineTo(FloatPoint(x1 + signal * step, y)); > + path.addLineTo(FloatPoint(x1 + signal * step, y + flat)); // Draw flat line between diagonal lines. > + y += 2 * step + flat; > + } Instinctively, I would have used bezier curve. Why do you use straight lines? > Source/WebCore/rendering/InlineTextBox.cpp:1115 > +#if ENABLE(CSS3_TEXT) > + if (decorationStyle == TextDecorationStyleWavy) { > + Path path; > + createWavyPath(context, FloatPoint(localOrigin.x(), localOrigin.y() + baseline + doubleOffset + 1), FloatPoint(localOrigin.x() + width, localOrigin.y() + baseline + doubleOffset + 1), textDecorationThickness, path); > + context->strokePath(path); > + } else { > +#endif // CSS3_TEXT > // Leave one pixel of white between the baseline and the underline. > context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1), width, isPrinting); > #if ENABLE(CSS3_TEXT) > - if (decorationStyle == TextDecorationStyleDouble) > - context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1 + doubleOffset), width, isPrinting); > + if (decorationStyle == TextDecorationStyleDouble) > + context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1 + doubleOffset), width, isPrinting); > + } > #endif // CSS3_TEXT Shouldn't this become a switch-case based on decorationStyle? > Source/WebCore/rendering/InlineTextBox.cpp:1130 > + if (decorationStyle == TextDecorationStyleWavy) { > + Path path; > + createWavyPath(context, FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset), FloatPoint(localOrigin.x() + width, localOrigin.y() - doubleOffset), textDecorationThickness, path); > + context->strokePath(path); > + } else { > +#endif // CSS3_TEXT > context->drawLineForText(localOrigin, width, isPrinting); > #if ENABLE(CSS3_TEXT) > - if (decorationStyle == TextDecorationStyleDouble) > - context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset), width, isPrinting); > + if (decorationStyle == TextDecorationStyleDouble) > + context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset), width, isPrinting); > + } Ditto. (In reply to comment #32) > (From update of attachment 186394 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186394&action=review > > Source/WebCore/ChangeLog:14 > > + Tests are in > > + fast/css3-text/css3-text-decoration/text-decoration-style.html already, > > + just need to rebaseline them (see bug 100546). > > Then when not already rebaseline them for your platform in this patch? Ok, I will do it. > As an alternative, why not 'strokeWavyTextDecoration(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2)'? > > +don't const the input float; Why not const them? > > Source/WebCore/rendering/InlineTextBox.cpp:971 > > + short signal = -1; > > Why a short? > What is this anyway? "Signal" is a strange name in this context. "sign" is the correct name here. I will fix it. > > Source/WebCore/rendering/InlineTextBox.cpp:994 > > + x1 = x2 = p1.x(); > > + > > + // Make sure (x1, y1) < (x2, y2) > > + if (p1.y() < p2.y()) { > > + y1 = p1.y(); > > + y2 = p2.y(); > > + } else { > > + y1 = p2.y(); > > + y2 = p1.y(); > > + } > > + > > + path.moveTo(FloatPoint(x1 + signal * step, y1)); > > + float y = y1 + 2 * step; > > + > > + while (y <= y2) { > > + signal = -signal; > > + path.addLineTo(FloatPoint(x1 + signal * step, y)); > > + path.addLineTo(FloatPoint(x1 + signal * step, y + flat)); // Draw flat line between diagonal lines. > > + y += 2 * step + flat; > > + } > > Instinctively, I would have used bezier curve. Why do you use straight lines? Well, I am not used to use bezier curves. Also, we deciced to make this curse to look similar to mozilla's implementation [1]. There is also some performance concerns about using strokPath to implement wavy stroke [2], would bezier curve improve performance here? [1] https://bugs.webkit.org/show_bug.cgi?id=94110#c16 [2] https://bugs.webkit.org/show_bug.cgi?id=93507#c43 > > Source/WebCore/rendering/InlineTextBox.cpp:1115 > > +#if ENABLE(CSS3_TEXT) > > + if (decorationStyle == TextDecorationStyleWavy) { > > + Path path; > > + createWavyPath(context, FloatPoint(localOrigin.x(), localOrigin.y() + baseline + doubleOffset + 1), FloatPoint(localOrigin.x() + width, localOrigin.y() + baseline + doubleOffset + 1), textDecorationThickness, path); > > + context->strokePath(path); > > + } else { > > +#endif // CSS3_TEXT > > // Leave one pixel of white between the baseline and the underline. > > context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1), width, isPrinting); > > #if ENABLE(CSS3_TEXT) > > - if (decorationStyle == TextDecorationStyleDouble) > > - context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1 + doubleOffset), width, isPrinting); > > + if (decorationStyle == TextDecorationStyleDouble) > > + context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1 + doubleOffset), width, isPrinting); > > + } > > #endif // CSS3_TEXT > > Shouldn't this become a switch-case based on decorationStyle? I can use change it to a switch-case, but it would look like this switch (decorationStyle) { case TextDecorationStyleWavy: ... break; default: ... break; } That more verbose than if (decorationStyle == TextDecorationStyleWavy) { ... } else { ... } (In reply to comment #33) > (In reply to comment #32) > > (From update of attachment 186394 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=186394&action=review > > > Source/WebCore/ChangeLog:14 > > > + Tests are in > > > + fast/css3-text/css3-text-decoration/text-decoration-style.html already, > > > + just need to rebaseline them (see bug 100546). > > > > Then when not already rebaseline them for your platform in this patch? > > Ok, I will do it. I have noticed that my other patch for https://bugs.webkit.org/show_bug.cgi?id=93507 already includes the baseline for my platform. We will have to revert that other patch to include the baseline in this patch. > > As an alternative, why not 'strokeWavyTextDecoration(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2)'? > > > > +don't const the input float; > > Why not const them? By the way, if I do not const them then I cannot write strokeWavyTextDecoration(context, FloatPoint(localOrigin.x(), localOrigin.y() + baseline + doubleOffset + 1), FloatPoint(localOrigin.x() + width, localOrigin.y() + baseline + doubleOffset + 1), textDecorationThickness); in InlineTextBox::paintDecoration(). I would have to create a temporary variable for each FloatPoint parameter before passing them to strokeWavyTextDecoration(). Created attachment 187833 [details]
Patch
Fixes some issues in code style and update changelogs.
Created attachment 187834 [details]
Patch (EWS only version)
Same patch with css3-text and css3-conditional-rules flags enabled.
Comment on attachment 187834 [details] Patch (EWS only version) Attachment 187834 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16488942 Comment on attachment 187834 [details] Patch (EWS only version) Attachment 187834 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16522180 Comment on attachment 187834 [details] Patch (EWS only version) Attachment 187834 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16520166 Comment on attachment 187834 [details] Patch (EWS only version) Attachment 187834 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16488945 Comment on attachment 187834 [details] Patch (EWS only version) Attachment 187834 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16519233 Comment on attachment 187834 [details] Patch (EWS only version) Attachment 187834 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16439252 Comment on attachment 187834 [details] Patch (EWS only version) Attachment 187834 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16440233 Created attachment 187866 [details]
Patch (EWS only version)
Drop ENABLE_CSS_HIERARCHIES flag.
Comment on attachment 187866 [details] Patch (EWS only version) Attachment 187866 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16514475 Created attachment 188309 [details]
Patch (EWS only version)
Save/restore graphics context.
Comment on attachment 188309 [details] Patch (EWS only version) Attachment 188309 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16542545 Comment on attachment 188309 [details] Patch (EWS only version) Attachment 188309 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16537584 Comment on attachment 188309 [details] Patch (EWS only version) Attachment 188309 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16527657 Comment on attachment 188309 [details] Patch (EWS only version) Attachment 188309 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16530644 Comment on attachment 188309 [details] Patch (EWS only version) Attachment 188309 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16586556 Created attachment 190078 [details]
Patch (EWS only version)
Fix issue with cr-linux bot.
Comment on attachment 190078 [details] Patch (EWS only version) Attachment 190078 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/16769201 > > As an alternative, why not 'strokeWavyTextDecoration(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2)'? > > > > +don't const the input float; > > Why not const them? This: "const float strokeThickness)" is not the WebKit convention. > > Instinctively, I would have used bezier curve. Why do you use straight lines? > > Well, I am not used to use bezier curves. Also, we deciced to make this curse to look similar to mozilla's implementation [1]. There is also some performance concerns about using strokPath to implement wavy stroke [2], would bezier curve improve performance here? > > [1] https://bugs.webkit.org/show_bug.cgi?id=94110#c16 > [2] https://bugs.webkit.org/show_bug.cgi?id=93507#c43 I don't really care about Mozilla's implementation if we can make ours looks better! :) I doubt a bezier path would improve performance. On a good rasterizer, that will not change performance too much. It is probably worth testing and comparing the two. > I can use change it to a switch-case, but it would look like this > > [...] Verbose is good. Take the one with the best readability. Created attachment 192492 [details]
Patch
Fix issues reported.
Created attachment 192493 [details]
Bezier with 1 pixel stroke thickness
Expectations using wavy bezier curve and 1 pixel stroke thickness
Created attachment 192494 [details]
Bezier with 2 pixels stroke thickness
Expectations using wavy bezier curve and 2 pixels stroke thickness
Attachment 192492 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-decoration-style-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/InlineTextBox.h']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-decoration-style-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 192495 [details]
Non-bezier with 1 pixel stroke thickness
Expectations using wavy curve and 1 pixel stroke thickness
Created attachment 192497 [details]
Non-bezier with 2 pixels stroke thickness
Expectations using wavy curve and 2 pixels stroke thickness
Created attachment 192500 [details] Bezier implementation with instrumentation code. (In reply to comment #54) > I don't really care about Mozilla's implementation if we can make ours looks better! :) For comparison you can see some screenshots of the Mozilla implementation in https://bugs.webkit.org/show_bug.cgi?id=94110 > I doubt a bezier path would improve performance. On a good rasterizer, that will not change performance too much. > It is probably worth testing and comparing the two. I have implemented the bezier version of this patch (find the attached patch). I did some benchmarks (the patch also includes the code used to get the data used in my analysis) and the bezier version is one order of magnitude slower than the non-bezier version. According to my results, the non-bezier version takes about 20 microseconds to draw while the bezier version takes about 220 microseconds. That is still fast in my oppinion. Created attachment 192780 [details]
Patch
Use bezier curve and fix small issues.
Created attachment 192958 [details]
Patch
Make bezier curve symmetric and update changelogs.
Created attachment 192959 [details]
Bezier with 1 pixel stroke thickness
Expectations using wavy bezier curve and 1 pixel stroke thickness.
Created attachment 192960 [details]
Bezier with 2 pixels stroke thickness
Expectations using wavy bezier curve and 2 pixels stroke thickness.
Created attachment 192961 [details]
Patch (EWS only version)
Same patch with css3-text enabled.
Comment on attachment 192961 [details] Patch (EWS only version) Attachment 192961 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17217028 Comment on attachment 192958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192958&action=review This looks good overall (except maybe the tail of the line). Mostly nitpicks: > Source/WebCore/rendering/InlineTextBox.cpp:961 > +void InlineTextBox::strokeWavyTextDecoration(GraphicsContext* context, const FloatPoint& point1, const FloatPoint& point2, float strokeThickness) Does this access any member of InlineTextBox? If not, it should be a local static function and not a member function. > Source/WebCore/rendering/InlineTextBox.cpp:971 > + const float controlPointDistance = 3 * max<float>(2, strokeThickness); > + const float step = 2 * max<float>(2, strokeThickness); Magic numbers. > Source/WebCore/rendering/InlineTextBox.cpp:972 > + float x1, y1, x2, y2; This should be declared locally + WebKit style. > Source/WebCore/rendering/InlineTextBox.cpp:973 > + float cp1x, cp1y, cp2x, cp2y; Ditto. Obscure names. > Source/WebCore/rendering/InlineTextBox.cpp:977 > + x1 = x2 = p1.x(); WebKit style. > Source/WebCore/rendering/InlineTextBox.cpp:979 > + // Make sure (x1, y1) < (x2, y2) Period. > Source/WebCore/rendering/InlineTextBox.cpp:992 > + while (y + 2 * step <= y2) { This should be a for() loop. Wouldn't this be too short for the tail? > Source/WebCore/rendering/InlineTextBox.cpp:998 > + } else { You should assert p1.y() == p2.y(). > Source/WebCore/rendering/InlineTextBox.cpp:1106 > + strokeWavyTextDecoration(context, FloatPoint(localOrigin.x(), localOrigin.y() + baseline + doubleOffset + 1), FloatPoint(localOrigin.x() + width, localOrigin.y() + baseline + doubleOffset + 1), textDecorationThickness); Break the arguments on local variables. > Source/WebCore/rendering/InlineTextBox.cpp:1123 > + strokeWavyTextDecoration(context, FloatPoint(localOrigin.x(), localOrigin.y() - doubleOffset), FloatPoint(localOrigin.x() + width, localOrigin.y() - doubleOffset), textDecorationThickness); Ditto. > Source/WebCore/rendering/InlineTextBox.cpp:1127 > context->drawLineForText(localOrigin, width, isPrinting); I would indent this line for readability. > Source/WebCore/rendering/InlineTextBox.cpp:1131 > + } missing break? > Source/WebCore/rendering/InlineTextBox.cpp:1139 > + strokeWavyTextDecoration(context, FloatPoint(localOrigin.x(), localOrigin.y() + 2 * baseline / 3), FloatPoint(localOrigin.x() + width, localOrigin.y() + 2 * baseline / 3), textDecorationThickness); Move the arguments outside the line. Comment on attachment 192958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192958&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:992 >> + while (y + 2 * step <= y2) { > > This should be a for() loop. > > Wouldn't this be too short for the tail? Yes, that can happen. With strokeThickness equals 1 pixel then it can be up to 7 pixels too short. I am not sure how to solve this problem. Do you know to draw a half Bezier curve? >> Source/WebCore/rendering/InlineTextBox.cpp:1131 >> + } > > missing break? No. This part belongs to the "default" part of the switch statement. Break is not necessary here and as far as I know about WebKit's code style it should not be used in this case. Created attachment 194898 [details]
Patch
Fix issues reported, except the curve is too short one.
Created attachment 194941 [details]
Patch
Fix the too short curve problem. I did a benchmark and this fix adds about 4% overhead, which translated to microseconds is less than the standard deviation I calculated. I think that is reasonable. One problem with this fix is that two wavy decorations in the same page but in different text strings can have slightly different shapes. I think most people cannot notice that without zooming in the page.
Comment on attachment 194941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194941&action=review This looks a lot better. Only minor comments. By the way, if this is the only test covering this feature, I think it needs _A LOT_ more tests: WebKit Transform, right to left text, vertical text, BIDI text, ruby text, etc. This must be addressed but it can be done in another patch. > Source/WebCore/rendering/InlineTextBox.cpp:991 > + * The start point (p1), controlPoint1), controlPoint2 and end point (p2) of the Bezier curve > + * form a diamond shape: Stale closing parenthesis after "controlPoint1". > Source/WebCore/rendering/InlineTextBox.cpp:1039 > + FloatPoint controlPoint1; > + FloatPoint controlPoint2; Move those down, closer to where they are used. The closer a declaration is to the variable use, the less you have to keep in mind when reading the code. > Source/WebCore/rendering/InlineTextBox.cpp:1044 > + // Make sure (x1, y1) < (x2, y2). This only make sure y1 <= y2. I would just remove the comment. > Source/WebCore/rendering/InlineTextBox.cpp:1062 > + int divisor = static_cast<int>(step); > + float quotient = static_cast<int>(y2 - y1 - 1) / divisor; > + float remainder = static_cast<int>(y2 - y1 - 1) % divisor; > + float adjustment = remainder / quotient; > + controlPointDistance += 2 * adjustment; > + step += adjustment; This whole block should be a static function. Looks like you should use unsigned instead of int. Why do you switch to integer at all for the step? Why do you use -1 for the length? I would think something like that would be better: float length = y2 - y1. unsigned stepCount = static_cast<unsigned>(length / step); float uncoveredLength = length - (stepCount * step); float adjustment = uncoveredLength / stepCount; step += adjustment; Created attachment 195421 [details]
Patch
Fix remaining issues reported.
Comment on attachment 194941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194941&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:1044 >> + // Make sure (x1, y1) < (x2, y2). > > This only make sure y1 <= y2. > I would just remove the comment. I meant "Make sure point (x1, y1) is at left of point (x2, y2)". I will remove the comment. >> Source/WebCore/rendering/InlineTextBox.cpp:1062 >> + step += adjustment; > > This whole block should be a static function. > > Looks like you should use unsigned instead of int. > > Why do you switch to integer at all for the step? > Why do you use -1 for the length? > > I would think something like that would be better: > float length = y2 - y1. > unsigned stepCount = static_cast<unsigned>(length / step); > float uncoveredLength = length - (stepCount * step); > float adjustment = uncoveredLength / stepCount; > step += adjustment; I used a module operator (%), I was not sure it if would work with float. Anyway, your suggestion does not need that operation so the casting is not necessary. I just fixed a small issue when calculating the covered lenght. Since one Bezier curve starts at the same pixels as the previous one ended we need to subtract (stepCount - 1) pixels from the covered lenght. The second line becomes "float uncoveredLength = length - (stepCount * step - (stepCount -1));". Without the -1 the last Bezier curve was not draw sometimes because the decoration crossed the x2 coordinate (for horizontal wavy decoration). Now everything works as it should and the -1 can be removed. Comment on attachment 195421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195421&action=review > Source/WebCore/rendering/InlineTextBox.cpp:988 > +// Adjust step to make wavy decoration to cover all the distance between start point (p1) and end point (p2). You can remove this, the name of your function says it all. > Source/WebCore/rendering/InlineTextBox.cpp:991 > + if (step <= 0 || length <= 0) Could step really be <= 0? Sounds like this should be an assertion. > Source/WebCore/rendering/InlineTextBox.cpp:999 > + float uncoveredLength = length - (stepCount * step - (stepCount -1)); Missing space between minus and 1. > Source/WebCore/rendering/InlineTextBox.cpp:1029 > + * step > + * |-----------| > + * controlPoint1 > + * + > + * > + * > + * . . > + * . . > + * . . > + * (x1, y1) p1 + . + p2 (x2, y2) - <--- Decoration's axis > + * . . | > + * . . | > + * . . | controlPointDistance > + * | > + * | > + * + - > + * controlPoint2 > + * > + * |-----------| > + * step Usually I am strongly against comment but I have to say this is great to help understand your code. Good job on this. > Source/WebCore/rendering/InlineTextBox.cpp:1055 > + float x1 = p1.x(); Maybe name the x1 differently to express it is the stable axis? Maybe just xAxis? > Source/WebCore/rendering/InlineTextBox.cpp:1081 > + float y1 = p1.y(); Ditto for the name + move thins one line up. Comment on attachment 195421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195421&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:991 >> + if (step <= 0 || length <= 0) > > Could step really be <= 0? > Sounds like this should be an assertion. I do not think so. I will change the check on step to an assert and keep the one for lenght since it can be 0 if the start and end points of the decoration are the same. >> Source/WebCore/rendering/InlineTextBox.cpp:1029 >> + * step > > Usually I am strongly against comment but I have to say this is great to help understand your code. Good job on this. Thanks :-) >> Source/WebCore/rendering/InlineTextBox.cpp:1055 >> + float x1 = p1.x(); > > Maybe name the x1 differently to express it is the stable axis? > > Maybe just xAxis? I will change it to xAxis and yAxis. Created attachment 195633 [details]
Patch
Patch for landing.
Comment on attachment 195633 [details] Patch Clearing flags on attachment: 195633 Committed r147170: <http://trac.webkit.org/changeset/147170> All reviewed patches have landed. Closing bug. |