156567 - combine txtsvc in to editor dll

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Alec Flett]

it now looks like after the latest find cleanup, the only consumer of txtsvc is
editor itself. I think we should combine the tiny dll into the main editor dll.
The DLL itself is only 27k (windows) so personally I think its worth simply
rolling this into editor - we'll save the overhead of having an additional DLL.
(my primary concern here is embedding footprint)

Comment 1

[Alec Flett]

assigning to myself now that I know the default owner

Comment 2

[timeless]

.

Comment 3

[Alec Flett]

kin, akkana, any objections here? Otherwise I'm just going to go ahead and do
the work.

Comment 4

[Akkana Peck]

Have we given up on any other parts of the product ever using the transaction
manager?  Originally it was thought that other consumers, like mail, might want
to use this for undo/redo.

Comment 5

[kinmoz]

Mail does use the Transaction manager (for non-editor related tasks). Other
non-mozilla projects do also.

alecf, I'm not sure if you're lumping the Netscape Spellchecker (in the
commercial tree) into the editor category or not, but it also uses the txtsvc
module, so does the other non-netscape spellchecker that someone is working on
at mozdev.

At some point we want to revamp the spellchecker api's to have a controller and
a backend. I'm thinking we'll move a way from using the txtsvc module for the
backend rewrite in favor of leveraging some of the new find work, since it can
handle ranges, etc.

I'm not sure it's worth all the trouble right now?

Comment 6

[Alec Flett]

akkana: txmgr != txtsvc

As for txtsvc, its worth it to me because it reduces the number (and total size)
of DLLs that embedding needs - part of a larger effort to reduce the number of
DLLs required for embedding. I'm willing to do the work, I just want to make
sure the editor team doesn't object.

The advantage of keeping it a seperate DLL is that people don't need to include
it if they don't need it - but at 27k, I think we're better off just rolling it
into editor. 

As for other consumers of txtsvc, that just makes it all the more attractive to
lump it into the editor dll..(i.e. more consumers means less incentive to keep
it optional)

Comment 7

[Alec Flett]

Attachment: roll txtsvc into editor

imagine there is a similar patch to mac :)

Comment 8

[Alec Flett]

Attachment: roll txtsvc into editor, with installer patches

Ok, this is a better patch
what's going on here:
1) txtsvc is becoming a static library on windows and unix
   (on mac it is going away entirely)
2) we're linking this static library in with editor, and adding it to the
factory
   (on mac I'm just adding it to the editor project directly)
3) xpinstall is not only no longer installing it, but also deleting the old dll
if it is installed.

Looking for reviewers.

Comment 9

[Akkana Peck]

Comment on attachment 96225 [details] [diff] [review]
roll txtsvc into editor, with installer patches

This is fine with me as long as kin approves it.  I have two comments on the
patch:
1. If txtsvcs is part of libeditor, I'd be happier seeing the directory moved
under libeditor as well, rather than libeditor/build building a directory
outside of itself.  Sorry, I know it's a pain to move directories in cvs, but
it would make it easier for future editor maintainers.
2. Please cvs remove any files that become obsolete (nsTextServiceFactory.*
should go away now, no?)
Otherwise r=akkana.

Comment 10

[Alec Flett]

akkana: I'll ask leaf to move the directories, and I'll definitely cvs remove
the files that are no longer used - I hate tree clutter too :)

Comment 11

[kinmoz]

Comment on attachment 96225 [details] [diff] [review]
roll txtsvc into editor, with installer patches

sr=[e-mail]

Personally, I think moving the dir is a waste of time, since we want txtsvc to
eventually go away in favor of a spellchecker rewrite ... is anyone really
going to maintain txtsvc aside from me?

That said, it's up to you guys, as long as it's a dir *copy* so we can retain
the CVS info of each file.

Thanks for doing this Alec.

Comment 12

[Akkana Peck]

If Kin doesn't think we need to move the directory, I certainly won't insist on
it (though I'd still slightly prefer it).

Comment 13

[Alec Flett]

how about I go ahead and land when the tree opens, and when leaf moves it, I'll
later move stuff around, r=akkana, sr=kin...

Comment 14

[kinmoz]

That's fine with me.

Comment 15

[Jerome Lacoste]

Alec, 

this checkin is perhaps the cause for the performance regression in bug 164209.
(3% startup increase).

I've had a look at the patch and the only thing I found strange was the change
in editor/libeditor/build/nsEditorRegistration.cpp;1.24

http://lxr.mozilla.org/mozilla/source/editor/libeditor/build/nsEditorRegistration.cpp

I am probably wrong, but can the NULL field in the new nsModuleComponentInfo
entry be the reason?

 98     { NULL, NS_TEXTSERVICESDOCUMENT_CID,
 99       "@mozilla.org/textservices/textservicesdocument;1",
100       nsTextServicesDocumentConstructor

Comment 16

[Alec Flett]

marking fixed - this MAY have caused bug 164209, but I'm still skeptical. The
end result was:

Before:
editor.dll 373k
txtsvc.dll 23k

After:
editor.dll 393k

This is why I think this is unlikely: an addition of 20k to one DLL should not
cause a 3% startup regression.

Comment 17

[sairuh (rarely reading bugmail)]

rs vrfy.