Remove the XPCOM component registration for nsTransactionManager

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

No description provided.
Assignee: nobody → ehsan
Blocks: 1477576
Hmm, Calendar uses this in JS:
https://searchfox.org/comm-central/rev/4a672c43e12378a7cacbb5f90e1f64db4d5256ec/calendar/base/src/calTransactionManager.js#10-12
and also mailnews uses this in C++:
https://searchfox.org/comm-central/rev/4a672c43e12378a7cacbb5f90e1f64db4d5256ec/mailnews/base/src/nsMsgWindow.cpp#69

The latter can be rewrite with |new TransactionManager()| since TransactionManager.h is exposed as "mozilla/TransactionManager.h".

JorgK:

I don't know whether Calender is active component or not, though. Anyway, I guess comm-central can provide the constructor for Calendar if necessary. Are you okay to remove it?
Flags: needinfo?(jorgk)
Thanks for the heads-up. Yes, Calendar is and active component, and no, I'm not OK with removing it. We'd have to fork the removed code into C-C, which is of course possible.

Forwarding the NI to the Calendar people.
Flags: needinfo?(philipp)
Flags: needinfo?(jorgk)
Flags: needinfo?(geoff)
What the c-c code needs to do is to add an XPI API somewhere that returns an nsITransactionManager object, implement it in C++ by returning a new nsTransactionManager object, and call that API from JS instead of the XPCOM machinery here: <https://searchfox.org/comm-central/rev/4a672c43e12378a7cacbb5f90e1f64db4d5256ec/calendar/base/src/calTransactionManager.js#11>.

For an example of how to do this, see the API added in the second iteration of the patch on this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1483650#c9
If nobody who were requested ni replies for this bug, I'll give r+ on this Friday.
Depends on: 1485275
Thanks for the heads-up, I've filed bug 1485275.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #5)
> If nobody who were requested ni replies for this bug, I'll give r+ on this
> Friday.

Sorry, I was hoping Philipp would take this as he likes XPCOM much more than I do. Apparently not.
Flags: needinfo?(philipp)
Flags: needinfo?(geoff)
Generally I'd like to see the use of the transaction manager replaced with a full js implementation that is independent of xpcom, but this should be a concern for bug 1485275. I think we are pretty much there for the quick fix, if Jörg/Geoff feel confident that the patches in that bug are ready I think we can move forward on this bug.
(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #8)
> Generally I'd like to see the use of the transaction manager replaced with a
> full js implementation that is independent of xpcom, but this should be a
> concern for bug 1485275. I think we are pretty much there for the quick fix,
> if Jörg/Geoff feel confident that the patches in that bug are ready I think
> we can move forward on this bug.

Do you mean that TransactionManager is completely replaced with same one implemented by JS? I.e., do you think that editor's transaction manager should be implemented with JS too? If so, I really worry about the performance. Some editor paths may be very hot paths. So, even only virtual calls may appear in profile. Currently, I'm working on splitting all XPCOM methods which are used internally, for avoiding virtual calls.
Comment on attachment 9001837 [details] [diff] [review]
Remove the XPCOM component registration for nsTransactionManager

Thank you for waiting to land this!

Looks like that they are ready to land into comm-central now.
Attachment #9001837 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> Do you mean that TransactionManager is completely replaced with same one
> implemented by JS? I.e., do you think that editor's transaction manager
> should be implemented with JS too? If so, I really worry about the
> performance. Some editor paths may be very hot paths. So, even only virtual
> calls may appear in profile. Currently, I'm working on splitting all XPCOM
> methods which are used internally, for avoiding virtual calls.

Sorry, I was speaking only for comm-central/calendar. We only use this as a stack for undo/redo operations, and I believe an independent implementation would work just as well there.
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae7dc05bee29
Remove the XPCOM component registration for nsTransactionManager; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/ae7dc05bee29
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.