Bug 28268 - adjustLineToPixelBoundaries used in platform/GraphicsContext drawLine needs refactoring finished.
: adjustLineToPixelBoundaries used in platform/GraphicsContext drawLine needs r...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-13 11:05 PDT by Mike Fenton
Modified: 2009-11-24 06:35 PST (History)
6 users (show)

See Also:


Attachments
Patch to move adjustLineToPixelBoundaries to GraphicsContext.cpp and cleanup existing uses of function. (7.23 KB, patch)
2009-08-13 11:25 PDT, Mike Fenton
eric: review-
Details | Formatted Diff | Diff
Replacement patch for move adjustLineToPixelBoundaries to GraphicsContext.cpp and cleanup existing uses of function based on review feedback. (7.23 KB, patch)
2009-08-13 12:27 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Style patch for GraphicsContext.cpp/h GraphicsContextCairo.cpp and GraphicsContextQt.cpp (16.13 KB, patch)
2009-08-13 12:30 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Fix Style patch to have correct replacement for % 2 == 0. Replaces Style patch for GraphicsContext.cpp.h GraphicsContextCairo.cpp and GraphicsContextQt.cpp. (16.14 KB, patch)
2009-08-13 13:03 PDT, Mike Fenton
eric: review+
Details | Formatted Diff | Diff
Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 in this bug. (22.93 KB, patch)
2009-08-13 13:15 PDT, Mike Fenton
eric: review-
Details | Formatted Diff | Diff
Style patch for GraphicsContextCG.cpp. (22.93 KB, patch)
2009-08-13 13:16 PDT, Mike Fenton
eric: review+
Details | Formatted Diff | Diff
Fix Changelog in patch #34776 (16.08 KB, patch)
2009-08-14 06:00 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code. Note: This patch is dependent on attachment 34767 in this bug. (2.72 KB, patch)
2009-08-14 06:18 PDT, Mike Fenton
eric: review+
eric: commit‑queue-
Details | Formatted Diff | Diff
Fix Changelog in patch #34778 (22.87 KB, patch)
2009-08-14 06:35 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries. (4.51 KB, patch)
2009-08-14 08:27 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Style patch for GraphicsContextSkia.cpp (4.36 KB, patch)
2009-08-14 08:29 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp. (14.54 KB, patch)
2009-08-14 11:51 PDT, Mike Fenton
eric: review-
Details | Formatted Diff | Diff
Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested. (14.56 KB, patch)
2009-08-18 07:17 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff
Correct version of Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested. (17.10 KB, patch)
2009-08-18 09:00 PDT, Mike Fenton
eric: review+
eric: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Fenton 2009-08-13 11:05:03 PDT
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.
Comment 1 Mike Fenton 2009-08-13 11:25:02 PDT
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 2 Eric Seidel 2009-08-13 11:42:49 PDT
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!
Comment 3 Mike Fenton 2009-08-13 12:27:02 PDT
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.
Comment 4 Mike Fenton 2009-08-13 12:30:19 PDT
Created attachment 34770 [details]
Style patch for GraphicsContext.cpp/h GraphicsContextCairo.cpp and GraphicsContextQt.cpp
Comment 5 Mike Fenton 2009-08-13 13:03:36 PDT
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.
Comment 6 Mike Fenton 2009-08-13 13:15:35 PDT
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.
Comment 7 Mike Fenton 2009-08-13 13:16:31 PDT
Created attachment 34778 [details]
Style patch for GraphicsContextCG.cpp.
Comment 8 Eric Seidel 2009-08-13 20:13:57 PDT
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 9 Eric Seidel 2009-08-13 20:14:43 PDT
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 10 Eric Seidel 2009-08-13 20:15:36 PDT
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 11 Eric Seidel 2009-08-13 20:16:26 PDT
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 12 Eric Seidel 2009-08-13 20:16:54 PDT
Comment on attachment 34778 [details]
Style patch for GraphicsContextCG.cpp.

Style fixes look fine.  ChangeLog will confuse bugzilla-tool/svn-apply
Comment 13 Mike Fenton 2009-08-14 06:00:51 PDT
Created attachment 34834 [details]
Fix Changelog in patch #34776
Comment 14 Mike Fenton 2009-08-14 06:18:13 PDT
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.
Comment 15 Mike Fenton 2009-08-14 06:35:05 PDT
Created attachment 34838 [details]
Fix Changelog in patch #34778
Comment 16 Mike Fenton 2009-08-14 08:27:26 PDT
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.
Comment 17 Mike Fenton 2009-08-14 08:29:18 PDT
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.
Comment 18 Mike Fenton 2009-08-14 11:51:35 PDT
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.
Comment 19 Adam Treat 2009-08-17 07:01:46 PDT
Landed with r47359. Clearing flags...
Comment 20 Adam Treat 2009-08-17 07:19:46 PDT
Landed with r47360.  Clearing flags.
Comment 21 Adam Treat 2009-08-17 08:08:23 PDT
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 22 Eric Seidel 2009-08-17 16:32:27 PDT
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.
Comment 23 Eric Seidel 2009-08-17 16:35:41 PDT
Adding Brett so he sees the Skia changes go by.
Comment 24 Eric Seidel 2009-08-17 16:35:50 PDT
Comment on attachment 34842 [details]
Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries.

Looks sane.
Comment 25 Eric Seidel 2009-08-17 16:36:24 PDT
Comment on attachment 34843 [details]
Style patch for GraphicsContextSkia.cpp

LGTM.
Comment 26 Eric Seidel 2009-08-17 16:42:36 PDT
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 27 Eric Seidel 2009-08-17 16:47:29 PDT
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 28 Eric Seidel 2009-08-17 16:58:08 PDT
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 29 Eric Seidel 2009-08-17 16:58:25 PDT
Comment on attachment 34843 [details]
Style patch for GraphicsContextSkia.cpp

Marking cq- for the same reasons as above.
Comment 30 Mike Fenton 2009-08-18 07:17:54 PDT
Created attachment 35040 [details]
Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested.
Comment 31 Eric Seidel 2009-08-18 08:39:42 PDT
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?
Comment 32 Mike Fenton 2009-08-18 09:00:34 PDT
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 33 Eric Seidel 2009-08-18 09:28:16 PDT
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 34 Eric Seidel 2009-08-18 14:12:01 PDT
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 35 Eric Seidel 2009-08-18 14:25:35 PDT
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 36 Eric Seidel 2009-08-18 14:59:03 PDT
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.
Comment 37 Eric Seidel 2009-08-18 15:03:37 PDT
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.
Comment 38 Adam Barth 2009-08-18 20:13:23 PDT
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 39 Eric Seidel 2009-08-18 22:42:50 PDT
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 40 Eric Seidel 2009-08-18 23:14:13 PDT
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 41 Eric Seidel 2009-08-18 23:31:34 PDT
Comment on attachment 34843 [details]
Style patch for GraphicsContextSkia.cpp

Clearing flags on attachment: 34843

Committed r47495: <http://trac.webkit.org/changeset/47495>
Comment 42 Eric Seidel 2009-08-18 23:31:43 PDT
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.
Comment 43 Eric Seidel 2009-08-19 00:34:54 PDT
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.
Comment 44 Eric Seidel 2009-08-19 00:35:50 PDT
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 45 Eric Seidel 2009-08-19 00:47:12 PDT
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>
Comment 46 Eric Seidel 2009-08-19 00:47:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Peter Kasting 2009-08-19 13:12:21 PDT
(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
Comment 48 Eric Seidel 2009-08-19 13:52:12 PDT
I'm happy to roll it out Peter, unless you'd like to make a fix for Skia.
Comment 49 Peter Kasting 2009-08-19 13:57:03 PDT
(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.
Comment 50 Simon Hausmann 2009-11-24 06:35:35 PST
*** Bug 14440 has been marked as a duplicate of this bug. ***