[webkit-reviews] review denied: [Bug 10740] Need to remove KCanvas enums from SVG code : [Attachment 10623] Improved patch

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has denied Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 10740: Need to remove KCanvas enums from SVG code
http://bugzilla.opendarwin.org/show_bug.cgi?id=10740

Attachment 10623: Improved patch
http://bugzilla.opendarwin.org/attachment.cgi?id=10623&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
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.