330663 - Content builder doesn't synchronize text nodes correctly.

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jan Varga [:janv]]

Comment 1

[Jan Varga [:janv]]

Attachment: trunk patch

Comment 2

[Neil Deakin]

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

Comment 3

[Neil Deakin]

Attachment: This is a better patch

Comment 4

[Jan Varga [:janv]]

yeah, the same approach is used elsewhere

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 215284 [details] [diff] [review]
This is a better patch

>Index: content/xul/templates/src/nsXULContentBuilder.cpp

This bug could have used a pointer to nsXULContentBuilder::BuildContentFromTemplate.  The patch should certainly have a comment pointing to it and saying that the two should stay in sync....  Possibly also a comment in BuildContentFromTemplate about keeping in sync.

In general, it would have been really nice if this bug had actually described the problem; would have saved me a lot of time.  :(

>+            nsIAtom *tag = tmplKid->Tag();
>+            PRInt32 nameSpaceID = tmplKid->GetNameSpaceID();

Why bother?  Why not just use tmplKid->NodeInfo->Equals(....) ?

>+                if (!attrValue.IsEmpty()) {

If we get into this code after changing the template, this looks wrong.  For example, when going from a nonempty attr to an empty one we won't update the value...

Of course when going from an empty attr to a nonempty one we won't even get the right realKid here, will we?  I mean given the similar test in BuildContentFromTemplate?

Or am I misunderstanding when this code is used?

>+                    nsCOMPtr<nsITextContent> textcontent = do_QueryInterface(realKid);
>+                    if (textcontent)

When would that QI return null?

Comment 6

[Neil Deakin]

This bug fixes the <textnode value="?label"> case which creates a text node with that label, but it doesn't update dynamically.

> Or am I misunderstanding when this code is used?

The code is used when the RDF data changes in such a way that only the attributes need to be modified, that is, only changes to existing item, no new or removed items. It doesn't have anything to do with <template> itself changing.

I suppose it's possible someone changed the template DOM in the meantime, but that isn't something that needs to be handled in any particular way. 

> 
> >+                    nsCOMPtr<nsITextContent> textcontent = do_QueryInterface(realKid);
> >+                    if (textcontent)
> 
> When would that QI return null?
> 

If someone manipulated the template generated content it might. 

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  So we still have a problem with the proposed patch if the RDF changes to/from an empty string, no?

Comment 8

[Neil Deakin]

Not that I can see, no. tmplKid is the node in the template, which doesn't change. The value from the RDF gets filled in inside SubstituteText. If it comes out empty, the text node just gets set to a null string. There's isn't an issue with that is there?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 215284 [details] [diff] [review]
This is a better patch

Ah, I see.  The empty checks are done on the "?whatever" string...  ok.

r+sr=bzbarsky with the comments added.

Comment 10

[Neil Deakin]

added the comments and checked in