Bug 112592 - [CSS Exclusions] Add support for the simple case of padding a polygonal shape-inside
Summary: [CSS Exclusions] Add support for the simple case of padding a polygonal shape...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Muller
URL:
Keywords:
Depends on:
Blocks: 111080
  Show dependency treegraph
 
Reported: 2013-03-18 10:24 PDT by Hans Muller
Modified: 2013-04-22 09:03 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Muller 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]".
Comment 1 Hans Muller 2013-03-18 10:25:53 PDT
Created attachment 193599 [details]
Screenshot that illustrates the padded boundary.
Comment 2 Hans Muller 2013-03-21 16:44:55 PDT
Created attachment 194384 [details]
Patch
Comment 3 WebKit Review Bot 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
Comment 4 WebKit Review Bot 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
Comment 5 WebKit Review Bot 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
Comment 6 WebKit Review Bot 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
Comment 7 Hans Muller 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).
Comment 8 Hans Muller 2013-03-22 10:06:42 PDT
Created attachment 194581 [details]
Patch

Recorrected the arc-indent margin-left subpixel-layout problem...
Comment 9 WebKit Review Bot 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
Comment 10 WebKit Review Bot 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
Comment 11 Hans Muller 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.
Comment 12 Hans Muller 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.
Comment 13 Dirk Schulze 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
Comment 14 Hans Muller 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.
Comment 15 Hans Muller 2013-03-27 19:02:18 PDT
Created attachment 195458 [details]
Patch

Made the changes that Dirk asked for.
Comment 16 WebKit Review Bot 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
Comment 17 Hans Muller 2013-03-28 06:54:48 PDT
Created attachment 195562 [details]
Patch

Added "reviewed by" entries to the ChangeLogs.
Comment 18 WebKit Review Bot 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>
Comment 19 WebKit Review Bot 2013-03-28 07:18:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Eric Seidel (no email) 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. :)