Bug 28268 – adjustLineToPixelBoundaries used in platform/GraphicsContext drawLine needs refactoring finished.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Fenton]

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.

[Mike Fenton]

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.

[Eric Seidel (no email)]

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!

[Mike Fenton]

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.

[Mike Fenton]

Created attachment 34770 [details]
Style patch for GraphicsContext.cpp/h GraphicsContextCairo.cpp and GraphicsContextQt.cpp

[Mike Fenton]

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.

[Mike Fenton]

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.

[Mike Fenton]

Created attachment 34778 [details]
Style patch for GraphicsContextCG.cpp.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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-

[Eric Seidel (no email)]

Comment on attachment 34778 [details]
Style patch for GraphicsContextCG.cpp.

Style fixes look fine.  ChangeLog will confuse bugzilla-tool/svn-apply

[Mike Fenton]

Created attachment 34834 [details]
Fix Changelog in patch #34776

[Mike Fenton]

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.

[Mike Fenton]

Created attachment 34838 [details]
Fix Changelog in patch #34778

[Mike Fenton]

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.

[Mike Fenton]

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.

[Mike Fenton]

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.

[Adam Treat]

Landed with r47359. Clearing flags...

[Adam Treat]

Landed with r47360.  Clearing flags.

[Adam Treat]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Adding Brett so he sees the Skia changes go by.

[Eric Seidel (no email)]

Comment on attachment 34842 [details]
Patch for GraphicsContextSkia.cpp to use adjustLineToPixelBoundaries.

Looks sane.

[Eric Seidel (no email)]

Comment on attachment 34843 [details]
Style patch for GraphicsContextSkia.cpp

LGTM.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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-.

[Eric Seidel (no email)]

Comment on attachment 34843 [details]
Style patch for GraphicsContextSkia.cpp

Marking cq- for the same reasons as above.

[Mike Fenton]

Created attachment 35040 [details]
Rename functions in patch - Refactor patternWidth and patternOffset calculations from GraphicsContextQt/Cairo/CG.cpp into GraphicsContext.cpp as requested.

[Eric Seidel (no email)]

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?

[Mike Fenton]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Adam Barth]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Comment on attachment 34843 [details]
Style patch for GraphicsContextSkia.cpp

Clearing flags on attachment: 34843

Committed r47495: <http://trac.webkit.org/changeset/47495>

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Peter Kasting]

(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

[Eric Seidel (no email)]

I'm happy to roll it out Peter, unless you'd like to make a fix for Skia.

[Peter Kasting]

(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.

[Simon Hausmann]

*** Bug 14440 has been marked as a duplicate of this bug. ***