291026 - Allow XBL to bind to XTF elements

Status: RESOLVED FIXED

(Core Graveyard :: XTF, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

To implement XForms custom controls, it would be useful to be
able to bind XBL bindings to XTF elements. I.e. implementing the UI using
XBL, but extending the functionality using XTF.
This way we can also support class and ID attributes.
The problematic part is the scriptable interfaces.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

Like this.

The nsIXTFElement::getScriptingInterfaces is not supported if the
element is of type nsIXTFBindableElement. The XTF element's scriptable
interfaces can be used by QIing to the right interface.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

a bit faster QueryInterface()

Comment 3

[timeless]

Comment on attachment 181294 [details] [diff] [review]
v2

>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "nsIXTFElement.idl"
>+
>+interface nsIXTFBindableElementWrapper;
>+
please use:
/**
 * comments start here
 * ...
 * last comment line
 */

not:
>+// nsIXTFBindableElement can be used to add support for new interfaces to
>+// normal XML elements. The UI can be still created using CSS/XBL.
>+// XXX getScriptingInterfaces is no-op with nsIXTFBindableElement, the scriptable
>+// interfaces come from the XML element and possibly from the XBL binding.
>+// However, if nsIXTFBindableElement implements scriptable interfaces, those
>+// can be used by explicitly QIing to them.
>+
>+[scriptable, uuid(8dcc630c-9adc-4c60-9954-a004cb45e4a7)]
>+interface nsIXTFBindableElement : nsIXTFElement
>+{
/**
 * onCreated  ...
 * @param wrapper ...
 * @throws ....
 */
>+  // onCreated: Will be called before any notifications are sent to
>+  // the xtf element. Parameter 'wrapper' is a weak proxy to the
>+  // wrapping element (i.e. it can safely be addrefed by the xtf
>+  // element without creating cyclic XPCOM referencing).
>+  void onCreated(in nsIXTFBindableElementWrapper wrapper);
>+};


>+nsXTFBindableElementWrapper::nsXTFBindableElementWrapper(nsINodeInfo* aNodeInfo,
>+                                                         nsIXTFBindableElement* xtfElement)
>+    : nsXTFBindableElementWrapperBase(aNodeInfo), mXTFElement(xtfElement)
      ^ this indentation is very unusal (i'd suggest 0 space indentation, so
that 'n' is in column 2 [0 based])
>+{
>+#ifdef DEBUG
>+//  printf("nsXTFBindableElementWrapper CTOR\n");
>+#endif
>+  NS_ASSERTION(mXTFElement, "xtfElement is null");
    ^ especially given two space indentation here
>+}
>+
>+nsresult
>+nsXTFBindableElementWrapper::Init()
>+{
>+  nsXTFBindableElementWrapperBase::Init();
>+  
>+  // pass a weak wrapper (non base object ref-counted), so that
>+  // our mXTFElement can safely addref/release.
>+  nsISupports *weakWrapper=nsnull;
please store the rv from this:
>+  NS_NewXTFWeakTearoff(NS_GET_IID(nsIXTFBindableElementWrapper),
>+                       (nsIXTFBindableElementWrapper*)this,
>+                       &weakWrapper);
>+  if (!weakWrapper) {
>+    NS_ERROR("could not construct weak wrapper");
please return the real rv. and don't use ns_error. oom is perfectly legal.
>+    return NS_ERROR_FAILURE;
>+  }
>+  
>+  mXTFElement->OnCreated((nsIXTFBindableElementWrapper*)weakWrapper);
please use NS_RELEASE(...);
>+  weakWrapper->Release();
>+  
>+  return NS_OK;
>+}
>+nsXTFBindableElementWrapper::~nsXTFBindableElementWrapper()
>+{
>+  mXTFElement->OnDestroyed();
>+  mXTFElement = nsnull;
>+  
>+#ifdef DEBUG
>+//  printf("nsXTFBindableElementWrapper DTOR\n");
>+#endif
>+}
>+
>+nsresult
>+NS_NewXTFBindableElementWrapper(nsIXTFBindableElement* xtfElement,
>+                               nsINodeInfo* ni,
>+                               nsIContent** aResult)
>+{
>+  *aResult = nsnull;
>+  
>+  if (!xtfElement) {
>+    NS_ERROR("can't construct an xtf wrapper without an xtf element");
>+    return NS_ERROR_FAILURE;
NS_ERROR_INVALID_ARG
>+  }

>+  if (aIID.Equals(NS_GET_IID(nsIXTFElementWrapperPrivate))) {
>+    *aInstancePtr = NS_STATIC_CAST(nsIXTFElementWrapperPrivate*, this);
>+    NS_ADDREF_THIS();
>+    return NS_OK;
>+  }
else after return, drop the else.
>+  else if(aIID.Equals(NS_GET_IID(nsIXTFElementWrapper))) {
>+    *aInstancePtr =
>+      NS_STATIC_CAST(nsIXTFElementWrapper*, 
>+                     NS_STATIC_CAST(nsXTFBindableElementWrapperBase*, this));
>+
>+    NS_ADDREF_THIS();
>+    return NS_OK;
>+  }
else after return, drop the else.
>+  // Note, using the nsIClassInfo from nsXMLElement.
>+  else if (NS_SUCCEEDED(rv = nsXTFElementWrapperBase::QueryInterface(aIID, aInstancePtr))) {
>+    return rv;
>+  }
else after return, drop the else.
>+  else {
>+    // try to get get the interface from our wrapped element:
>+    void *innerPtr = nsnull;
>+    QueryInterfaceInner(aIID, &innerPtr);
>+
>+    if (innerPtr)
>+      return NS_NewXTFInterfaceAggregator(aIID,
>+                                          NS_STATIC_CAST(nsISupports*, innerPtr),
>+                                          NS_STATIC_CAST(nsIContent*, this),
>+                                          aInstancePtr);
>+  }
>+
>+  return NS_ERROR_NO_INTERFACE;
>+}

>Index: content/xtf/public/nsIXTFElement.idl
>===================================================================
>RCS file: /cvsroot/mozilla/content/xtf/public/nsIXTFElement.idl,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 nsIXTFElement.idl
>--- content/xtf/public/nsIXTFElement.idl	28 Jan 2005 03:49:44 -0000	1.7
>+++ content/xtf/public/nsIXTFElement.idl	20 Apr 2005 15:38:59 -0000
>@@ -62,16 +62,20 @@ interface nsIXTFElement : nsISupports
>   // Elements of type XML_VISUAL are required to implement the
>   // nsIXTFXMLVisual interface in addition to nsIXTFElement:
>   const unsigned long ELEMENT_TYPE_XML_VISUAL      = 2;
> 
>   // Elements of type XUL_VISUAL are required to implement the
>   // nsIXTFXULVisual interface in addition to nsIXTFElement:
>   const unsigned long ELEMENT_TYPE_XUL_VISUAL      = 3;
						     ^ someone took effort to
make 2 and 3 align,
>+  // Elements of type BINDABLE are required to implement the
>+  // nsIXTFBindableElement interface in addition to nsIXTFElement:
>+  const unsigned long ELEMENT_TYPE_BINDABLE      = 4;
						     ^ shouldn't you make 4
align?

You *must* bump uuid's if you change the interfaces.
> [scriptable, uuid(94c05b72-997a-4bb0-a23d-63e40c55b02c)]
> interface nsIXTFElementWrapper : nsISupports
> {  

>+
>+  // This sets the name of the class attribute.
>+  // Should be called only during ::onCreated.
/**
 * style documentation. please.
 */
>+  void setClassAttributeName(in nsIAtom name);

why not:
attribute nsIAtom classAttributeName;
?


>+nsXTFElementWrapper::SetClassAttributeName(nsIAtom* aName)
>+{
>+  // The class attribute name can be set only once
>+  if (!mClassAttributeName && aName)
>+    mClassAttributeName = aName;
return early on failure.
>+  else
>+    return NS_ERROR_FAILURE;
>+  return NS_OK;

>Index: extensions/xforms/nsXFormsControlStub.cpp\
> NS_IMETHODIMP
> nsXFormsControlStub::OnCreated(nsIXTFXMLVisualWrapper *aWrapper)
> {
this is a scriptable method. you must NS_ENSURE_ARG_POINTER(aWrapper)
>+  aWrapper->SetClassAttributeName(nsXFormsAtoms::clazz);
>+
>   aWrapper->SetNotificationMask(kStandardNotificationMask);

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

> why not: attribute nsIAtom classAttributeName; ?

Because GetClassAttributeName is defined in nsIStyledContent.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 181419 [details] [diff] [review]
v3

Bryner, would you have time to review this?
XTFB (Extensible Tag Framework Bindings ;) ) seem to be very useful for XForms.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 181419 [details] [diff] [review]
v3

Alex, I was suggested to ask you to review this.

Comment 7

[Alex Fritze]

Comment on attachment 181419 [details] [diff] [review]
v3

r=afri for the non-xforms code.

>--- /dev/null	2005-04-21 19:23:09.954531680 +0300
>+++ content/xtf/public/nsIXTFBindableElement.idl	2005-04-21 17:08:35.000000000 
>[...]
>+interface nsIXTFBindableElementWrapper;
>+
>+/**
>+ * nsIXTFBindableElement can be used to add support for new interfaces to
>+ * normal XML elements. The UI can be still created using CSS/XBL.
>+ *
>+ * @note getScriptingInterfaces is no-op with nsIXTFBindableElement, the scriptable
>+ * interfaces come from the XML element and possibly from the XBL binding.
>+ * However, if nsIXTFBindableElement implements scriptable interfaces, those
>+ * can be used by explicitly QIing to them.
>+ */
>+

Can you please expand this comment a little bit, mentioning _why_ you had to
make getScriptingInterfaces a no-op (that evidently XBL doesn't play nice with
getScriptingInterfaces).

>Index: content/xtf/public/nsIXTFElementWrapper.idl
>===================================================================
> ...
>+  /**
>+   * This sets the name of the class attribute.
>+   * Should be called only during ::onCreated.
>+   */
>+  void setClassAttributeName(in nsIAtom name);

For elements of type ELEMENT_TYPE_GENERIC_ELEMENT, the class attribute 
is not applicable. Can you please a) either add a comment to that effect or 
b) move this method into a new interface (nsIXTFStyleableWrapper) from which
nsIXTFVisualElementWrapper & nsIXTFBindableElementWrapper inherit or c) move
this method
into both nsIXTFVisualElementWrapper and nsIXTFBindableElementWrapper.

Also, please retain the comment that nsIXTFAttributeHandler can't be used to
handle the class attribute.

>Index: content/xtf/src/nsXTFElementWrapper.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xtf/src/nsXTFElementWrapper.cpp,v
>retrieving revision 1.9
>diff -u -8 -p -r1.9 nsXTFElementWrapper.cpp
>--- content/xtf/src/nsXTFElementWrapper.cpp	5 Apr 2005 23:54:33 -0000	1.9
>+++ content/xtf/src/nsXTFElementWrapper.cpp	21 Apr 2005 15:05:48 -0000
> [...]
>+nsIAtom *
>+nsXTFElementWrapper::GetClassAttributeName() const
>+{
>+  return mClassAttributeName;
>+}
>+
>+const nsAttrValue*
>+nsXTFElementWrapper::GetClasses() const
>+{
>+  const nsAttrValue* val = nsnull;
>+  nsIAtom* clazzAttr = GetClassAttributeName();
>+  if (clazzAttr) {
>+    val = mAttrsAndChildren.GetAttr(clazzAttr);
>+    // This is possibly the first time we need any classes.
>+    if (val && val->Type() == nsAttrValue::eString) {
>+      nsAutoString value;
>+      val->ToString(value);
>+      nsAttrValue newValue;
>+      newValue.ParseAtomArray(value);
>+      NS_CONST_CAST(nsAttrAndChildArray*, &mAttrsAndChildren)->
>+        SetAndTakeAttr(clazzAttr, newValue);
>+    }
>+  }
>+  return val;
>+}
>+
>+NS_IMETHODIMP_(PRBool)
>+nsXTFElementWrapper::HasClass(nsIAtom* aClass, PRBool /*aCaseSensitive*/) const
>+{
>+  const nsAttrValue* val = GetClasses();
>+  if (val) {
>+    if (val->Type() == nsAttrValue::eAtom) {
>+      return aClass == val->GetAtomValue();
>+    }
>+
>+    if (val->Type() == nsAttrValue::eAtomArray) {
>+      return val->GetAtomArrayValue()->IndexOf(aClass) >= 0;
>+    }
>+  }
>+  return PR_FALSE;
>+}
>+
>+NS_IMETHODIMP
>+nsXTFElementWrapper::SetClassAttributeName(nsIAtom* aName)
>+{
>+  // The class attribute name can be set only once
>+  if (mClassAttributeName || !aName)
>+    return NS_ERROR_FAILURE;
>+  
>+  mClassAttributeName = aName;
>+  return NS_OK;
>+}
>Index: content/xtf/src/nsXTFElementWrapper.h
>===================================================================
>[...]
>+  // nsIStyledContent
>+  virtual nsIAtom *GetClassAttributeName() const;
>+  virtual const nsAttrValue* GetClasses() const;
>+  NS_IMETHOD_(PRBool) HasClass(nsIAtom* aClass, PRBool aCaseSensitive) const;
>[...]
>+  nsCOMPtr<nsIAtom> mClassAttributeName;
> };

This code doesn't really belong into nsXTFElementWrapper.{h,cpp}, but into a
another common base class of nsXTFBindableElementWrapper and
nsXTFVisualElementWrapper.
I don't feel very strongly about it, though.

Comment 8

[timeless]

Comment on attachment 181419 [details] [diff] [review]
v3

>+interface nsIXTFBindableElementWrapper;
>+ * The UI can be still created using CSS/XBL.
?? can still be created?
what does that mean anyway?

>+ * @note getScriptingInterfaces is no-op with nsIXTFBindableElement

what does that mean? are you talking about a concrete impl?

>+interface nsIXTFBindableElement : nsIXTFElement
>+   * onCreated will be called before any notifications are sent to
before any _other_
>+   * the xtf element.

>+   * @param wrapper is a weak proxy to the wrapping element 
>+   * (i.e. it can safely be addrefed by the xtf element without creating 
>+   * cyclic XPCOM referencing).
this is awkward, perhaps:
(i.e. holding a reference to this will not create a cycle) ?

>+  void onCreated(in nsIXTFBindableElementWrapper wrapper);

>+++ content/xtf/public/nsIXTFBindableElementWrapper.idl	2005-04-19 20:21:50.000000000 +0300
>+interface nsIXTFBindableElementWrapper : nsIXTFElementWrapper
empty interface ... at least document why it's necessary

>+++ content/xtf/src/nsXTFBindableElementWrapper.cpp	2005-04-21 17:13:52.000000000 +0300
>+ * Portions created by the Initial Developer are Copyright (C) 2004

this is really from 2004?

>+class nsXTFBindableElementWrapper : public nsXTFBindableElementWrapperBase,
>+                                    public nsIXTFBindableElementWrapper
>+{
>+protected:
>+  friend nsresult
this isn't well aligned:
>+  NS_NewXTFBindableElementWrapper(nsIXTFBindableElement* xtfElement,
>+                            nsINodeInfo* ni,

>+  nsresult Init();
protected: ?
>+  NS_IMETHOD GetInterfaces(PRUint32 *count, nsIID * **array);

>+nsresult
>+nsXTFBindableElementWrapper::Init()
>+{
this can't return failure?:
>+  nsXTFBindableElementWrapperBase::Init();

spaces around =:
why not use an nsCOMPtr?
>+  nsISupports *weakWrapper=nsnull;
>+  nsresult rv = NS_NewXTFWeakTearoff(NS_GET_IID(nsIXTFBindableElementWrapper),
>+                                     (nsIXTFBindableElementWrapper*)this,
>+                                     &weakWrapper);
>+  if (!weakWrapper) {
allocations can fail, this is not an error condition. deal with it without
erroring.
>+    NS_ERROR("could not construct weak wrapper");

document that you don't care about the return from oncreated?
>+  mXTFElement->OnCreated((nsIXTFBindableElementWrapper*)weakWrapper);

this isn't well aligned:
>+NS_NewXTFBindableElementWrapper(nsIXTFBindableElement* xtfElement,
>+                               nsINodeInfo* ni,

>+  *aResult = nsnull;
>+  
what's wrong w/ NS_ENSURE_ARG_POINTER?
>+  if (!xtfElement) {
>+    NS_ERROR("can't construct an xtf wrapper without an xtf element");
>+    return NS_ERROR_INVALID_ARG;

>Index: content/xtf/src/nsXTFService.cpp
>@@ -146,16 +148,22 @@ nsXTFService::CreateElement(nsIContent**
>   switch (elementType) {
>     case nsIXTFElement::ELEMENT_TYPE_GENERIC_ELEMENT:
>       return NS_NewXTFGenericElementWrapper(elem2, aNodeInfo, aResult);
>       break;
>+    case nsIXTFElement::ELEMENT_TYPE_BINDABLE:
>+      return NS_NewXTFBindableElementWrapper(elem2, aNodeInfo, aResult);
unreachable code (yes it's the style, but that still doesn't make any sense):
>+      break;

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: SetClassAttributeName defined in nsIXTFStyledElementWrapper

Comment 10

[Alex Fritze]

Comment on attachment 183077 [details] [diff] [review]
SetClassAttributeName defined in nsIXTFStyledElementWrapper

looks good. r=afri

Comment 11

[aaronr]

I got a build break on Windows in nsXTFElementWrapper.h because of the signature
for HasClass.  I changed it to: NS_IMETHOD_(PRBool) HasClass(nsIAtom* aClass,
PRBool aCaseSensitive) const; to get it to work for me.  something to check out
before you check this in.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> I got a build break on Windows in nsXTFElementWrapper.h because of the signature
> for HasClass.  I changed it to: NS_IMETHOD_(PRBool) HasClass(nsIAtom* aClass,
> PRBool aCaseSensitive) const; to get it to work for me.  something to check out
> before you check this in.

Hmm, of course. Thanks for testing!

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: + NS_IMETHOD_(PRBool)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 183303 [details] [diff] [review]
+ NS_IMETHOD_(PRBool)

Bz, would you have time to
sr this?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Probably not for at least a week.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 17

[Mike Kaply [:mkaply]]

Comment on attachment 184274 [details] [diff] [review]
up-to-date

a=mkaply

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Checked in

Comment 19

[Allan Beaufour]

This was checked in a week ago.