Closed
Bug 1253424
Opened 8 years ago
Closed 8 years ago
small refactorings of nsTransactionStack
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
Details
(Whiteboard: btpp-active)
Attachments
(2 files)
4.62 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
5.15 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
We unnecessarily refcount in several places when pushing items onto an nsTransactionStack. Adding an already_AddRefed overload will enable avoid those unnecessary refcounts. editor/ is currently unowned. However, since these patches are small and they're not touching core editor/ functionality, I don't feel bad about not seeking Ehsan's input on them. I think they can be reviewed by anybody who understands Gecko code.
Attachment #8726415 -
Flags: review?(erahm)
Assignee | ||
Comment 2•8 years ago
|
||
Checking to see whether the stack is empty is a reasonably common operation. We can make the code clearer and more efficient (no need to refcount to check the emptiness of the stack) in several cases.
Attachment #8726416 -
Flags: review?(erahm)
Comment 3•8 years ago
|
||
Comment on attachment 8726415 [details] [diff] [review] part 1 - add a already_AddRefed nsTransactionStack::Push overload Review of attachment 8726415 [details] [diff] [review]: ----------------------------------------------------------------- r=me Just a few small comments. ::: editor/txmgr/nsTransactionStack.cpp @@ +25,5 @@ > nsTransactionStack::Push(nsTransactionItem *aTransaction) > { > if (!aTransaction) { > return; > } Can we get rid of this check? @@ +41,5 @@ > + return; > + } > + > + // XXX we really want to use emplace_back here, but we don't have a > + // C++11 stdlib everywhere. It might be worth noting why we use a dummy here -- to avoid the addref b/c an empty RefPtr won't addref when copied -- I know that because I'm reading the code but I could see it being confusing out of context. Aside: push_back(Move(item)) would have worked if we could count on C++11 as well right?
Attachment #8726415 -
Flags: review?(erahm) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8726416 [details] [diff] [review] part 2 - add nsTransactionStack::IsEmpty Review of attachment 8726416 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #8726416 -
Flags: review?(erahm) → review+
Updated•8 years ago
|
Whiteboard: btpp-active
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c048ada6176 https://hg.mozilla.org/integration/mozilla-inbound/rev/4f323559171d
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c048ada6176 https://hg.mozilla.org/mozilla-central/rev/4f323559171d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•