Summary: | Make use of CG rounded-rect primitives | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||
Component: | Layout and Rendering | Assignee: | Tim Horton <thorton> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | mrahaman, simon.fraser, webkit.review.bot, zimmermann | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Tim Horton
2012-02-29 12:18:53 PST
Created attachment 129500 [details]
patch
Comment on attachment 129500 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129500&action=review > Source/WebKit/mac/WebCoreSupport/WebSystemInterface.mm:181 > +#if PLATFORM(MAC) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD) Not sure why you need the PLATFORM(MAC) here. This is a .mm file. Landed in http://trac.webkit.org/changeset/109255 with Simon's adjustment. (In reply to comment #3) > Landed in http://trac.webkit.org/changeset/109255 with Simon's adjustment. This is giving a build break on Windows port for revision: 109309 & the error is \platform\graphics\cg\PathCG.cpp(244) : error C3861: 'wkCGPathAddRoundedRect': identifier not found. Did any one notice this before? (In reply to comment #4) > (In reply to comment #3) > > Landed in http://trac.webkit.org/changeset/109255 with Simon's adjustment. > > This is giving a build break on Windows port for revision: 109309 & the error is > \platform\graphics\cg\PathCG.cpp(244) : error C3861: 'wkCGPathAddRoundedRect': identifier not found. > > Did any one notice this before? Upon further investigation I found that, the cause of the build break is that the changes in WebCore are inside #if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD), where the expected behavior is that these changes should work for LION & above(as per ChangeLog), but because of the above conditional the changes are applied for non-MAC ports also. So, I was thinking the correct fix should be something like #if PLATFORM(MAC) #if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD) Proposed Changes.. #endif #endif If any one can please comment on this, I can create a new issue & submit a patch for the same, Thanks. Reopening bug. Some pixel test results for SVG now needs rebaselines. One test broke, that's why I'm reopening: svg/custom/dasharrayOrigin.svg. It uses rounded <rects> and stroke-dasharray - it seems that combination doesn't work when using the optimized primitives from CG. (In reply to comment #5) > #if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD), where the expected behavior is that these changes should work for LION & above(as per ChangeLog), but because of the above conditional the changes are applied for non-MAC ports also. That only affects CG/Win, as win is the only non-mac port using CoreGraphics. I think your idea is correct, unless Tim can supply the needed function for win as well. Created attachment 129683 [details]
Follow-up patch
(In reply to comment #8) > Created an attachment (id=129683) [details] > Follow-up patch This includes the PLATFORM(MAC) fix mentioned by Mustafizur Rahaman as well, which should fix CG/Win builds again. This patch lets my Lion SVG pixel baseline pass with --tolerance 0 again. I've switched to the old code path for SVG again, as dashed strokes seem to fail with the new native approach. Maybe it'll never work as SVG expects it. Comment on attachment 129683 [details] Follow-up patch (In reply to comment #9) > I've switched to the old code path for SVG again, as dashed strokes seem to fail with the new native approach. Maybe it'll never work as SVG expects it. I've discussed this with Zoltan on IRC -- I'm holding back this patch, we should discuss with Tim/Simon first if wkCGPathAddRoundedRect can be fixed, if not, we can revisit my patch, but only pass the 'PreferBeziers..' argument if the SVG stroke-dasharray is set, otherwise use the default PreferNative method. I'm going to roll this out instead for now, to have passing tests again. Reverted r109255 for reason: Breaks rounded rects with dashed strokes in SVG Committed r109340: <http://trac.webkit.org/changeset/109340> Note: I didn't try, but if we have test cases of rounded rects in <canvas> with webkitLineDash set, then this should be broken as well. Needs Apple comments, what this fast-path actually supports :-) (In reply to comment #11) > Reverted r109255 for reason: > Breaks rounded rects with dashed strokes in SVG > > Committed r109340: <http://trac.webkit.org/changeset/109340> > > Note: I didn't try, but if we have test cases of rounded rects in <canvas> with webkitLineDash set, then this should be broken as well. > > Needs Apple comments, what this fast-path actually supports :-) Canvas doesn't have an intrinsic rounded rectangle, so it's not affected. What did you see? All I see is that the origin of the line dash is incorrect. I'm not completely sure how to fix it, but I'm not sure that that calls into question what the fast path supports (after all, it is just modifying the path, in the end). From IRC: thorton: Wild|AWAY: what would you prefer! Wild|AWAY: thorton: platformAddRoundedRect maybe? which can then use fallbackAddRoundedRect, if it has no platform impl Wild|AWAY: s/fallbackAddRoundedRect/addRoundedRectsCreatedByBeziers/, and put that in Path.cpp Wild|AWAY: and the platformAdd.. in the PathCG.cpp / PathQt.cpp (not now, but in future) etc, .. I think we should land this plus Niko's patch, plus the Windows fix, plus the minor refactor that Niko wants, ending with it enabled in all cases except SVG. All of the CSS tests passed, and CSS doesn't specify line dash origin; Canvas can't hit this code because it has neither a rounded rectangle primitive nor dashed line support. The path created by wkCGPathAddRoundedRect currently starts in the middle of the righthand segment (the 'origin' if you were to split this into quadrants/consider it as a circle). The path created by addBeziersForRoundedRect starts at the left of the topmost segment. To adjust the dash origin, we would have to somehow plumb knowledge of which rounded rectangle method was used through from Path to GraphicsContext; specifically, we'd need that knowledge when setting the line dash settings. Then, we can offset the phase of the line dash by the correct distance (I've worked it out for the case where rx and ry are equal -- and that works -- but not the more general case). This seems quite irritating, though, since knowledge of the Path and the code that set the line dash are separated by quite a gap. Created attachment 132936 [details]
patch
Attachment 132936 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/Path.h:155: The parameter name "strategy" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 132939 [details]
style fix
Landed again in http://trac.webkit.org/changeset/111600 |