Last Comment Bug 328149 - split base binding on two bindings
: split base binding on two bindings
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
:
Mentors:
Depends on:
Blocks: 327236
  Show dependency treegraph
 
Reported: 2006-02-21 21:40 PST by alexander :surkov
Modified: 2016-07-15 14:46 PDT (History)
0 users
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (5.89 KB, patch)
2006-04-12 03:02 PDT, alexander :surkov
allan: review+
aaronr: review+
Details | Diff | Splinter Review
patch2 (6.45 KB, patch)
2006-04-12 21:12 PDT, alexander :surkov
aaronr: review+
Details | Diff | Splinter Review
updated (7.27 KB, patch)
2006-04-18 01:39 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description alexander :surkov 2006-02-21 21:40:01 PST
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 2006-04-12 03:02:58 PDT
Created attachment 218159 [details] [diff] [review]
patch

"xformswidget-general" - xforms:case
"xformswidget-accessors" - xforms:group
"xformswidget-base" - rest xfroms controls
Comment 2 aaronr 2006-04-12 13:01:22 PDT
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 2006-04-12 14:36:39 PDT
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 2006-04-12 21:12:23 PDT
Created attachment 218251 [details] [diff] [review]
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 2006-04-14 14:21:02 PDT
Comment on attachment 218251 [details] [diff] [review]
patch2

I like the new comments.  Thanks!
Comment 6 alexander :surkov 2006-04-18 01:39:02 PDT
Created attachment 218796 [details] [diff] [review]
updated
Comment 7 Allan Beaufour 2006-04-18 01:45:12 PDT
Fixed on trunk

Note You need to log in before you can comment on or make changes to this bug.