640106 - Transaction manager needs tests for getUndoList, getRedoList

Status: NEW

(Core :: DOM: Editor, defect)

Description

[Alex Vincent [:WeirdAl]]

http://mxr.mozilla.org/mozilla-central/source/editor/txmgr/tests/TestTXMgr.cpp makes no mention of getUndoList or getRedoList.

Comment 1

[Serge Gautherie (:sgautherie)]

Ftr,

These 2 methods have been there since CVS r1.1.

http://mxr.mozilla.org/comm-central/search?string=getRedoList&case=on
{
/mozilla/editor/txmgr/idl/nsITransactionManager.idl
    * line 153 -- nsITransactionList getRedoList();
/suite/debugQA/content/debugQAEditorOverlay.js
    * line 425 -- PrintTxnList(txmgr.getRedoList(), "");
}
http://mxr.mozilla.org/comm-central/search?string=getUndoList&case=on
{
/mozilla/editor/txmgr/idl/nsITransactionManager.idl
    * line 146 -- nsITransactionList getUndoList();
/suite/debugQA/content/debugQAEditorOverlay.js
    * line 403 -- PrintTxnList(txmgr.getUndoList(), "");
}
Then I wonder whether these methods could just be removed/replaced_somehow (now)?
(Asking just to be explicit.)

***

http://mxr.mozilla.org/comm-central/search?string=getUndoList&case=on
{
/suite/browser/navigator.js
    * line 1412 -- recentTabsItem.setAttribute("disabled", !browser || browser.getUndoList().length == 0);
/suite/browser/tabbrowser.xml
    * line 1601 -- <method name="getUndoList">
}
Misak, these 2 occurrences are not-yet-sync'ed sessionstore code, right?

Comment 2

[Alex Vincent [:WeirdAl]]

If you do replace or remove them, be aware of what peekUndoStack and peekRedoStack say for batched transactions (which is how I got interested in this).

"Callers should be aware that this method could return return a null in some implementations if there is a batch at the top of the undo stack."

(Doubled return word from the current text.  peekRedoStack replaces "undo" with "redo", of course.)

In other words, without getUndoList, I might not be able to inspect the batched transaction.  In our current implementation, I cannot do that inspection because the top transaction in peekUndoStack is null.

beginBatch() and endBatch() are at least moderately useful, and from my experience, removing those would seriously annoy certain Composer-based editors.

Comment 3

[(no longer active)]

Why do we need to test this in the cpp test?  I'm planning to get rid of it at some point.  This should be done in a new xpcshell test I guess.

Comment 4

[(no longer active)]

(In reply to comment #1)
> Then I wonder whether these methods could just be removed/replaced_somehow
> (now)?
> (Asking just to be explicit.)

What's the value in doing that?  Add-ons and other applications might be relying on them, and I don't see the point in removing them.

> Misak, these 2 occurrences are not-yet-sync'ed sessionstore code, right?

Those are not relevant.  It just happens that the XUL tabbrowser control also has a method named getUndoList.