283004 - Store instance node type as property instead of attribute

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

Right now, we store the type information for instance data nodes directly on the
node as an attribute (nsXFormsModelElement::ProcessBind()). We need to store it
as a property instead.

Comment 1

[Allan Beaufour]

Attachment: Patch

Comment 2

[Allan Beaufour]

Attachment: Correct patch

Hmmm, something went wrong in the previous patch.

This patch stores the type information as a property instead of as an
attribute.

Comment 3

[Allan Beaufour]

(In reply to comment #2)
> Created an attachment (id=175298) [edit]

To questions
1) I'm not sure I use SetProperty correct. Will it delete the nsAutoString for me?
2) Should I use nsString instead?

Comment 4

[Doron Rosenberg (IBM)]

Comment on attachment 175298 [details] [diff] [review]
Correct patch

bryner: could you look at the nsIContent usage and say if its correct/etc?

Comment 5

[Allan Beaufour]

*** Bug 283298 has been marked as a duplicate of this bug. ***

Comment 6

[Allan Beaufour]

Attachment: Updated patch

Added a note about bug 283376, and ignored errors for non-content nodes (like
attributes (per bug 283298).

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> Added a note about bug 283376, and ignored errors for non-content nodes (like
> attributes (per bug 283298).

Problem is that the spec states that "However, in contrast to xsi:type, type can
be added to both elements and attributes."
http://www.w3.org/TR/xforms/slice6.html#model-prop-type

and attributes do not implement nsIContent :(

Comment 8

[Doron Rosenberg (IBM)]

would storing it on the nodestate be of any help?  I guess not since we don't
create something on it for the attributes, right?

Comment 9

[Allan Beaufour]

As Doron found out: Bug 264308 is our friend.

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 175410 [details] [diff] [review]
Updated patch

>+        // Set property
>+        nsAutoString *prop = new nsAutoString(propStrings[j]);

Hm, this is rather unusual (malloc'ing an nsAutoString).  It does work though,
and only mallocs once, instead of twice as it would be if you used an nsString.
 You might ask darin if there's a better way though.

>+        rv = content->SetProperty(sModelPropsList[j],
>+                                  prop,
>+                                  nsnull);

However, you do need a destructor callback, I think.  Otherwise the string
buffer will be leaked.

>--- xforms/nsXFormsUtils.cpp	2005-02-24 08:51:42.000000000 +0100
>+++ xforms.typeproperty/nsXFormsUtils.cpp	2005-02-24 10:27:53.297872008 +0100
>@@ -1015,30 +1016,31 @@ nsXFormsUtils::GetInstanceNodeForData(ns
...
>+  nsAString *typeAttribute = (nsAString*) nodeContent->GetProperty(nsXFormsAtoms::type, &rv);

Cast to nsAutoString (it'll be more efficient to call through).  Also, use
NS_STATIC_CAST.

sr=me with those changes.

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> (From update of attachment 175410 [details] [diff] [review] [edit])
> >+        // Set property
> >+        nsAutoString *prop = new nsAutoString(propStrings[j]);
> 
> Hm, this is rather unusual (malloc'ing an nsAutoString).  It does work though,
> and only mallocs once, instead of twice as it would be if you used an nsString.
>  You might ask darin if there's a better way though.

Any comments on this one Darin?

Comment 12

[Allan Beaufour]

(In reply to comment #7)
> (In reply to comment #6)
> > Added a note about bug 283376, and ignored errors for non-content nodes (like
> > attributes (per bug 283298).
> 
> Problem is that the spec states that "However, in contrast to xsi:type, type can
> be added to both elements and attributes."
> http://www.w3.org/TR/xforms/slice6.html#model-prop-type
> 
> and attributes do not implement nsIContent :(

Looked some more on this. Funnily enough attributes implement nsIAttribute ...
and it's possible to get to the nsIContent through that. Problem is that it is
the nsIContent for the node with the attribute, so that does not help us. I feel
we are heading in the wrong direction here :( Question is then where to store it. 

Should we really store it in the MDG? I've been somewhat reluctant about that,
as I do not feel that type information belongs there. Maybe just a simple
hashmap (nsIDOMnode, nsAString) on the model? What do you say?

Comment 13

[Doron Rosenberg (IBM)]

A hashmap would work probably.  Is porting what we need from  Bug 264308 too hard?

Comment 14

[aaronr]

If you can't seperate out what you need from the other code, then I'd say go
with the hashmap.

Comment 15

[Allan Beaufour]

In case you haven't seen, I'm working on bug 285597 and bug 235512 to get this
one working.

Comment 16

[Allan Beaufour]

Attachment: Testcase

Comment 17

[Allan Beaufour]

Attachment: updated to current CVS

Let's do a review round again, just to be sure... it's been a while since it
was originally reviewed.

Comment 18

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

(In reply to comment #17)
> Created an attachment (id=185248) [edit]
> updated to current CVS
> 
I guess I need Bug 285597 with this patch?

Comment 19

[Allan Beaufour]

(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=185248) [edit] [edit]
> > updated to current CVS
> > 
> I guess I need Bug 285597 with this patch?

Sure is. I'll check it in quite soon.

Comment 20

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 185248 [details] [diff] [review]
updated to current CVS

r=me

>+  /// @bug We need to check for any type attributes set directly on the node
>+  /// too (XXX)
>+
Is there a bug open for this?

Comment 21

[Allan Beaufour]

(In reply to comment #20)
> (From update of attachment 185248 [details] [diff] [review] [edit])
> r=me
> 
> >+  /// @bug We need to check for any type attributes set directly on the node
> >+  /// too (XXX)
> >+
> Is there a bug open for this?

I think bug 293082 should fix it.

Comment 22

[aaronr]

Comment on attachment 185248 [details] [diff] [review]
updated to current CVS

looks good.  r=me

Comment 23

[Allan Beaufour]

Checked in