[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]

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
https://bugs.webkit.org/show_bug.cgi?id=20212

Attachment 22552: First attempt: convert "element" into a binding stylesheet
that is added to the bound document's
https://bugs.webkit.org/attachment.cgi?id=22552&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.