RESOLVED FIXED 112592
[CSS Exclusions] Add support for the simple case of padding a polygonal shape-inside
https://bugs.webkit.org/show_bug.cgi?id=112592
Summary [CSS Exclusions] Add support for the simple case of padding a polygonal shape...
Hans Muller
Reported 2013-03-18 10:24:51 PDT
Computing the padded shape-inside boundary for a polygon is moderately straightforward when the padded boundary is the same shape as the original boundary. In this case all of the edges are simply inset by the shape-padding distance. Edges that meet at a reflex vertex must be connected by a circular arc which we will approximate with a regular polygon segment. The attached screenshot demonstrates this case. As you can see, the intersection of each pair of inset padded edges becomes a vertex of the padded shape, except when the original common vertex was a reflex vertex. In the screenshot the original boundary's reflex vertex is labeled "4" and the approximation to the circular arc that joins inset edges 3-4 and 4-5 is labeled "[4" and "4]".
Attachments
Screenshot that illustrates the padded boundary. (59.51 KB, image/png)
2013-03-18 10:25 PDT, Hans Muller
no flags
Patch (22.30 KB, patch)
2013-03-21 16:44 PDT, Hans Muller
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-04 (1.29 MB, application/zip)
2013-03-21 17:58 PDT, WebKit Review Bot
no flags
Archive of layout-test-results from gce-cr-linux-08 (987.32 KB, application/zip)
2013-03-21 18:28 PDT, WebKit Review Bot
no flags
Patch (22.61 KB, patch)
2013-03-22 09:35 PDT, Hans Muller
no flags
Patch (22.61 KB, patch)
2013-03-22 10:06 PDT, Hans Muller
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-01 (737.03 KB, application/zip)
2013-03-22 10:57 PDT, WebKit Review Bot
no flags
Patch (22.95 KB, patch)
2013-03-25 12:28 PDT, Hans Muller
krit: review+
krit: commit-queue-
Patch (22.98 KB, patch)
2013-03-27 19:02 PDT, Hans Muller
webkit.review.bot: commit-queue-
Patch (22.97 KB, patch)
2013-03-28 06:54 PDT, Hans Muller
no flags
Hans Muller
Comment 1 2013-03-18 10:25:53 PDT
Created attachment 193599 [details] Screenshot that illustrates the padded boundary.
Hans Muller
Comment 2 2013-03-21 16:44:55 PDT
WebKit Review Bot
Comment 3 2013-03-21 17:58:24 PDT
Comment on attachment 194384 [details] Patch Attachment 194384 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17216569 New failing tests: fast/exclusions/shape-inside/shape-inside-polygon-padding-001.html fast/exclusions/shape-inside/shape-inside-polygon-padding-003.html
WebKit Review Bot
Comment 4 2013-03-21 17:58:27 PDT
Created attachment 194406 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
WebKit Review Bot
Comment 5 2013-03-21 18:28:24 PDT
Comment on attachment 194384 [details] Patch Attachment 194384 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17213837 New failing tests: fast/exclusions/shape-inside/shape-inside-polygon-padding-001.html fast/exclusions/shape-inside/shape-inside-polygon-padding-003.html
WebKit Review Bot
Comment 6 2013-03-21 18:28:27 PDT
Created attachment 194408 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Hans Muller
Comment 7 2013-03-22 09:35:43 PDT
Created attachment 194571 [details] Patch Corrected the element size problems that caused scrollbars to appear on Chromium for shape-inside-polygon-padding-00{1,3}-expected.html. Adjusted the margin-left for the arc-indent element in shape-inside-polygon-padding-003-expected.html when subpixel layout isn't enabled (because the value can't be safely truncated).
Hans Muller
Comment 8 2013-03-22 10:06:42 PDT
Created attachment 194581 [details] Patch Recorrected the arc-indent margin-left subpixel-layout problem...
WebKit Review Bot
Comment 9 2013-03-22 10:57:00 PDT
Comment on attachment 194581 [details] Patch Attachment 194581 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17133577 New failing tests: fast/exclusions/shape-inside/shape-inside-polygon-padding-003.html
WebKit Review Bot
Comment 10 2013-03-22 10:57:03 PDT
Created attachment 194598 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Hans Muller
Comment 11 2013-03-25 12:28:57 PDT
Created attachment 194902 [details] Patch After discussing the off by one layout error that occurred on the cr-linux bot with LeviW and others on IRC, I changed the polygon-padding-003 test. It now checks the layout programmatically and includes a +/- 1 tolerance to account for the small unpredictability in the underlying floating point computation.
Hans Muller
Comment 12 2013-03-25 15:41:23 PDT
(In reply to comment #11) > After discussing the off by one layout error that occurred on the cr-linux bot with LeviW and others on IRC, I changed the polygon-padding-003 test. It now checks the layout programmatically and includes a +/- 1 tolerance to account for the small unpredictability in the underlying floating point computation. I filed a bug about this problem, it's https://bugs.webkit.org/show_bug.cgi?id=113245.
Dirk Schulze
Comment 13 2013-03-27 15:28:27 PDT
Comment on attachment 194902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194902&action=review I need to trust you at the math. :). Some comments inside. r=me > Source/WebCore/rendering/ExclusionPolygon.cpp:108 > + return inwardEdgeNormal(edge) * -1; Maybe I wrote to much in JS lately, doesn't -inwardEdgeNormal(edge) work? :P > Source/WebCore/rendering/ExclusionPolygon.cpp:113 > + float startAngle = atan2(startArcVertex.y() - arcCenter.y(), startArcVertex.x() - arcCenter.x()); No atan2f? > Source/WebCore/rendering/ExclusionPolygon.cpp:116 > + startAngle += piFloat * 2; I thought we have pi*2 in our Math header? > Source/WebCore/rendering/ExclusionPolygon.cpp:123 > + for (unsigned i = 1; i < arcSegmentCount; i++) { ++i
Hans Muller
Comment 14 2013-03-27 17:30:23 PDT
(In reply to comment #13) > (From update of attachment 194902 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194902&action=review Thanks for the feedback! > > Source/WebCore/rendering/ExclusionPolygon.cpp:113 > > + float startAngle = atan2(startArcVertex.y() - arcCenter.y(), startArcVertex.x() - arcCenter.x()); > > No atan2f? I believe atan2f is just a C backwards compatibility function. The C++ atan2 is overloaded for different numeric types and it looks like MathExtras actually #defines it on some platforms to work around bugs. > > > Source/WebCore/rendering/ExclusionPolygon.cpp:116 > > + startAngle += piFloat * 2; > > I thought we have pi*2 in our Math header? We have pi/2 and pi/4 but not pi*2.
Hans Muller
Comment 15 2013-03-27 19:02:18 PDT
Created attachment 195458 [details] Patch Made the changes that Dirk asked for.
WebKit Review Bot
Comment 16 2013-03-27 20:32:43 PDT
Comment on attachment 195458 [details] Patch Rejecting attachment 195458 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'validate-changelog', '--non-interactive', 195458, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-commit-queue.appspot.com/results/17349029
Hans Muller
Comment 17 2013-03-28 06:54:48 PDT
Created attachment 195562 [details] Patch Added "reviewed by" entries to the ChangeLogs.
WebKit Review Bot
Comment 18 2013-03-28 07:18:29 PDT
Comment on attachment 195562 [details] Patch Clearing flags on attachment: 195562 Committed r147111: <http://trac.webkit.org/changeset/147111>
WebKit Review Bot
Comment 19 2013-03-28 07:18:33 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 20 2013-04-22 09:03:17 PDT
When I reviewed this for Blink just now, I noticed a lack of OwnPtr/PassOwnPtr usage. I'd recommend a follow-up patch to add such. Manual memory management is asking for a bad time. :)
Note You need to log in before you can comment on or make changes to this bug.