283257 - enable native objects to provide an XBL binding implementation

Status: RESOLVED WONTFIX

(Core :: XBL, defect)

Description

[Brian Ryner (not reading)]

This bug is about adding support to XBL for creating a "delegate" object to
provide the implementation for a binding.  This would allow you, for example, to
create a binding whose implementation is defined in C++, or as a standalone JS
object (which is somewhat better than what we currently have, in terms of
data-hiding).

I'm proposing a syntax like this:

<binding id="foo" contractid="@mozilla.org/foo;1">
  <implementation>
    <constructor/>
    <destructor/>
  </implementation>
  <handler event="click"/>
</binding>

As you can see, the actual implementation and handlers are empty; they just
serve to declare what notifications are expected.  The object instantiated by
the given contractid is automatically set up as a listener for the constructor
and destructor calls, and for the click event.

Comment 1

[Brian Ryner (not reading)]

Attachment: first cut, not ready yet but comments welcome

Comment 2

[Hixie (not reading bugmail)]

Seems good to me. How should you handle the case where someone gives a
contractid but then also includes text in the various event handlers, etc? Does
specifying a contractid="" mean that it is _only_ a binary implementation?

Is there a way for the implementation itself to say what it implements so that
we don't have to have the <handler>s nor the <implementation>? That would make
it a lot neater and less brittle -- with the current proposal the author has to
keep the binding in sync with the code.

Comment 3

[Brian Ryner (not reading)]

(In reply to comment #2)
> Seems good to me. How should you handle the case where someone gives a
> contractid but then also includes text in the various event handlers, etc? Does
> specifying a contractid="" mean that it is _only_ a binary implementation?

We could go either way... I don't think it's too difficult to support both types
of implementations on the same binding, if people would fine it useful.  We'd
have to decide who wins in the case of a collision, of course, or whether they
chain somehow.

> 
> Is there a way for the implementation itself to say what it implements so that
> we don't have to have the <handler>s nor the <implementation>? That would make
> it a lot neater and less brittle -- with the current proposal the author has to
> keep the binding in sync with the code.

At least in the case of handlers, I believe what I have now is actually the
least work for the author.  They can simply list out the handlers in the binding
rather than having a series of addEventListener() and removeEventListener()
calls from native code.

Comment 4

[Brian Ryner (not reading)]

Attachment: revised patch

Changes from the last patch:
- moved contractid attribute up to <binding>, this makes more sense to me since
it affects the <handlers> as well as the <implementation>.
- added some security checks - only chrome bindings can instantiate native
objcets for now
- made sure event listeners are unregistered properly
- added xpt files to installers

I'm going to be attaching a small testcase I made too.

Comment 5

[Brian Ryner (not reading)]

Attachment: testcase (.tar.gz)

This test exercies the attached and detached handlers, methods and properties,
and event handlers.  To run it, you'll need to untar it and build in
content/xbl/tests, then bring up chrome://xbltest/content/xbltest.xul.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It'll take me a bit to get to this (still need to review the nodeXBL patch....)

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 176176 [details] [diff] [review]
revised patch

So, this patch doesn't exactly honor COM identity - after QI'ing to a native
interface, you can't get back to the other element interfaces. My current
thinking is that the native QI should just fall through to mBoundElement.

Comment 8

[Brian Ryner (not reading)]

Attachment: preserve COM identity

I updated the interface (here) and the testcase (coming up) to show how to
preserve COM identity.

Comment 9

[Brian Ryner (not reading)]

Attachment: testcase (.tar.gz)

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 176176 [details] [diff] [review]
revised patch

removing obsolete request

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176319 [details] [diff] [review]
preserve COM identity

>Index: content/xbl/public/nsIXBLImplementation.idl

>+  void bindingQueryInterface(in nsIIDRef uuid,
>+                             [iid_is(uuid),retval] out nsQIResult result);

What does this method have to do or not do?  In particular, what are the
assumptions about this calling QI on the boundElement?	What can (or must?) it
throw, and in what cases?  Please document....

>+  void bindingAttached(in nsIDOMElement boundElement);
>+  void bindingDetached(in nsIDOMElement boundElement);

Again, please document.  What's the element being passed in?  When will these
methods be called (in particular, how is that ordered wrt the binding
constructor and destructor)?  What happens if these methods change properties
of the element that change what binding is bound to it?

Are there any expectations about how calling QI on this interface will behave? 
If so, probably worth documenting...

>Index: content/xbl/src/nsXBLBinding.cpp

>@@ -644,11 +646,15 @@ nsXBLBinding::InstallEventHandlers()
>+        // We handle key listeners separately below, except in the case of
>+        // native implementations.

But aren't native key listeners are still ending up in the list returned by
CreateKeyHandlers() with this patch, no?  So won't we end up adding them twice,
or at least trying to?

>+nsXBLBinding::InstallNativeImplementation()
>+{
>+  // For now, only allow trusted content to instantiate a native object.
>+  PRBool isChrome;
>+  mPrototypeBinding->BindingURI()->SchemeIs("chrome", &isChrome);

This allows skin bindings to install native implementations.  That seems pretty
wrong...  I suspect you want a AllowScripts() check around this whole function.

>+  nsCOMPtr<nsIXBLImplementation> nativeImpl =
>+    mPrototypeBinding->CreateNativeImpl();
>+  SetNativeImpl(nativeImpl);

Do we really not want to propagate errors in any way here?

>@@ -753,15 +778,23 @@ nsXBLBinding::ExecuteAttachedHandler()
> nsXBLBinding::ExecuteDetachedHandler()

I think if we attach the native impl after firing the constructor, we should
detach it before firing the destructor, unless there's a good reason to have
the ordering this implements?

>@@ -779,33 +812,41 @@ nsXBLBinding::UnhookEventHandlers()

This seems to have an issue similar to InstallEventHandlers, though it's less
of a problem here (because unhooking a handler that was never hooked up is not
so bad).


> nsXBLBinding::MarkForDeath()
> {
>-  mMarkedForDeath = PR_TRUE;
>+  mNativeImplBits |= 0x2;

I'd really rather we had #defines for both 0x1 and 0x2 that actually mean
something (and possibly a define for 0x3; something like TAGGED_BITS or
something).  That would make a lot of the code in nsXBLBinding.h clearer too.

>Index: content/xbl/src/nsXBLBinding.h

>+  void SetIsStyleBinding(PRBool aIsStyle)
>+  {
>+    mNativeImplBits = (mNativeImplBits & ~0x1) | aIsStyle;
>+  }

  NS_PRECONDITION(aIsStyle == PRBool(0) || aIsStyle == PRBool(1),
		  "Bogus boolean passed");

here, please.  I realize that passing PRBools that are not 0 or 1 is no good,
but it still happens, and it's best to assert if it does.

>+  // A native object which implements the binding, specified via
>+  // <implementation contractid="...">.  Shares some bits to save space.
>+  PRUint32 mNativeImplBits; 

Not going to work on 64-bit platforms... Please use a reasonable type here.

Also, please document which exact bits are shared (if you have a #define for
that, just point to it), and document what the ownership model for the pointer
is.

>+  nsIXBLImplementation* NativeImpl()
>+  void SetNativeImpl(nsIXBLImplementation *aNativeImpl)

Is there a reason these are public?  They seem to only be called from
nsXBLBinding, so protected would make more sense...

>+    if (oldImpl != aNativeImpl) {
>+      NS_IF_RELEASE(oldImpl);

Do we ever expect to have SetNativeImpl() called when we already have a native
impl?  I don't see how that could happen given existing code, and it may make
more sense to just assert that oldImpl is null...  but if we are in fact
getting this called when oldImpl is non-null, I guess we can leave it as-is.

>Index: content/xbl/src/nsXBLContentSink.cpp

>+  nsAutoString contractid;
>+  binding->GetAttr(kNameSpaceID_None, nsXBLAtoms::contractid, contractid);

Does having a contractid place any limitations on what else can happen in the
binding?  If so, we shoul enforce it in the sink.

In general, it would be good to put some documentation for this functionality
somewhere on mozilla.org.....

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp

>+  mContractID = aContractID;

Why not pass in the UTF-16 string, and do CopyUTF16toUTF8 here?  That
guarantees that you only have to do one copy (as things stand you might end up
with two), and saves on object creation costs...

Comment 12

[Brian Ryner (not reading)]

(In reply to comment #11)
> Does having a contractid place any limitations on what else can happen in the
> binding?  If so, we shoul enforce it in the sink.

I'm not sure what you mean really, as far as limitations.

> 
> In general, it would be good to put some documentation for this functionality
> somewhere on mozilla.org.....
> 

Yes; where is an appropriate place?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what you mean really, as far as limitations.

For example, if there is a contractid specified the text in <handler> will just
be ignored, right?  Given that, it may make sense to error out of there is a
contractid and a nonempty <handler>, say.  That seems to be the only part of
normal XBL syntax overridden by having a contractid.

By the way, for the case when we can't createInstance the native impl, even if
we silently proceed internally we should log an error to the JS console.

> Yes; where is an appropriate place?

I don't actually know... if we have no existing XBL docs, perhaps ping
[e-mail] for advice?

Comment 14

[Brian Ryner (not reading)]

(In reply to comment #13)
> > I'm not sure what you mean really, as far as limitations.
> 
> For example, if there is a contractid specified the text in <handler> will just
> be ignored, right?  Given that, it may make sense to error out of there is a
> contractid and a nonempty <handler>, say.  That seems to be the only part of
> normal XBL syntax overridden by having a contractid.

Yeah, currently that's the case.  I'm not sure it's actually the best approach
though.  We could try to make it fire any non-empty script handlers in addition
to the native handlers, or perhaps _instead_ of the native handlers if the
handler is for the same event/phase (sort of like subclassing the native binding).

Comment 15

[Aaron Leventhal]

What's the status on this? We need this for xforms a11y.

Comment 16

[Boris Zbarsky [:bzbarsky]]

We're not going to make this change, since XBL is going away.