730882 - Clean up nsTransactionStack

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

I have a patch which cleans up nsTransactionStack to make it look and perform better.

Comment 1

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 600958
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=05aeacdc221e
Try run started, revision 05aeacdc221e. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=05aeacdc221e

Comment 2

[Mozilla RelEng Bot]

Try run for 05aeacdc221e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=05aeacdc221e
Results (out of 217 total builds):
    exception: 1
    success: 160
    warnings: 40
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-05aeacdc221e

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 600958 [details] [diff] [review]
Patch (v1)

Review of attachment 600958 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/txmgr/src/nsTransactionItem.cpp
@@ +187,5 @@
>      NS_ENSURE_TRUE(mUndoStack, NS_ERROR_FAILURE);
>  
> +    nsRefPtr<nsTransactionItem> child = mUndoStack->GetItem(aIndex);
> +    child.forget(aChild);
> +    return (*aChild) ? NS_OK : NS_ERROR_FAILURE;

don't need parens

@@ +202,5 @@
>    NS_ENSURE_TRUE(mRedoStack && numItems != 0 && aIndex < numItems, NS_ERROR_FAILURE);
>  
> +  nsRefPtr<nsTransactionItem> child = mRedoStack->GetItem(aIndex);
> +  child.forget(aChild);
> +  return (*aChild) ? NS_OK : NS_ERROR_FAILURE;

don't need parens

@@ +385,5 @@
>      return NS_OK;
>    }
>  
> +  *aNumItems = mUndoStack->GetSize();
> +  return (*aNumItems) ? NS_OK : NS_ERROR_FAILURE;

don't need parens

@@ +399,5 @@
>      return NS_OK;
>    }
>  
> +  *aNumItems = mRedoStack->GetSize();
> +  return (*aNumItems) ? NS_OK : NS_ERROR_FAILURE;

don't need parens

Comment 4

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7546dd13db9d

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7546dd13db9d

Comment 6

[Asaf Romano (gone)]

Hey Ehsan,

I'm working on a JS implementation of the TM (bug 891303), and I was wondering if you've any idea on why nsTransactionStack::Clear removes the transaction items from the stacks one-by-one, - and in reverse order for the redo stack (Pop vs. PopBottom) - rather than just clearing the arrays right away. I know you've only consolidated this code, and that the algorithm dates back to 1998 (with the useful comment "initial checkin"), so "no idea" is a fine answer :)

I would only expect this to affect the order in which the transactions _might_ be released from memory (and, as a result, the destructors call order, for transactions implemented in c++), but I would hope no one relies on this behavior.

Comment 7

[(no longer active)]

(In reply to comment #6)
> Hey Ehsan,
> 
> I'm working on a JS implementation of the TM (bug 891303), and I was wondering
> if you've any idea on why nsTransactionStack::Clear removes the transaction
> items from the stacks one-by-one, - and in reverse order for the redo stack
> (Pop vs. PopBottom) - rather than just clearing the arrays right away. I know
> you've only consolidated this code, and that the algorithm dates back to 1998
> (with the useful comment "initial checkin"), so "no idea" is a fine answer :)

I'm not really sure why this behavior exists, but presumably some code somewhere has been relying on this exact release order?

> I would only expect this to affect the order in which the transactions _might_
> be released from memory (and, as a result, the destructors call order, for
> transactions implemented in c++), but I would hope no one relies on this
> behavior.

This will most likely not be important for transactions implemented in js, since they cannot have destructors, and will be collected by GC.

Speaking of which, does this mean that once bug 891303 lands, there will be no places consumers of this code?

Comment 8

[Asaf Romano (gone)]

That's the goal, yes. Though I'm not sure if the old tm will be removed at the same time or if we're going to wait for addons to catch up.