1484118 - Remove the XPCOM component registration for nsTransactionManager

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[(no longer active)]

Attachment: Remove the XPCOM component registration for nsTransactionManager

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[(no longer active)]

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If nobody who were requested ni replies for this bug, I'll give r+ on this Friday.

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up, I've filed bug 1485275.

Comment 7

[Geoff Lankow (:darktrojan)]

(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.

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

(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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae7dc05bee29
Remove the XPCOM component registration for nsTransactionManager; r=masayuki

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ae7dc05bee29