Created attachment 102728 [details] Patch

This change doesn't have updated test expectations yet, I need to check a few more tests first.

Comment on attachment 102728 [details] Patch Attachment 102728 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9287878 New failing tests: fast/borders/border-antialiasing.html fast/box-shadow/basic-shadows.html fast/box-shadow/inset-with-extraordinary-radii-and-border.html fast/blockflow/border-radius-clipping-vertical-lr.html fast/borders/border-radius-inline-flow.html fast/box-shadow/inset-box-shadows.html fast/backgrounds/gradient-background-leakage.html fast/box-shadow/inset-box-shadow-radius.html fast/borders/border-radius-constraints.html fast/borders/border-radius-split-inline.html fast/backgrounds/border-radius-split-background-image.html fast/blockflow/box-shadow-vertical-rl.html fast/backgrounds/repeat/negative-offset-repeat-transformed.html fast/blockflow/box-shadow-horizontal-bt.html fast/borders/border-radius-huge-assert.html fast/box-shadow/border-radius-big.html css2.1/t0805-c5517-brdr-s-00-c.html http/tests/inspector/resource-tree/resource-tree-reload.html fast/blockflow/box-shadow-vertical-lr.html fast/backgrounds/border-radius-split-background.html

Created attachment 102745 [details] With updated test_expectations for tests which need rebaselining.

Comment on attachment 102745 [details] With updated test_expectations for tests which need rebaselining. View in context: https://bugs.webkit.org/attachment.cgi?id=102745&action=review > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:396 > + if (platformContext()->getXfermodeMode() == SkXfermode::kClear_Mode) This check seems fragile, since a different call might have the mode set to clear, call clipOut(), and then change modes before drawing. In that case they will get unantialiased clip. > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:400 > + platformContext()->makeGrContextCurrent(); I wonder if we should move the makecurrent call inside clipPathAntiAliased, just to cut down on all of the call-sites. Just a thought.

(In reply to comment #5) > (From update of attachment 102745 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102745&action=review > > > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:396 > > + if (platformContext()->getXfermodeMode() == SkXfermode::kClear_Mode) > > This check seems fragile, since a different call might have the mode set to clear, call clipOut(), and then change modes before drawing. In that case they will get unantialiased clip. I agree. Perhaps we can add a DCHECK or webkit equivalent that the mode isn't changed to clear mode while we have antialiased clip paths. Another option is to have a clearModeClipOut in the graphics context. I'm worried this is polluting the graphics context with a special case. Is there a way to make the antialiasing clip work with clear mode without major surgery? > > > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:400 > > + platformContext()->makeGrContextCurrent(); > > I wonder if we should move the makecurrent call inside clipPathAntiAliased, just to cut down on all of the call-sites. Just a thought. That's a good thought, it's easy to miss these.

I think the notion just breaks the API model of GraphicsContext, that you can know the xfermode before you actually draw something (or in this case, before you draw everything). I think we should definitely consider a specialty function void clearOut(Path); We can always implement it in terms of the current pattern, but allow ports to perform this more efficiently if they wish (e.g. skia can definitely do this more efficiently in both raster and gpu).

(In reply to comment #7) > I think the notion just breaks the API model of GraphicsContext, that you can know the xfermode before you actually draw something (or in this case, before you draw everything). I think we should definitely consider a specialty function > > void clearOut(Path); > > We can always implement it in terms of the current pattern, but allow ports to perform this more efficiently if they wish (e.g. skia can definitely do this more efficiently in both raster and gpu). How about fillOutside(Path), which if called with clear mode does clearOut? This feels more in harmony with the rest of the graphics context API and still solves our problem. Skia can implement this easily with its inverse path settings and the other implementations can do what they do already.

(In reply to comment #8) > (In reply to comment #7) > > I think the notion just breaks the API model of GraphicsContext, that you can know the xfermode before you actually draw something (or in this case, before you draw everything). I think we should definitely consider a specialty function > > > > void clearOut(Path); > > > > We can always implement it in terms of the current pattern, but allow ports to perform this more efficiently if they wish (e.g. skia can definitely do this more efficiently in both raster and gpu). > > How about fillOutside(Path), which if called with clear mode does clearOut? This feels more in harmony with the rest of the graphics context API and still solves our problem. Skia can implement this easily with its inverse path settings and the other implementations can do what they do already. BTW either approach of clearOut or fillOutside will probably need path and rect versions.

I still wonder if simply removing the callers to clipOut() that set the mode to clear is a better option, since the only caller I know of is displayTransparencyElsewhere(), which is fundamentally broken.

(In reply to comment #10) > I still wonder if simply removing the callers to clipOut() that set the mode to clear is a better option, since the only caller I know of is displayTransparencyElsewhere(), which is fundamentally broken. OK sounds like a good plan as that needs to change anyway, and won't be using clipOut long ter. I'll put this change on hold and look at the canvas compositing.

(late to the party) I'm fine with fillOut(rect or path)

Created attachment 104775 [details] Patch

Comment on attachment 104775 [details] Patch Updated to remove hacky change to clipOut that is no longer needed (html canvas implementation no longer uses clipOut).

Looks surprisingly simple. Stephen, Mike - any thoughts on this?

Just a note: there are some problems with the new code with dashed and dotted borders. Details can be seen in bug 58711. Dashed / dotted borders are a little worse in skia though, which seems to be due to differences in where the dashes / dots are placed. I'd like to look at that in another bug, as there are different approaches to fixing it and it is probably to as important as getting the new path in.

Comment on attachment 104775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104775&action=review > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:483 > + if (!isPathSkiaSafe(getCTM(), path)) Just FYI, this is not necessary anymore. ENSURE_VALUE_SAFETY_FOR_SKIA is not defined, since Skia doesn't blow up on large coordinates anymore (we should probably rip out all these calls at some point). > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:490 > + if (antialiased) > + platformContext()->clipPathAntiAliased(path); > + else > + platformContext()->canvas()->clipPath(path); Perhaps we should have rename PlatformContextSkia::clipPathAntiAliased() to PlatformContextSkia::clipPath(), and give it a parameter to specify antialiasing or not. Could be done in a future CL.

Leaving for Mike to take a look.

works for me

Comment on attachment 104775 [details] Patch Let's boogie, then.

Comment on attachment 104775 [details] Patch Rejecting attachment 104775 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: ast/text/international/thai-line-breaks.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE svg/custom/inline-svg-in-xhtml.xml = IMAGE tables/mozilla/bugs/bug138725.html = IMAGE tables/mozilla_expected_failures/collapsing_borders/bug41262-5.html = IMAGE tables/mozilla_expected_failures/collapsing_borders/bug41262-6.html = IMAGE Regressions: Unexpected image and text mismatch : (1) svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT Full output: http://queues.webkit.org/results/9495602

Created attachment 105292 [details] Patch

Comment on attachment 105292 [details] Patch Not sure what went wrong with the last patch. The test reported as failing fails for me with and without my change, but not when I try using git try. I did find one extra test which needs rebaselining and have added it to the expectations.

Comment on attachment 105292 [details] Patch Clearing flags on attachment: 105292 Committed r93850: <http://trac.webkit.org/changeset/93850>

All reviewed patches have been landed. Closing bug.

I've rebaselined some fast/border/* tests that looked fine, see r93869. These ones looks strange however: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fborders%2FborderRadiusDashed06.html%2Cfast%2Fborders%2FborderRadiusDotted05.html&showExpectations=true.

Are you planning on rebaselining the other tests as well? The media/ ones look like they should just be rebased. (In reply to comment #26) > I've rebaselined some fast/border/* tests that looked fine, see r93869. > > These ones looks strange however: > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fborders%2FborderRadiusDashed06.html%2Cfast%2Fborders%2FborderRadiusDotted05.html&showExpectations=true.

(In reply to comment #27) > Are you planning on rebaselining the other tests as well? The media/ ones look like they should just be rebased. > > (In reply to comment #26) > > I've rebaselined some fast/border/* tests that looked fine, see r93869. > > > > These ones looks strange however: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fborders%2FborderRadiusDashed06.html%2Cfast%2Fborders%2FborderRadiusDotted05.html&showExpectations=true. BorderRadiusDotted06 still has some issues, which are due to the problems noted on bug 58711. These problems manifest a bit differently on skia due to differences in where dashes and dots show. I was planning on addressing those problems in a separate patch. There are lots of tests that need rebaselining after this change, I was planning on doing this next week.

(In reply to comment #28) > There are lots of tests that need rebaselining after this change, I was planning on doing this next week. If you know about these failures when you check in a change, it is ideal if you add them to test-expectation.txt so that gardeners don't have to sort it all out. :) (Now, I'll also admit from experience I also know that sometimes we don't always realize when we will break things.)

> (In reply to comment #28) > > There are lots of tests that need rebaselining after this change, I was planning on doing this next week. > > If you know about these failures when you check in a change, it is ideal if you add them to test-expectation.txt so that gardeners don't have to sort it all out. :) > > (Now, I'll also admit from experience I also know that sometimes we don't always realize when we will break things.) Sorry, I thought I had updated all the expectations, and the bots all looked green. This is some of the expectations I checked in, let me know if I'm not doing it right. // Need new baselines due to border drawing now using the PATH_BASED_BORDER_RADIUS_DRAWING // code path. ..... BUGWK65583 LINUX WIN : fast/borders/borderRadiusDotted05.html = IMAGE