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: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 111080 | ||||||||||||||||||||||||
Attachments: |
|
Description
Hans Muller
2013-03-18 10:24:51 PDT
Created attachment 193599 [details]
Screenshot that illustrates the padded boundary.
Created attachment 194384 [details]
Patch
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 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 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 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
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).
Created attachment 194581 [details]
Patch
Recorrected the arc-indent margin-left subpixel-layout problem...
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 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
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.
(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 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 (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. Created attachment 195458 [details]
Patch
Made the changes that Dirk asked for.
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 Created attachment 195562 [details]
Patch
Added "reviewed by" entries to the ChangeLogs.
Comment on attachment 195562 [details] Patch Clearing flags on attachment: 195562 Committed r147111: <http://trac.webkit.org/changeset/147111> All reviewed patches have been landed. Closing bug. 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. :) |