267300 - Implementation of the output control

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

Comment 1

[Allan Beaufour]

Attachment: Output element implementation

Here's a go at it, pretty simple stuff.

I've added a |nsAString& aBindingAttr| parameter to
nsXFormsUtils::EvaluateNodeBinding() as <output> has a |value| attribute. This
feature also needed for the Repeat Module
(https://bugzilla.mozilla.org/show_bug.cgi?id=264329).

Comment 2

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

'value' and 'ref' attributes are not synonyms.
'ref' is used only for binding expression, with 'value' you can do more.

There shouldn't be need for copy-pasting Refresh() method. We could have a helper
method(in XFormsUtils?), which would implement the common functionality of 
Refresh().

Comment 3

[Allan Beaufour]

Regarding 'value' and 'ref', I am also handling them differently. The only 
difference is AFAIK that 'value' is not limited to returning a node, but also a 
string. I handle that. 
 
As for the copy'n'paste of Refresh()... again, it's not an exact copy'n'paste, 
there are subtle differences. But yes we have common code there. Common 
handling of that will come, when my MDG code progresses. 

Comment 4

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

hmm, sorry. I just reread the code and noticed the difference between 'ref' and
'value'

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 164276 [details] [diff] [review]
Output element implementation

>--- xforms/nsXFormsOutputElement.cpp	1970-01-01 01:00:00.000000000 +0100
>+++ xforms.output/nsXFormsOutputElement.cpp	2004-11-02 14:13:28.257865976 +0100
>+static const nsIID sScriptingIIDs[] = {
>+  NS_IDOMELEMENT_IID,
>+  NS_IDOM3NODE_IID
>+};

Why no NS_IDOMEVENTTARGET_IID ?

>+  // TODO use span instead.
>+  rv = domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML),
>+                               NS_LITERAL_STRING("span"),
>+                               getter_AddRefs(mHTMLElement));

What's the comment above this mean? You _are_ using span.

>+NS_IMETHODIMP
>+nsXFormsOutputElement::ParentChanged(nsIDOMElement *aNewParent)
>+{
>+  // We need to re-evaluate our instance data binding when our parent
>+  // changes, since xmlns declarations in effect could have changed.
>+  if (aNewParent) Refresh();

Put this on two lines please.

>+NS_IMETHODIMP
>+nsXFormsOutputElement::Refresh()
>+{
...
>+  if (model) {
>+    model->AddFormControl(this);
>+    nsAutoString text;
>+
>+    printf("We've got a model\n");

This will need to be removed before checking in.

>+    
>+    if (hasRef) {      
>+      nsCOMPtr<nsIDOMNode> resultNode;
>+      if (result)
>+        result->GetSingleNodeValue(getter_AddRefs(resultNode));
>+
>+      if (resultNode) {
>+        PRUint16 nodeType = 0;
>+        resultNode->GetNodeType(&nodeType);
>+      
>+
>+        switch (nodeType) {
>+          case nsIDOMNode::TEXT_NODE:
>+          case nsIDOMNode::ATTRIBUTE_NODE:
>+            resultNode->GetNodeValue(text);
>+            break;
>+          case nsIDOMNode::ELEMENT_NODE:
>+          {
>+            nsCOMPtr<nsIDOMNode> firstChild;
>+            resultNode->GetFirstChild(getter_AddRefs(firstChild));
>+            if (firstChild)
>+              firstChild->GetNodeValue(text);
>+            break;
>+          }
>+          default:
>+            NS_ERROR("form control references invalid node type in instance data");
>+        }
>+      }

I think we'll probably want to factor this section out into a utility function,
so we don't duplicate it between input and output.

>+      
>+    } else if (result) {
>+      printf("We've got a result\n");

Same as previous comment about printf.

>--- xforms/nsXFormsUtils.cpp	2004-11-01 14:00:44.000000000 +0100
>+++ xforms.output/nsXFormsUtils.cpp	2004-11-02 14:26:14.014453280 +0100
>@@ -243,41 +244,44 @@ nsXFormsUtils::EvaluateNodeset(nsIDOMEle
> 
>   nsCOMPtr<nsIDOMNode> contextNode = FindBindContext(bindElement, model);
>   if (!contextNode)
>     return nsnull;
> 
>   return EvaluateXPath(nodeset, contextNode, bindElement, aResultType);
> }
> 
>+const nsAutoString nsXFormsUtils::sDefaultBindingAttr(NS_LITERAL_STRING("ref"));
>+

This is going to be problematic, I think.  The problem is when strings are at
the global scope, it's possible for their constructors to run before XPCOM
initialization (in a static build).  When that happens, they can fail to
allocate memory, if I remember corectly.  So I'd suggest one of these
alternatives:

1. Just have the callers pass NS_LITERAL_STRING("ref") if that's what they
want.
2. Change the parameter to a const PRUnichar* or const char*.

Option 1 is better IMO.

Looks ok otherwise, but please post a new patch with those issues fixed or
comments saying why the code here is correct.  Thanks.

Comment 6

[Allan Beaufour]

(In reply to comment #5) 
> Why no NS_IDOMEVENTTARGET_IID ? 
 
My bad, I'll fix that. 
 
> >+  // TODO use span instead. 
> >+  rv = domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML), 
> >+                               NS_LITERAL_STRING("span"), 
> >+                               getter_AddRefs(mHTMLElement)); 
>  
> What's the comment above this mean? You _are_ using span. 
 
Argh!! I thought I had looked through the patch. Aparently I haven't. :( Sorry 
for that! The printf's should also not have been there. 
 
> >+NS_IMETHODIMP 
> >+nsXFormsOutputElement::ParentChanged(nsIDOMElement *aNewParent) 
> >+{ 
> >+  // We need to re-evaluate our instance data binding when our parent 
> >+  // changes, since xmlns declarations in effect could have changed. 
> >+  if (aNewParent) Refresh(); 
>  
> Put this on two lines please. 
 
okie. 
 
> I think we'll probably want to factor this section out into a utility 
function, 
> so we don't duplicate it between input and output. 
 
I will have to look into it with regards to the MDG code, so I thought we could 
look at it then? 
 
> >+const nsAutoString 
nsXFormsUtils::sDefaultBindingAttr(NS_LITERAL_STRING("ref")); 
> >+ 
>  
> This is going to be problematic, I think.  The problem is when strings are at 
> the global scope, it's possible for their constructors to run before XPCOM 
> initialization (in a static build).  When that happens, they can fail to 
> allocate memory, if I remember corectly. 
 
I wasn't aware of that. 
 
> So I'd suggest one of these 
> alternatives: 
>  
> 1. Just have the callers pass NS_LITERAL_STRING("ref") if that's what they 
> want. 
> 2. Change the parameter to a const PRUnichar* or const char*. 
>  
> Option 1 is better IMO. 
 
Ok. The above was made so we did not have to pass "ref" as a parameter. I also 
vote for 1 then. 
 
> Looks ok otherwise, but please post a new patch with those issues fixed or 
> comments saying why the code here is correct.  Thanks. 
 
Check. I'll do it tomorrow morning. 

Comment 7

[Allan Beaufour]

Attachment: Fixed review comments

Ok, here's a new go. Changed:
* Removed my debug statements, old comments (*doh*), etc.
* Included NS_IDOMEVENTTARGET_IID in sScriptingIIDs
* Removed default argument for nsXFormsUtils::EvaluateNodeset(), and inserted
NS_LITERAL_STRING("ref") the apropriate places.
* Factored out common code for nsXFormsInputElement::Refresh() and
nsXFormsOutputElement::Refresh() into nsXFormsUtils::GetTextValue().

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 164369 [details] [diff] [review]
Fixed review comments

Looks ok.  I think we're all colliding on wanting a reusable function to get
the text value of a node (see bug 267612), but I'm not going to hold this up
for that patch.

Comment 9

[Brian Ryner (not reading)]

I merged this with my implementation of nsXFormsUtils::GetNodeValue and checked
it in.