Bug 28808 – [Skia] Fix flaky layout test svg/dynamic-updates/SVGClipPathElement-dom-clipPathUnits-attr.html [DEBUG]

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Roland Steiner]

Note: DEBUG build

It seems the layout test fails in Skia if a render event occurs before the clipPathUnit attribute has been reset to "userSpaceOnUse" from the initally set "objectBoundingBox". This causes the 300px coordinates in the clipping path to be interpreted as 300*100% of the bounding box size, resulting in values that are too large for Skia's fixed point data types. This causes an ASSERT during the value conversion.

[Roland Steiner]

Created attachment 38733 [details]
patch: check skia safety of path on conversion to local coordinates


The patch adds an explicit check for the validity of the path when it is transformed into local coordinates. 

This was the most suitable place I could find that didn't cause an inordinate amount of refactoring. Also, it seems pertinent and may also catch other cases that are not limited to clipping paths.

[Roland Steiner]

(In reply to comment #1)

additional remark to the patch: I tested it only under Windows so far, but the issue should be OS-independent.

[Eric Seidel (no email)]

Comment on attachment 38733 [details]
patch: check skia safety of path on conversion to local coordinates

Looks fine.

+extern bool isPathSkiaSafe(const SkMatrix& transform, const SkPath& path);
should not have names for the arguments since they don't add anything.

Not sure why you're moving the inline functions around... I guess so you can export a function?

If we're going to export the function we should just add it to a header no?  Or to some utility file?

This looks OK.  Waiting for your response or new patch before landing.

[Roland Steiner]

(In reply to comment #3)
> (From update of attachment 38733 [details])
> [...]
> Not sure why you're moving the inline functions around... I guess so you can
> export a function?

Yes, I needed to move the relevant functions out of the anonymous namespace, but wanted to retain it for the generically named 'fastMod' and 'square' functions.

> If we're going to export the function we should just add it to a header no?  Or
> to some utility file?

Well, since the comment on those functions was that they should go away sooner or later I didn't want to give them additional exposure. (I don't know what would happen to this patch at that point, though.) Also, a short'n'simple 'extern' was sufficient for my purposes so I left it at that.

But if you think it'd still be worthwhile I could certainly add them to some header file.

[Eric Seidel (no email)]

Comment on attachment 38733 [details]
patch: check skia safety of path on conversion to local coordinates

The Skia safetybelt is lame, and should die, yes.  :)  this is OK for now.

[Eric Seidel (no email)]

Comment on attachment 38733 [details]
patch: check skia safety of path on conversion to local coordinates

Clearing flags on attachment: 38733

Committed r47925: <http://trac.webkit.org/changeset/47925>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.