328149 - split base binding on two bindings

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1

We should have one binding for nsIXFormsDelegate controls (like case control) and one binding for nsIXFormsUIWidget controls.

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

"xformswidget-general" - xforms:case
"xformswidget-accessors" - xforms:group
"xformswidget-base" - rest xfroms controls

Comment 2

[aaronr]

Comment on attachment 218159 [details] [diff] [review]
patch

You said that xf:case was a xformswidget-general, but I don't see that change in this patch.  Is that on purpose?

I have some nits below that I hope will make the bindings a little clearer for someone coming in here that isn't that familiar with our setup, yet.


>+  <!-- BASE BINDINGS
>+    The next bindings are used as a base for xforms controls implementations.
>+  -->
>+  
>+  <!-- Base binding provides common interface for all xforsm controls-->
>+  <binding id="xformswidget-general">
>+    <implementation>

nit: spelling... s/xforsm/xforms

>     <!-- interface -->
>       <property name="XFORMS_NS" readonly="true">
>         <getter>
>@@ -98,22 +73,27 @@
>         </getter>
>       </property>
> 
>-      <property name="accessors" readonly="true">
>+      <!--
>+        Return object to operate with underliying controls. Each xforms widget
>+        defines interface what should implement the object.
>+      -->
>+      <property name="control" readonly="true">
>         <getter>

nit: this comment always lost me.  Since we are already in here, let's try to tweak it a little.  How about, 'Return the native control widget that is the visual representation of this xforms control'?

>@@ -141,27 +121,36 @@
>         </body>
>       </method>
> 
>-      <!--
>-        Return object to operate with underliying controls. Each xforms widget
>-        defines interface what should implement the object.
>-      -->
>-      <property name="control" readonly="true">
>+    <!-- private -->
>+      <destructor>
>+        this._control = null;
>+      </destructor>
>+
>+      <field name="_control">null</field>
>+    </implementation>
>+  </binding>
>+
>+
>+  <!-- Base binding for xforms controls bounded to instance data -->

nit: spelling...s/bounded/bound.  Also, this binding isn't just for controls already bound to instance data (which is kinda what the comment sounds like), but rather this binding is for controls that are ABLE to bind to instance data.

Comment 3

[aaronr]

Comment on attachment 218159 [details] [diff] [review]
patch

I guess with those, r=me.  I don't really like the split too much, but I couldn't come up with anything better so I'll r+.  Seems like we have 3 kinds of bindings that we use:

1) element with anonymous content
2) element with anonymous content able to bind to an instance node
3) element with anonymous content able to bind to an instance node and the anonymous content needs to be able to take direction from the c++ implementation of the control.

But we really don't have anything in place that fits group or repeat well (no anonymous content that the control needs to access, but bound to instance data).  I guess that they can just stub out getControlElement to keep it from throwing an error.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch2

(In reply to comment #2)
> (From update of attachment 218159 [details] [diff] [review] [edit])
> You said that xf:case was a xformswidget-general, but I don't see that change
> in this patch.  Is that on purpose?

Right, there aren't changes in the patch. xf:case should be reorganized a little. I want to do it in the bug 328148.

> nit: spelling... s/xforsm/xforms
Fixed.

> nit: this comment always lost me.  Since we are already in here, let's try to
> tweak it a little.  How about, 'Return the native control widget that is the
> visual representation of this xforms control'?

It's not right comment too because it's not widget it's object with access to native control widget. I changed the comment. Please looks at it.

> nit: spelling...s/bounded/bound.  Also, this binding isn't just for controls
> already bound to instance data (which is kinda what the comment sounds like),
> but rather this binding is for controls that are ABLE to bind to instance data.

Agree, fixed.

(In reply to comment #3)
> (From update of attachment 218159 [details] [diff] [review] [edit])
> I guess with those, r=me.  I don't really like the split too much, but I
> couldn't come up with anything better so I'll r+.  Seems like we have 3 kinds
> of bindings that we use:
> 
> 1) element with anonymous content
> 2) element with anonymous content able to bind to an instance node
> 3) element with anonymous content able to bind to an instance node and the
> anonymous content needs to be able to take direction from the c++
> implementation of the control.
> 
> But we really don't have anything in place that fits group or repeat well (no
> anonymous content that the control needs to access, but bound to instance
> data).  I guess that they can just stub out getControlElement to keep it from
> throwing an error.
> 

I don't think it's a bad that we provide methods for anonymous content for all controls (including xf:group) because XBL binding always supposes that it can have content. Though I agree getControlElement() shouldn't throw exception.

I ask for review again since I changed some comments.

Comment 5

[aaronr]

Comment on attachment 218251 [details] [diff] [review]
patch2

I like the new comments.  Thanks!

Comment 6

[alexander :surkov (:asurkov)]

Attachment: updated

Comment 7

[Allan Beaufour]

Fixed on trunk