Bug 10740 – Need to remove KCanvas enums from SVG code

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Need to remove KCanvas enums from SVG code

KCanvas used to be its own separate library.  So it had it's own duplicate Enumerations.  KCanvas is on its way out now.  It's time to kill all of the duplicate enums and the scary c-style casts which they caused:

one of many, many examples:
m_filterEffect->setXChannelSelector((KCChannelSelectorType)(xChannelSelectorBaseValue()));

[Eric Seidel (no email)]

A more egregious example:
+    m_filterEffect->setType((KCTurbulanceType)(typeBaseValue() - 1));

[Rob Buis]

Created attachment 10581 [details]
First attempt

I checked, but the filter enums are "unique", so it should probably not be touched. I
tried to kill KCGradientSpreadMethod, but it seems hard due to dependencies. Maybe it
can wait until we can redesign the paint servers...
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10581 [details]
First attempt

This wasn't really what I meant with this bug.  More I meant that the KCanvas classes should all use the DOM enums instead of having their own duplicate copies with the KC prefix.  Especially since KCanvas is slated for the chopping block.

SVGSVGElement *svg = static_cast<SVGSVGElement*>(node());

is not at all safe.  RenderSVGContainers can also represent SVGGElements as well as SVGSVGElements.

[Rob Buis]

Created attachment 10602 [details]
Improved patch

This one should be better. It is actually quite hard to reuse the enums, since there
are some dependency problems. The patch is not complete, there are some non-filter enums
that can be reused, but I wanted to put it up early to check that this is the right direction.
Cheers,

Rob.

[Rob Buis]

Created attachment 10623 [details]
Improved patch

This patch should be more complete, I think I have all double enums removed now.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10623 [details]
Improved patch

We're going to need to think about these changes:
-        virtual KCanvasFEBlend *filterEffect() const;
+        virtual KCanvasFilterEffect* filterEffect() const;

we may need to rip the Enums out of the classes to avoid them.

We've talked about dumbing down KCanvasFilterEffect classes into simple structs, that could be passed into some other class to have a PlatformFilterEffect* returned.  That way there is no need for as much virtual dispatch.

This is a good patch, but it highlights some issues with our previous thinking.  It's kinda ugly to have to list the entire ClassName::ENUM_VALUE every time, would be nicer if the enums were separated out into the root level namespace.  We'd have to tweak the autogen system to support such enums however.

The problems with the duplicate  KCanvas enums that this patch was originally trying to solve, were fragility and unnecessary code bloat.  This patch sorta solves those problems but points out a raft of others in KCanvas.  One of them being the duplicity of data.  Some of the kcanvas structures don't need to be classes at all (like the filter effects) and some of them might not need to store as much information and could instead grab it off of the element() pointer.

As much as I'd like to see these enums go away, I'm tempted to say we should just shelve this patch for now and concentrate on fixing other related issues in the tree first.  Such as the bad int vs. enum usage in the SVG DOM for animated value storage, or the logic flow for generating gradient and pattern resources from the DOM.

I'm really undecided.  We could just land this patch as is, it would fix certain enum awkwardness, but introduce other awkwardness of its own (such as the int to enum casts between DOM and renderer -- the whole point of sharing the same enums between them is to avoid the need to cast -- and virtual methods which don't override the return type due to header include problems).

Unless you have strong feelings otherwise, I think we should shelve this patch until we fix the other mentioned problems in the DOM/Renderer which are forcing this patch to regress code cleanliness.

[Rob Buis]

(In reply to comment #6)
> Unless you have strong feelings otherwise, I think we should shelve this patch
> until we fix the other mentioned problems in the DOM/Renderer which are forcing
> this patch to regress code cleanliness.

I am okay with keeping the patch around for ideas until we fix some other things first. For the record, the patch was not just copy and paste (because of the
dependency problems) :)
Cheers,

Rob.

[Eric Seidel (no email)]

The only two remaining are:

KCAlign
and
KCDashArray

Both can be removed and then this bug can be closed.

[Rob Buis]

This was fixed in r23509.