[webkit-reviews] review denied: [Bug 15451] SVGStyledElement must unregister itself from Resources on detach : [Attachment 16620] Fix attempt #2, a single hashtable

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 15451: SVGStyledElement must unregister itself from Resources on detach
http://bugs.webkit.org/show_bug.cgi?id=15451

Attachment 16620: Fix attempt #2, a single hashtable
http://bugs.webkit.org/attachment.cgi?id=16620&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
+    {
+	 for (int i = 0; i < _ResourceTypeCount; i++)
+	     resources[i] = 0;
+    }

could also just be a call to memset... but that's fine.

Removing the 0 from +	     ClipperResourceType,
 could make _ResourceTypeCount be not the right number on some platforms (but
maybe K&R says otherwise..)

I thinik this is the only place I've seen _ used in an enum before.  Mayabe it
should be LastResourceType like is used by CSS values or ResourceTypeMax

Why the var name "item" (twice).  That seems like a hold-over from the KCanvas
days.

I think that addClient's lookup should just be a get:
+    HashMap<SVGStyledElement*, ResourceSet*>::iterator associatedResource =
clientMap().find(item);

Then you don't have to compare against the end iterator, you just check if
(target) and if !target then you set it to a new ResourceSet.  seems cleaner.

SVGResource doesn't know how to remove itself from ResourceSet when it dies. 
addClient's line:

+    if (SVGResource* oldResource = target->resources[type])
+	 oldResource->m_clients.remove(item);

will crash w/o fixing that.