178264 - nsRangeUpdater bugs and enhancements

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Joe Francis]

While working on bug 143338 I found a few bugs and limitations in nsRangeUpdater.

1) nsRangeUpdater does not protect against multiple registrations of the same
nsRangeItem, which can lead to multiple adjustment of te same range item.

2) nsRangeUpdater assumed that it had ownership of all nsRangeItems passed to
it, and that they were all on the heap (it called delete to free them).  This
assumption ended up not being very convenient for the caller.

3) nsRangeUpdater notification methods were incomplete for deletion.  While
DeleteText() and DeleteNode() called into nsRangeUpdater, DeleteSelection() didn't.

4) nsRangeUpdater had some dead code.

Next is a patch for all this, which also fixes some broken comments, unused
variables, etc, here and there.

Comment 1

[Joe Francis]

Attachment: patch to editor/libeditor/base

The only suxy part of this patch is that I had to move the deletion notifyers
for nsRangeUpdater down into the delete transactions themselves, further
poluting their api.  But since most of the txns already have a poluted api
(taking an nsIEditor as an Init param, usually) this is just more sauce for the
goose.

Comment 2

[Kathleen :Brade]

Comment on attachment 105067 [details] [diff] [review]
patch to editor/libeditor/base

in DeleteElementTxn.cpp, around line 113, fix this typo:  nlike  (unlike?)

r=brade

Comment 3

[kinmoz]

Comment on attachment 105067 [details] [diff] [review]
patch to editor/libeditor/base

==== Just in case ... should we be initializing mRangeUpdater in the
constructors for DeleteElementTxn and DeleteRangeTxn like we do in
DeleteTextTxn?

==== Do we have to be concerned about the fact that DeleteTextTxn will call
|mRangeUpdater->SelAdjDeleteText()| during a redo? I was just curious since the
range updater is never used during a redo of a DeleteElementTxn.

Comment 4

[Joe Francis]

I fixed brade's typo; I corected the constructors for DeleteElementTxn and
DeleteRangeTxn (good catch); and I added range updating to
DeleteElementTxn::RedoTransaction()  (another good catch).

Fix landed on trunk.

Comment 5

[Joe Francis]

oops.  can't check this in yet becasue it depends on patch to 143338, which is
not yet reviewed.  reopening.

Comment 6

[Joe Francis]

Attachment: updated diff to address review comments

Comment 7

[Joe Francis]

fix landed on trunk.

Comment 8

[Joe Francis]

marking...

Comment 9

[Joe Francis]

self verifying this (preventive maintainance only...)