[webkit-reviews] review denied: [Bug 19917] [XBL] We need the ability to manage bindings : [Attachment 22119] Make XBLBindingManager global, add most of ElementXBL interface

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 19917: [XBL] We need the ability to manage bindings
https://bugs.webkit.org/show_bug.cgi?id=19917

Attachment 22119: Make XBLBindingManager global, add most of ElementXBL
interface
https://bugs.webkit.org/attachment.cgi?id=22119&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
static XBLBindingManager* manager;
 37 
 38 XBLBindingManager* XBLBindingManager::sharedInstance()
 39 {
 40	if (!manager)
 41	    manager = new XBLBindingManager();
 42 
 43	return manager;
 44 }
 

should probably just chagne to:

sharedInstance()
{
   static XBLBindingManager manager;
   return &manager;
}

Um... just fix it:
 57	// FIXME: we can add the same binding several times.
Seems like it would be easy to walk the bindings.

Why do these XBLBindings need to be new'd?  Can'd you just use:
Vector<XBLBinding> ?

Also, as much as I hate DeprecatedValueList, I think we might want to do this
with a list structure.	I'm not sure how fast we need removes to be... but with
a vector they'll be quite slow.
I see these as quite similar to Event listeners.

Overall, I think this is fine, but I think that this should at least be:
Vector<XBLBinding>
instead of:
Vector<XBLBinding*>
for easier memory management.

You could also just use:
HashMap<String, Element*>

Why wouldn't this just be:
HashMap<Element*, Vector<std:pair<String, Element*> > >

Yeah, this just seems a bit over-designed, when all you need is a simple hash
to map from elements to a list of uris.  XBLBinding could/should probably just
be a private class on XBLbinding manager.

we'll talk about this over irc.