WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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.
Top of Page
Format For Printing
XML
Clone This Bug