The canvas spec requires that these points are considered inside. We fail 2 tests in test_canvas.html because of this. This was regressed by the change in bug 806780. I don't think we can just change the behaviour of the skia method, since it's implemented on SkRect and would probably break other things. We could add a second method to skia (SkPath::containsInclusive or something?), backout bug 806780 (which would be a shame, since the old code was horrible), or pad the input rect. Thoughts?

(In reply to George Wright (:gw280) from comment #1) > http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/skia/ > SkiaUtils.cpp#L121 We switched from this style of hit testing.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #2) > We switched from this style of hit testing. Was this a correct change? The new behaviour doesn't match the canvas spec at least.

(In reply to Matt Woodrow (:mattwoodrow) from comment #3) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #2) > > We switched from this style of hit testing. > > Was this a correct change? The new behaviour doesn't match the canvas spec > at least. It would be correct from the point of view of using the Skia api properly, but not correct from the point of view of the canvas spec. I have no problem with us reverting to the webkit behaviour but we should try to get this fixed upstream.

SkPath now has a native contais(x, y) method. It should be much faster than creating a region. I think it also has the min <= x < max rule, but if that is all that needs tweaking, I think we can make a version that is min <= x <= max.

(In reply to reed from comment #5) > SkPath now has a native contais(x, y) method. It should be much faster than > creating a region. I think it also has the min <= x < max rule, but if that > is all that needs tweaking, I think we can make a version that is min <= x > <= max. Great, it seems to me that we would need that tweak. Would this change break much in other parts of Skia/Chrome (and thus would it be best to use Matt's suggestion of adding a containsInclusive(x,y) instead of changing the behaviour of contains)?

I would definitely make this new bahavior optional (either via a parameter or a different entry-point).

Great, thanks Mike. Lets back out the change to use SkPath::contains() for now, since it's stopping us from enabling skia. We should get a bug filed on the skia bug tracker for adding the new functionality.

Let's leave this open for now so we can track a real fix. For now, only a temporary solution is in place.

Doesn't seem like anybody is tracking anything here, so if you still care about a real fix open a new bug?