Bug 20263 – [XBL] Add loading code for XBLBinding

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Julien Chaffraix]

Currently an XBLBinding will not load its binding. This is required to generate the shadow tree as we must hold the binding element to do so.

[Julien Chaffraix]

Created attachment 22619 [details]
Proposed fix: add loading code

[Eric Seidel (no email)]

Comment on attachment 22619 [details]
Proposed fix: add loading code

What does "hasIndex" mean?  Maybe you mean "has fragment"?  #foo is a "fragment url", maybe the XBL spec calls it an index?

I wonder if it's safe to just grab weak pointers to other documents:
+    Document* bindingDocument;
+    if (documentURL == boundElement->document()->url())
+        // This is the case of an <xbl> element inside another document.
+        // The binding document is the bound document.
+        bindingDocument = boundElement->document();

Does this automatically ref?
+        m_cachedDocument = boundElement->document()->docLoader()->requestXBLDocument(documentURL);
if so, when is the XBLBinding releasing the CachedDocument?

How does the .get() make sense here?

bindingDocument = m_cachedDocument->document().get();

does document() return a const RefPtr&?

I'm not sure this check is really necessary:
if (!m_id.isNull()) {

the hash-lookup of a NULL pointer won't be many more cycles :)

Why is it safe for XBLBinding to have a weak pointer to an XBLBindingElement?
+        Element* bindingElement = bindingDocument->getElementById(m_id);
+        if (bindingElement->isXBLElement() && static_cast<XBLElement*>(bindingElement)->isXBLBindingElement())
+            m_bindingElement = static_cast<XBLBindingElement*>(bindingElement);

What if that binding element was removed from the document?

This could probably have a slightly more descriptive name:
+        String m_id;

r- for the leak of the CachedDocument and possible crash due to weak pointers.

[Julien Chaffraix]

(In reply to comment #2)
> (From update of attachment 22619 [details] [edit])
> What does "hasIndex" mean?  Maybe you mean "has fragment"?  #foo is a "fragment
> url", maybe the XBL spec calls it an index?

It was the name used in the legacy code. The spec does not give any specific term so we will go for fragment url.

> I wonder if it's safe to just grab weak pointers to other documents:
> +    Document* bindingDocument;
> +    if (documentURL == boundElement->document()->url())
> +        // This is the case of an <xbl> element inside another document.
> +        // The binding document is the bound document.
> +        bindingDocument = boundElement->document();

I think it is ok here:
- if it is the bound document, then we hold it with the bound element.
- if it is not, we hold a pointer to the associated CachedResource.

> Does this automatically ref?
> +        m_cachedDocument =
> boundElement->document()->docLoader()->requestXBLDocument(documentURL);
> if so, when is the XBLBinding releasing the CachedDocument?

The Cache code does not use refcounting. Instead they just use raw pointers and inherits from CachedResourceClient. I will change that.

> How does the .get() make sense here?
> 
> bindingDocument = m_cachedDocument->document().get();
> 
> does document() return a const RefPtr&?

It returns a PassRefPtr. I have thought about that and we need to have a strong reference to the binding document in case it was loaded because of a binding so the get() will be removed.

> I'm not sure this check is really necessary:
> if (!m_id.isNull()) {
> 
> the hash-lookup of a NULL pointer won't be many more cycles :)


Indeed.

> Why is it safe for XBLBinding to have a weak pointer to an XBLBindingElement?
> +        Element* bindingElement = bindingDocument->getElementById(m_id);
> +        if (bindingElement->isXBLElement() &&
> static_cast<XBLElement*>(bindingElement)->isXBLBindingElement())
> +            m_bindingElement =
> static_cast<XBLBindingElement*>(bindingElement);
> 
> What if that binding element was removed from the document?

I will change to a RefPtr as it is only safe for the bound element (which has to clean its bindings when deleted).

> This could probably have a slightly more descriptive name:
> +        String m_id;

Will change to m_bindingID.

[Julien Chaffraix]

Created attachment 22732 [details]
Updated version: Correct member ownership + Eric's comment

[Julien Chaffraix]

Created attachment 22758 [details]
Same as previously with a correction for 2 small mistakes

[Eric Seidel (no email)]

Comment on attachment 22758 [details]
Same as previously with a correction for 2 small mistakes

+    int hasFragment = uri.find('#');

should probably be called fragmentOffset

This should be < 0:
(hasFragment != -1)


Sounds like we need a test case!
+    // FIXME: if no ID was given, the spec does not say what to do so for the moment
+    // do nothing for now.

There is a deleteAllValues() call you should use instead:
+        for (Vector<XBLBinding*>::iterator it = elementBindings->begin(); it != elementBindings->end(); ++it)
+            delete *it;

Otherwise looks OK to land with the above mentioned test case.   I don't need to see this again, but you should make the above fixes/additional test case.

[Julien Chaffraix]

Landed in r35819 with the requested changes.