our 1.1 insert code can't handle empty target nodesest

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
10 years ago
11 months ago

People

(Reporter: aaronr, Assigned: Merle Sterling)

Tracking

({fixed1.8.1.8})

Trunk
x86
All
fixed1.8.1.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

1.92 KB, application/xhtml+xml
Details
2.88 KB, application/xhtml+xml
Details
2.25 KB, application/xhtml+xml
Details
10.12 KB, patch
aaronr
: review+
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
Created attachment 273173 [details]
testcase

Found by a user on the newsgroup and SteveS, our 1.1 insert doesn't correctly handle the case where the nodeset that we are inserting into is empty.

Testcase I'm attaching comes from John Boyer's blog.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 1

10 years ago
Created attachment 273187 [details]
testcase2

This testcase is similar to the first testcase but with attributes.  Works on Orbeon, but not formsPlayer/Sidewinder.  But I don't think that there is anything wrong with it.
(Reporter)

Comment 2

10 years ago
Created attachment 273189 [details]
testcase3

testcase from John on NG.
(Reporter)

Comment 3

10 years ago
Created attachment 273194 [details]
testcase2

fixed the 'delete' trigger in testcase 2
Attachment #273187 - Attachment is obsolete: true
(Assignee)

Comment 4

10 years ago
Created attachment 273209 [details] [diff] [review]
patch

- No longer restricting insert of an element to another element. Now elements can be inserted to any non-attribute node. Fixes testcase 1, 2 and 3.
- If the insert location node is empty, the inserted node will become the first child of the location node. Fixes the first test of testcase 3.
- We were unable to delete an attribute node but now check if the node to be deleted is an attribute node. If so, gets the owner element and removes the attribute node. Fixes the delete in testcase 2.
Attachment #273209 - Flags: review?(aaronr)
(Reporter)

Comment 5

10 years ago
Comment on attachment 273209 [details] [diff] [review]
patch

>Index: nsXFormsInsertDeleteElement.cpp
>===================================================================

>@@ -652,52 +684,54 @@ nsXFormsInsertDeleteElement::InsertNode(

>+    // New node will be inserted either before or after targetNode.
>+    nsCOMPtr<nsIDOMNode> targetNode;
>+    targetNode = aTargetNode;

nit: assign targetNode to aTargetNode when you declare it on the previous line.

with that, r=me
Attachment #273209 - Flags: review?(aaronr) → review+
(Assignee)

Comment 6

10 years ago
Created attachment 273437 [details] [diff] [review]
patch2
Attachment #273209 - Attachment is obsolete: true
Attachment #273437 - Flags: review?(Olli.Pettay)

Updated

10 years ago
Attachment #273437 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 7

10 years ago
Created attachment 273863 [details] [diff] [review]
final patch

Delete attribute case was not incrementing the deleteIndex for the while loop so we loop more than necessary and get ns_ensure_state warnings when the second pass thru the loop tried to get the owner element for the attribute node that was deleted in the first iteration of the loop.
Attachment #273437 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
Attachment #273863 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #273863 - Flags: review?(Olli.Pettay)
(Reporter)

Updated

10 years ago
Duplicate of this bug: 389868

Updated

10 years ago
Attachment #273863 - Flags: review?(Olli.Pettay) → review+
(Reporter)

Comment 9

10 years ago
checked into trunk and 1.8 branch for msterlin
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.7
Resolution: --- → FIXED
>+  enum Location {
>+    eLocation_After,
>+    eLocation_Before,
>+    eLocation_FirstChild,
>+  };

The trailing comma there is a compile error with gcc, for what it's worth.  Please fix?
(Reporter)

Comment 11

10 years ago
(In reply to comment #10)
> >+  enum Location {
> >+    eLocation_After,
> >+    eLocation_Before,
> >+    eLocation_FirstChild,
> >+  };
> 
> The trailing comma there is a compile error with gcc, for what it's worth. 
> Please fix?
> 

Thanks for catching it.  Will fix.
(Reporter)

Updated

10 years ago
Duplicate of this bug: 413354
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.