[Webkit-unassigned] [Bug 20212] [XBL] Add full support for element="" attachment

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20212





------- Comment #4 from jchaffraix at webkit.org  2008-08-01 16:47 PDT -------
(In reply to comment #3)
> (From update of attachment 22552 [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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.