Bug 20212 – [XBL] Add full support for element="" attachment

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

[Julien Chaffraix]

Now that element <binding> attribute is supported (see bug20171), we need to add its handling.

[Julien Chaffraix]

Created attachment 22552 [details]
First attempt: convert "element" into a binding stylesheet that is added to the bound document's

[Julien Chaffraix]

There is a flaw in the patch: we modify the Document's style sheets (m_stylesheets) which are exported by styleSheet() to JavaScript.

This could be solved by adding an optional parameter to CSSStyleSelector constructor or querying the binding manager in the CSSStyleSelector constructor (we have the bound document) and adding the binding's sheets.

Leaving the patch r? to have comments on the global approach before correcting this.

[Eric Seidel (no email)]

Comment on attachment 22552 [details]
First attempt: convert "element" into a binding stylesheet that is added to the bound document's

Shouldn't:
 49         String currentAttr = getAttribute(XBLNames::elementAttr);

Just be:
attr->value(); ?

+        Document* boundDocument = document()->isXBLDocument() ? 0 : document();
is confusing to me.  What would we do in the XBLDocument case?

Um... we'll crash here:
+        // Update the styleSelector so that bindings are updated.
+        // FIXME: is it always required?
+        boundDocument->updateStyleSelector();

This patch crashes for inline XBL.  Can't land it that way.

Argument names are not neede here:
+        XBLBindingElement(const WebCore::QualifiedName& qName, WebCore::Document* doc); 

The fist one is not needed here:
+        virtual void attributeChanged(Attribute* attr, bool preserveDecls);

This should use early return, or early ASSERT:
+void XBLBindingManager::removeBindingSheet(Document* document, PassRefPtr<CSSStyleSheet> sheet)
+{
+    StyleSheetVector* documentSheets = m_bindingSheets.get(document);
+
+    if (documentSheets) {
+        for (StyleSheetVector::iterator it = documentSheets->begin(); it != documentSheets->end(); ++it)
+            if (*it == sheet) {
+                documentSheets->remove(it - documentSheets->begin());
+                return;
+            }
+    }
+    // We should have the sheet in our binding sheets.
+    ASSERT_NOT_REACHED();

is it OK to call removeBindingSheet after the document has already been removed from the sheets list?  Part of the code seems to assume it is, part doesn't...

You need to include a test case which tests the inline xbl case.

[Julien Chaffraix]

(In reply to comment #3)
> (From update of attachment 22552 [details] [edit])
> Shouldn't:
>  49         String currentAttr = getAttribute(XBLNames::elementAttr);
> 
> Just be:
> attr->value(); ?

Nope: attr may be hold the old value if the attribute has really changed. That's why we check it against the real value to determine if it is a change or an addition.

> 
> +        Document* boundDocument = document()->isXBLDocument() ? 0 :
> document();
> is confusing to me.  What would we do in the XBLDocument case?

If our Document is an XBLDocument we need to get the proper bound document but that logic is not implemented yet (the crash was intended as inline binding is the common case that would have appeared pretty soon). I will remove the isXBLDocument() and just take the current document (leaving a FIXME).

> Argument names are not neede here:
> +        XBLBindingElement(const WebCore::QualifiedName& qName,
> WebCore::Document* doc); 
> 
> The fist one is not needed here:
> +        virtual void attributeChanged(Attribute* attr, bool preserveDecls);
> 
Will fix this.

> This should use early return, or early ASSERT:
> +void XBLBindingManager::removeBindingSheet(Document* document,
> PassRefPtr<CSSStyleSheet> sheet)
> +{
> +    StyleSheetVector* documentSheets = m_bindingSheets.get(document);
> +
> +    if (documentSheets) {
> +        for (StyleSheetVector::iterator it = documentSheets->begin(); it !=
> documentSheets->end(); ++it)
> +            if (*it == sheet) {
> +                documentSheets->remove(it - documentSheets->begin());
> +                return;
> +            }
> +    }
> +    // We should have the sheet in our binding sheets.
> +    ASSERT_NOT_REACHED();
> 
> is it OK to call removeBindingSheet after the document has already been removed
> from the sheets list?  Part of the code seems to assume it is, part doesn't...

No; we should have the sheet in the document's list or something went wrong (but we do not want to crash).

> You need to include a test case which tests the inline xbl case.

Will be added.

[Julien Chaffraix]

> > This should use early return, or early ASSERT:
> > +void XBLBindingManager::removeBindingSheet(Document* document,
> > PassRefPtr<CSSStyleSheet> sheet)
> > +{
> > +    StyleSheetVector* documentSheets = m_bindingSheets.get(document);
> > +
> > +    if (documentSheets) {
> > +        for (StyleSheetVector::iterator it = documentSheets->begin(); it !=
> > documentSheets->end(); ++it)
> > +            if (*it == sheet) {
> > +                documentSheets->remove(it - documentSheets->begin());
> > +                return;
> > +            }
> > +    }
> > +    // We should have the sheet in our binding sheets.
> > +    ASSERT_NOT_REACHED();
> > 
> > is it OK to call removeBindingSheet after the document has already been removed
> > from the sheets list?  Part of the code seems to assume it is, part doesn't...
> 
> No; we should have the sheet in the document's list or something went wrong
> (but we do not want to crash).
> 

I have not really replied to your comment: we do not want to crash, that's why there is a check on documentSheets. However we must have removed the binding sheet so that's why there is an ASSERT_NOT_REACHED. I will modify the comments to show that.

[Julien Chaffraix]

Created attachment 22618 [details]
Second attempt: updated with Eric's comment + removed issue I had spotted (added a test case for that)

[Eric Seidel (no email)]

Comment on attachment 22618 [details]
Second attempt: updated with Eric's comment + removed issue I had spotted (added a test case for that)

Looks OK.  Please add a test case which fails due to this FIXME:
 53         // FIXME: If we have an XBLDocument, we need to get the bound document.
 54         Document* boundDocument = document();

when you land.

[Julien Chaffraix]

(In reply to comment #7)
> (From update of attachment 22618 [details] [edit])
> Looks OK.  Please add a test case which fails due to this FIXME:
>  53         // FIXME: If we have an XBLDocument, we need to get the bound
> document.
>  54         Document* boundDocument = document();
> 
> when you land.

I have tried to produce this test case. However we cannot load an external XBL document for the moment  (because we do not support DocumentXBL interface, <?xbl?> processing instruction or really load bindinds when they are attached). As it is a fairly common situation (which will arise once the previous support is added), I have committed the patch as-is in r35640.