Bug 28268 - adjustLineToPixelBoundaries used in platform/GraphicsContext drawLine needs refactoring finished.
: adjustLineToPixelBoundaries used in platform/GraphicsContext drawLine needs r...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-08-13 11:05 PST by
Modified: 2009-11-24 06:35 PST (History)


Attachments
Patch to move adjustLineToPixelBoundaries to GraphicsContext.cpp and cleanup existing uses of function. (7.23 KB, patch)
2009-08-13 11:25 PST, Mike Fenton
eric: review-
Review Patch | 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 PST, Mike Fenton
no flags Review Patch | Details | Formatted Diff | Diff
Style patch for GraphicsContext.cpp/h GraphicsContextCairo.cpp and GraphicsContextQt.cpp (16.13 KB, patch)
2009-08-13 12:30 PST, Mike Fenton
no flags Review Patch | 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 PST, Mike Fenton
eric: review+
Review Patch | 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 PST, Mike Fenton
eric: review-
Review Patch | Details | Formatted Diff | Diff
Style patch for GraphicsContextCG.cpp. (22.93 KB, patch)
2009-08-13 13:16 PST, Mike Fenton
eric: review+
Review Patch | Details | Formatted Diff | Diff
Fix Changelog in patch #34776 (16.08 KB, patch)
2009-08-14 06:00 PST, Mike Fenton
no flags Review Patch | 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 PST, Mike Fenton
eric: review+
eric: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Fix Changelog in patch #34778 (22.87 KB, patch)
2009-08-14 06:35 PST, Mike Fenton
no flags Review Patch | Details | Formatted Diff | Diff
Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries. (4.51 KB, patch)
2009-08-14 08:27 PST, Mike Fenton
no flags Review Patch | Details | Formatted Diff | Diff
Style patch for GraphicsContextSkia.cpp (4.36 KB, patch)
2009-08-14 08:29 PST, Mike Fenton
no flags Review Patch | 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 PST, Mike Fenton
eric: review-
Review Patch | 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 PST, Mike Fenton
no flags Review Patch | 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 PST, Mike Fenton
eric: review+
eric: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-13 11:05:03 PST
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 From 2009-08-13 11:25:02 PST -------
Created an attachment (id=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 From 2009-08-13 11:42:49 PST -------
(From update of attachment 34761 [details])
+        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 From 2009-08-13 12:27:02 PST -------
Created an attachment (id=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 From 2009-08-13 12:30:19 PST -------
Created an attachment (id=34770) [details]
Style patch for GraphicsContext.cpp/h GraphicsContextCairo.cpp and GraphicsContextQt.cpp
------- Comment #5 From 2009-08-13 13:03:36 PST -------
Created an attachment (id=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 From 2009-08-13 13:15:35 PST -------
Created an attachment (id=34777) [details]
Patch for GraphicsContextCG.cpp to use adjustLineToPixelBoundaries to replace duplicated code.

Note: This patch replaces code that is duplicated in the function but has not been built for the platform.
------- Comment #7 From 2009-08-13 13:16:31 PST -------
Created an attachment (id=34778) [details]
Style patch for GraphicsContextCG.cpp.
------- Comment #8 From 2009-08-13 20:13:57 PST -------
(From update of attachment 34776 [details])
Looks fine, but the ChangeLog is going to break automated landing.
------- Comment #9 From 2009-08-13 20:14:43 PST -------
(From update of attachment 34767 [details])
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 From 2009-08-13 20:15:36 PST -------
(From update of attachment 34777 [details])
Looks fine.  The ChangeLog will confuse bugzilla-tool.  it won't know how to fix that diff.
------- Comment #11 From 2009-08-13 20:16:26 PST -------
(From update of attachment 34777 [details])
This is the same as the next patch.  I don't think this is what you meant to upload. r-
------- Comment #12 From 2009-08-13 20:16:54 PST -------
(From update of attachment 34778 [details])
Style fixes look fine.  ChangeLog will confuse bugzilla-tool/svn-apply
------- Comment #13 From 2009-08-14 06:00:51 PST -------
Created an attachment (id=34834) [details]
Fix Changelog from in patch #34776
------- Comment #14 From 2009-08-14 06:18:13 PST -------
Created an attachment (id=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 From 2009-08-14 06:35:05 PST -------
Created an attachment (id=34838) [details]
Fix Changelog in patch #34778
------- Comment #16 From 2009-08-14 08:27:26 PST -------
Created an attachment (id=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 From 2009-08-14 08:29:18 PST -------
Created an attachment (id=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 From 2009-08-14 11:51:35 PST -------
Created an attachment (id=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 From 2009-08-17 07:01:46 PST -------
Landed with r47359. Clearing flags...
------- Comment #20 From 2009-08-17 07:19:46 PST -------
Landed with r47360.  Clearing flags.
------- Comment #21 From 2009-08-17 08:08:23 PST -------
(From update of attachment 34767 [details])
Landed with r47361.  Clearing flags.
------- Comment #22 From 2009-08-17 16:32:27 PST -------
(From update of attachment 34835 [details])
Ideally this would be one patch per bug.  Looks OK.
------- Comment #23 From 2009-08-17 16:35:41 PST -------
Adding Brett so he sees the Skia changes go by.
------- Comment #24 From 2009-08-17 16:35:50 PST -------
(From update of attachment 34842 [details])
Looks sane.
------- Comment #25 From 2009-08-17 16:36:24 PST -------
(From update of attachment 34843 [details])
LGTM.
------- Comment #26 From 2009-08-17 16:42:36 PST -------
(From update of attachment 34865 [details])
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 From 2009-08-17 16:47:29 PST -------
(From update of attachment 34835 [details])
Rejecting patch 34835 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1
------- Comment #28 From 2009-08-17 16:58:08 PST -------
(From update of attachment 34842 [details])
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 From 2009-08-17 16:58:25 PST -------
(From update of attachment 34843 [details])
Marking cq- for the same reasons as above.
------- Comment #30 From 2009-08-18 07:17:54 PST -------
Created an attachment (id=35040) [details]
Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested.
------- Comment #31 From 2009-08-18 08:39:42 PST -------
(From update of attachment 35040 [details])
This patch is identical to the previous, or?
------- Comment #32 From 2009-08-18 09:00:34 PST -------
Created an attachment (id=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 From 2009-08-18 09:28:16 PST -------
(From update of attachment 35044 [details])
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 From 2009-08-18 14:12:01 PST -------
(From update of attachment 35044 [details])
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 From 2009-08-18 14:25:35 PST -------
(From update of attachment 35044 [details])
Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436
------- Comment #36 From 2009-08-18 14:59:03 PST -------
(From update of attachment 35044 [details])
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 From 2009-08-18 15:03:37 PST -------
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 From 2009-08-18 20:13:23 PST -------
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 From 2009-08-18 22:42:50 PST -------
(From update of attachment 34835 [details])
Rejecting patch 34835 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1
------- Comment #40 From 2009-08-18 23:14:13 PST -------
(From update of attachment 34842 [details])
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 From 2009-08-18 23:31:34 PST -------
(From update of attachment 34843 [details])
Clearing flags on attachment: 34843

Committed r47495: <http://trac.webkit.org/changeset/47495>
------- Comment #42 From 2009-08-18 23:31:43 PST -------
(From update of attachment 35044 [details])
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 From 2009-08-19 00:34:54 PST -------
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 From 2009-08-19 00:35:50 PST -------
(From update of attachment 34842 [details])
This was rejected for an out-of-date ChangeLog. I think that was just a bug in the commit-queue.  retrying.
------- Comment #45 From 2009-08-19 00:47:12 PST -------
(From update of attachment 34842 [details])
Clearing flags on attachment: 34842

Committed r47498: <http://trac.webkit.org/changeset/47498>
------- Comment #46 From 2009-08-19 00:47:18 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #47 From 2009-08-19 13:12:21 PST -------
(In reply to comment #45)
> (From update of attachment 34842 [details] [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 From 2009-08-19 13:52:12 PST -------
I'm happy to roll it out Peter, unless you'd like to make a fix for Skia.
------- Comment #49 From 2009-08-19 13:57:03 PST -------
(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 From 2009-11-24 06:35:35 PST -------
*** Bug 14440 has been marked as a duplicate of this bug. ***