The functions for drawLine in GraphicsContext share a large block of overlapping code that has already been separated out into the function adjustLineToPixelBoundaries for the Qt and Cairo ports. This function needs to be relocated to platform/GraphicsContext.cpp and removed from the individual GraphicsContextQt.cpp and GraphicsContextCairo.cpp. In addition to this move, the Skia and CG ports also reuse this block of code and should be updated to utilize the function.
Created attachment 34761 [details] Patch to move adjustLineToPixelBoundaries to GraphicsContext.cpp and cleanup existing uses of function. This patch moves adjustLineToPixelBoundaries to GraphicsContext.cpp and removes it from GraphicsContextQt.cpp and GraphicsContextCairo.cpp. Note: This change has only been compiled under Qt.
Comment on attachment 34761 [details] Patch to move adjustLineToPixelBoundaries to GraphicsContext.cpp and cleanup existing uses of function. + static void adjustLineToPixelBoundaries(FloatPoint& p1, FloatPoint& p2, float strokeWidth, const StrokeStyle& penStyle); "penStyle" name is not needed. KHTML has little to do with WebKit anymore: + // works out. For example, with a border width of 3, KHTML will pass us (y1+y2)/2, e.g., That should be "WebKIt" If you were a committer, I would have you fix those tiny nits when you landed, but since you aren't (yet), please post a revised patch. It's not clear when this function should be used. I suspect that CoreGraphics takes care of all this automatically internally? Is this function used to work around missing features in graphics libraries, or is this something that all ports should be using all the time? Please add some comments to the function declaration to help others understand when (if ever) this should be used. If it's only needed by Qt and CAiro, consider wrapping it in an #ifdef Otherwise looks fine. Thanks!
Created attachment 34767 [details] Replacement patch for move adjustLineToPixelBoundaries to GraphicsContext.cpp and cleanup existing uses of function based on review feedback. This patch replaces the previous patch by correcting the comment and the definition as requested. Patches for Skia and CG to follow as this also affects those platforms.
Created attachment 34770 [details] Style patch for GraphicsContext.cpp/h GraphicsContextCairo.cpp and GraphicsContextQt.cpp
Created attachment 34776 [details] Fix Style patch to have correct replacement for % 2 == 0. Replaces Style patch for GraphicsContext.cpp.h GraphicsContextCairo.cpp and GraphicsContextQt.cpp.
Created attachment 34777 [details] Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 [details] in this bug. Note: This patch replaces code that is duplicated in the function but has not been built for the platform.
Created attachment 34778 [details] Style patch for GraphicsContextCG.cpp.
Comment on attachment 34776 [details] Fix Style patch to have correct replacement for % 2 == 0. Replaces Style patch for GraphicsContext.cpp.h GraphicsContextCairo.cpp and GraphicsContextQt.cpp. Looks fine, but the ChangeLog is going to break automated landing.
Comment on attachment 34767 [details] Replacement patch for move adjustLineToPixelBoundaries to GraphicsContext.cpp and cleanup existing uses of function based on review feedback. Looks fine. The commit-queue will get confused by the extra r=? patches on this bug, so I won't mark this cq+ yet.
Comment on attachment 34777 [details] Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 [details] in this bug. Looks fine. The ChangeLog will confuse bugzilla-tool. it won't know how to fix that diff.
Comment on attachment 34777 [details] Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 [details] in this bug. This is the same as the next patch. I don't think this is what you meant to upload. r-
Comment on attachment 34778 [details] Style patch for GraphicsContextCG.cpp. Style fixes look fine. ChangeLog will confuse bugzilla-tool/svn-apply
Created attachment 34834 [details] Fix Changelog in patch #34776
Created attachment 34835 [details] Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 [details] in this bug. Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 [details] in this bug.
Created attachment 34838 [details] Fix Changelog in patch #34778
Created attachment 34842 [details] Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries. This patch modifies drawLine in Skia to use adjustLineToPixelBoundaries and should maintain the existing functionality. Note: This patch is dependent on attachment 34767 [details] in this bug and has not been compiled for Skia.
Created attachment 34843 [details] Style patch for GraphicsContextSkia.cpp Note: This leaves the header order partially against WebKit style in order to group Skia headers.
Created attachment 34865 [details] Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp. Note: This patch should be applied after other patches in this bug to avoid conflicts.
Landed with r47359. Clearing flags...
Landed with r47360. Clearing flags.
Comment on attachment 34767 [details] Replacement patch for move adjustLineToPixelBoundaries to GraphicsContext.cpp and cleanup existing uses of function based on review feedback. Landed with r47361. Clearing flags.
Comment on attachment 34835 [details] Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 [details] in this bug. Ideally this would be one patch per bug. Looks OK.
Adding Brett so he sees the Skia changes go by.
Comment on attachment 34842 [details] Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries. Looks sane.
Comment on attachment 34843 [details] Style patch for GraphicsContextSkia.cpp LGTM.
Comment on attachment 34865 [details] Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp. These functions should be something like "patternOffsetForDashes" and "patternWidthForDashes" or similarly longer/clearer names. Then you can use ful names like "patternOffset" instead of "patOffset" for the locals. I was at first confused as to what these functions were for until I realized they only applied to the line decoration/stroke style code paths. Otherwise the patch looks good.
Comment on attachment 34835 [details] Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 [details] in this bug. Rejecting patch 34835 from commit-queue. This patch will require manual commit. WebKitTools/Scripts/build-webkit failed with exit code 1
Comment on attachment 34842 [details] Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries. Since the commit-queue can't build GraphicsContextSkia (And Chromium doesn't have a build bot), I don't feel comfortable having the commit-queue land these. Especially after the CG patch failed to build. Marking cq-.
Comment on attachment 34843 [details] Style patch for GraphicsContextSkia.cpp Marking cq- for the same reasons as above.
Created attachment 35040 [details] Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested.
Comment on attachment 35040 [details] Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested. This patch is identical to the previous, or?
Created attachment 35044 [details] Correct version of Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested. Upload correct file.
Comment on attachment 35044 [details] Correct version of Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested. This seems unneeded: + // Special case 1px dotted borders for speed. + if (width == 1) + return 1.0f; A few floating point divisions should not going to outweigh the cost of a function call. We don't even have to subtract the remainder here, do we? 602 int numSegments = (distance - remainder) / width; unless I'm forgetting how int / int works. (It's fine to do so, just surprised to see it). It seems that patternOffsetForDashes could probably use a bit more commentary around the math. Looks OK though.
Comment on attachment 35044 [details] Correct version of Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested. Rejecting patch 35044 from commit-queue. This patch will require manual commit. Failed to run "['git', 'svn', 'rebase']" exit_code: 1 cwd: None
Comment on attachment 35044 [details] Correct version of Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested. Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436
Comment on attachment 35044 [details] Correct version of Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested. Rejecting patch 35044 from commit-queue. This patch will require manual commit. Patch https://bugs.webkit.org/attachment.cgi?id=35044 from bug 28268 failed to download and apply.
patching file WebCore/platform/graphics/cg/GraphicsContextCG.cpp Hunk #2 FAILED at 154. Hunk #3 succeeded at 198 (offset 24 lines). Hunk #4 succeeded at 211 (offset 24 lines). Hunk #5 succeeded at 271 (offset 24 lines). Hunk #6 succeeded at 294 (offset 24 lines). 1 out of 6 hunks FAILED -- saving rejects to file WebCore/platform/graphics/cg/GraphicsContextCG.cpp.rej patch -p0 "WebCore/platform/graphics/cg/GraphicsContextCG.cpp" returned 1. Pass --force to ignore patch failures.
This bug is out of control. Let's give commit-queue a try. It doesn't matter that Skia isn't covered by the build bots. That's what the canaries are for.
Comment on attachment 34842 [details] Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries. Rejecting patch 34842 from commit-queue. This patch will require manual commit. Failed to run "['git', 'svn', 'rebase']" exit_code: 1 cwd: None
Comment on attachment 34843 [details] Style patch for GraphicsContextSkia.cpp Clearing flags on attachment: 34843 Committed r47495: <http://trac.webkit.org/changeset/47495>
Comment on attachment 34842 [details] Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries. This was rejected for an out-of-date ChangeLog. I think that was just a bug in the commit-queue. retrying.
Comment on attachment 34842 [details] Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries. Clearing flags on attachment: 34842 Committed r47498: <http://trac.webkit.org/changeset/47498>
All reviewed patches have been landed. Closing bug.
(In reply to comment #45) > (From update of attachment 34842 [details]) > Clearing flags on attachment: 34842 > > Committed r47498: <http://trac.webkit.org/changeset/47498> This seems to have caused the following compile errors: ebCore\platform\graphics\skia\GraphicsContextSkia.cpp(524) : error C2065: 'pts' : undeclared identifier WebCore\platform\graphics\skia\GraphicsContextSkia.cpp(535) : error C2440: 'initializing' : cannot convert from 'WebCore::FloatSize' to 'WebCore::FloatPoint' No constructor could take the source type, or constructor overload resolution was ambiguous
I'm happy to roll it out Peter, unless you'd like to make a fix for Skia.
(In reply to comment #48) > I'm happy to roll it out Peter, unless you'd like to make a fix for Skia. Landed a build fix in r47526. The code in question looked pretty simple so I think my fix is correct, but people can feel free to check.
*** Bug 14440 has been marked as a duplicate of this bug. ***