[webkit-reviews] review denied: [Bug 20212] [XBL] Add full support for element="" attachment : [Attachment 22552] First attempt: convert "element" into a binding stylesheet that is added to the bound document's

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 1 15:26:52 PDT 2008

Eric Seidel <eric at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 20212: [XBL] Add full support for element="" attachment

Attachment 22552: First attempt: convert "element" into a binding stylesheet
that is added to the bound document's

------- Additional Comments from Eric Seidel <eric at webkit.org>
 49	    String currentAttr = getAttribute(XBLNames::elementAttr);

Just be:
attr->value(); ?

+	 Document* boundDocument = document()->isXBLDocument() ? 0 :
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.

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.

More information about the webkit-reviews mailing list