[XBL] We need the ability to manage bindings

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently we cannot manage bindings.
We need to re-enable the XBLBindingManager and add some support methods to manage the bindings. The specification provide the ElementXBL interface to control the bindings from JavaScript.

Patch will follow.

[Julien Chaffraix]

Created attachment 22119 [details]
Make XBLBindingManager global, add most of ElementXBL interface

[Eric Seidel (no email)]

Comment on attachment 22119 [details]
Make XBLBindingManager global, add most of ElementXBL interface

Why would a binding manager be global?  I think per-document was the right way to go.  Otherwise how do you load two different pages in two different tabs with same-named bindings?

[Julien Chaffraix]

(In reply to comment #2)
> (From update of attachment 22119 [details] [edit])
> Why would a binding manager be global?  I think per-document was the right way
> to go.  Otherwise how do you load two different pages in two different tabs
> with same-named bindings?
> 

It will not matter whether the binding manager is global: a binding links a bound element with a <binding> element, the name is only to fetch the resource.
We index the bindings' list with the bound element in the XBLBindingManager. This ensure that it will be different per-document.

Furthermore, having a per-document binding manager is a restriction that is not on the specification. For example, with a per-document binding manager, you will not be able to do the following (which is right):

var element = document.createElement('p');
element.attachBinding('http://example.com');
document.getElementById('foobar').appendChild(element);

[Julien Chaffraix]

Comment on attachment 22119 [details]
Make XBLBindingManager global, add most of ElementXBL interface

[Setting the review flag again]

As discussed with David Hyatt, a global manager is the right choice.
There may be a performance hit as we need to do a table lookup but we will postpone it for later.

[Eric Seidel (no email)]

Comment on attachment 22119 [details]
Make XBLBindingManager global, add most of ElementXBL interface

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.

[Eric Seidel (no email)]

Comment on attachment 22119 [details]
Make XBLBindingManager global, add most of ElementXBL interface

On second thought, I don't think this is quite right, but I think you'll have an easier time fixing it once you've done more of the actual meat.  This boiler plate stuff can be reworked later as needed.

[Julien Chaffraix]

Committed in r35324 with the requested changes (Vector<XBLBinding*> -> Vector<XBLBinding> and XBLBindingManager defined in sharedInstance). I left the FIXME about adding the same binding multiple times as the spec does not prevent us to but we may limit that later on.