Bug 112592

Summary:

[CSS Exclusions] Add support for the simple case of padding a polygonal shape-inside

Product:

WebKit

Reporter:

Hans Muller <giles_joplin>

Component:

CSS

Assignee:

Hans Muller <giles_joplin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bjonesbe, dglazkov, eric, esprehn+autocc, ojan.autocc, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

111080

Attachments:

Description	Flags
Screenshot that illustrates the padded boundary.	none
Patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-04	none
Archive of layout-test-results from gce-cr-linux-08	none
Patch	none
Patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-01	none
Patch	krit: review+, krit: commit-queue-
Patch	webkit.review.bot: commit-queue-
Patch	none

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	Details
Patch (22.30 KB, patch) 2013-03-21 16:44 PDT, Hans Muller	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
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	Details
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	Details
Patch (22.61 KB, patch) 2013-03-22 09:35 PDT, Hans Muller	no flags	Details Formatted Diff Diff
Patch (22.61 KB, patch) 2013-03-22 10:06 PDT, Hans Muller	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
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	Details
Patch (22.95 KB, patch) 2013-03-25 12:28 PDT, Hans Muller	krit: review+ krit: commit-queue-	Details Formatted Diff Diff
Patch (22.98 KB, patch) 2013-03-27 19:02 PDT, Hans Muller	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Patch (22.97 KB, patch) 2013-03-28 06:54 PDT, Hans Muller	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

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

Created attachment 194384 [details] Patch

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.