[webkit-reviews] review denied: [Bug 20263] [XBL] Add loading code for XBLBinding : [Attachment 22619] Proposed fix: add loading code

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 20263: [XBL] Add loading code for XBLBinding
https://bugs.webkit.org/show_bug.cgi?id=20263

Attachment 22619: Proposed fix: add loading code
https://bugs.webkit.org/attachment.cgi?id=22619&action=edit

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