Closed Bug 730882 Opened 8 years ago Closed 8 years ago

Clean up nsTransactionStack

Categories

(Core :: DOM: Editor, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file)

Attached patch Patch (v1)Splinter Review
I have a patch which cleans up nsTransactionStack to make it look and perform better.
Attachment #600958 - Flags: review?(roc)
Whiteboard: [autoland]
Whiteboard: [autoland] → [autoland-in-queue]
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
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/autolanduser@mozilla.com-05aeacdc221e
Whiteboard: [autoland-in-queue]
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
Attachment #600958 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7546dd13db9d
Assignee: nobody → ehsan
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/7546dd13db9d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
(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?
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.
You need to log in before you can comment on or make changes to this bug.