On 16 Aug 2019 11:29, Mirko Brodesser wrote: > we've decided to remove Thunderbird-related code in > nsPlainTextSerializer (e.g. > https://searchfox.org/mozilla-central/source/dom/base/nsPlainTextSerializer.h#172). > > This wasn't an easy decision, but given the complexity of the code and > that no one in Mozilla is very familiar with it, this step is > inevitable. Otherwise, it's impossible to confidently fix bugs in this > area (and there are some, e.g. > https://bugzilla.mozilla.org/show_bug.cgi?id=1568313). The work on > this will likely start on the coming Monday (19th of August). > > Please let me know if I can help in any way so that the code > transition is digestible for Thunderbird. > For this, it could help, if you explain how Thunderbird-code is > actually related to Gecko-code. As far as I understand, it uses parts > of Gecko as a basis, but some parts are forked and additional code is > put on top. It could also help to understand, how you're going to deal > with this change. Hi Mirko, thanks for the head-up. To answer the last paragraph first: Thunderbird compiles *all* Gecko code, there are only a few TB-specific tweaks which you can see here: https://searchfox.org/mozilla-central/search?q=moz_thunderbird&case=false&regexp=false&path= There are other tweaks for tests which we skip when running toolkit tests in a Thunderbird environment: https://searchfox.org/mozilla-central/search?q=skip.*thunderbird&case=false&regexp=true&path= https://searchfox.org/mozilla-central/search?q=MOZ_APP_NAME.*thunderbird&case=false&regexp=true&path= There are some more tweaks which you can see just looking for "thunderbird", but I believe the above searches are less confusing and more to the point. Additionally to compiling all mozilla-central code, we also add out own code which resides in https://hg.mozilla.org/comm-central/file. It's a lot less code than M-C. We have our own classes which implement M-C provided interfaces, like nsIURI, and many more. As for the "forked" code: Yes, over the course of the history of the project, M-C has discontinued some components, for example the Mork database, RDF and the source viewer. So we forked those bits. We're currently working on removing the remaining RDF usage and the RDF fork. TB uses the Mozilla editor for editing HTML and so-called plaintext e-mail messages. In M-C's editor you find things like: https://searchfox.org/mozilla-central/search?q=ismaileditor&case=false&regexp=false&path= https://searchfox.org/mozilla-central/search?q=eEditorMailMask&case=false&regexp=true&path= https://searchfox.org/mozilla-central/search?q=mPreFormattedMail&case=false&regexp=false&path= seems to be another special code path implemented for Thunderbird, or more precisely, implemented 20 years ago in the Netscape Suite for the mail part. As you can understand from the description above, we have no way to "deal with this change". We can't simple replace what's happening in a single source file, like dom/base/nsPlainTextSerializer.cpp. From the query above you can see that there are six code locations where mPreFormattedMail is actually checked, so instead of removing the code, I suggest to instead understand and document it, and we can collaborate on that, of course. You can also add conditional compilation with #ifdef MOZ_THUNDERBIRD. As you can see from the other queries, this collaboration is already taking place in other areas. May I say, also to the benefit of the Mozilla platform, since Thunderbird provides first-class testing and uncovers many bugs before they hit Firefox users, especially in the area of the editor and somewhat related, the serialiser. With kind regards, Jörg.

On 16-08-2019 14:19, Jörg Knobloch wrote: > On 16 Aug 2019 11:29, Mirko Brodesser wrote: >> we've decided to remove Thunderbird-related code in >> nsPlainTextSerializer (e.g. >> https://searchfox.org/mozilla-central/source/dom/base/nsPlainTextSerializer.h#172). >> >> This wasn't an easy decision, but given the complexity of the code >> and that no one in Mozilla is very familiar with it, this step is >> inevitable. Otherwise, it's impossible to confidently fix bugs in >> this area (and there are some, e.g. >> https://bugzilla.mozilla.org/show_bug.cgi?id=1568313). The work on >> this will likely start on the coming Monday (19th of August). >> >> Please let me know if I can help in any way so that the code >> transition is digestible for Thunderbird. >> For this, it could help, if you explain how Thunderbird-code is >> actually related to Gecko-code. As far as I understand, it uses parts >> of Gecko as a basis, but some parts are forked and additional code is >> put on top. It could also help to understand, how you're going to >> deal with this change. > > > https://searchfox.org/mozilla-central/search?q=mPreFormattedMail&case=false&regexp=false&path= > > seems to be another special code path implemented for Thunderbird, or > more precisely, implemented 20 years ago in the Netscape Suite for the > mail part. > > As you can understand from the description above, we have no way to > "deal with this change". We can't simple replace what's happening in a > single source file, like dom/base/nsPlainTextSerializer.cpp. From the > query above you can see that there are six code locations where > mPreFormattedMail is actually checked, so instead of removing the > code, I suggest to instead understand and document it, and we can > collaborate on that, of course. You can also add conditional > compilation with #ifdef MOZ_THUNDERBIRD. I this case, the code for mPreFormattedMail is indeed not very understandable (relating to how Thunderbird handles plain text quotes of long lines). Seem like it's relying on some magic assumptions atm. So perhaps there's a better way to go about it nowadays(?) Did you already file a bug for this?  -Magnus

Hi Jörg, Magnus, thanks for the clear explanations. Highly appreciated. Let's see if we can come up with a different approach of providing the most value to Gecko and Thunderbird. `mPreFormattedMail` was just one example, there's for instance also `OutputFormatFlowed` which seems to be used only by Thunderbird. Understanding and documenting the code is an option, however this can be very time consuming. One reason is, the code might've ended up in a contradictory state with certain code-paths are not being executed or parts of the code are buggy, but it's unclear, that they are. Comments are often outdated too. The same potentially applies to Thunderbird-code. Reasoning over one complex codebase is already tough, and it gets tougher with a second one. Ideally, Firefox developers shouldn't have to spend time on Thunderbird-code. I wonder if a smarter re-usage of Gecko-code (for `nsPlainTextSerializer` and perhaps `nsDocumentEncoder`) in Thunderbird-code would be possible, if the classes in Gecko were broken down to smaller classes (doing ideally one thing, not many). What do you think about this? Having `#ifdef`s in Gecko is possible, but it makes the code complicated. Magnus: there's no Bugzilla ticket for `mPreFormattedMail` yet. Currently I've just been cleaning up Thunderbird-unrelated code around `nsDocumentEncoder`: e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1574458, https://bugzilla.mozilla.org/show_bug.cgi?id=1574463, https://bugzilla.mozilla.org/show_bug.cgi?id=1574479. Kind regards, Mirko On Fri, Aug 16, 2019 at 1:49 PM Magnus Melin <mkmelin+mozilla@iki.fi> wrote: > On 16-08-2019 14:19, Jörg Knobloch wrote: > > On 16 Aug 2019 11:29, Mirko Brodesser wrote: > >> we've decided to remove Thunderbird-related code in > >> nsPlainTextSerializer (e.g. > >> > https://searchfox.org/mozilla-central/source/dom/base/nsPlainTextSerializer.h#172 > ). > >> > >> This wasn't an easy decision, but given the complexity of the code > >> and that no one in Mozilla is very familiar with it, this step is > >> inevitable. Otherwise, it's impossible to confidently fix bugs in > >> this area (and there are some, e.g. > >> https://bugzilla.mozilla.org/show_bug.cgi?id=1568313). The work on > >> this will likely start on the coming Monday (19th of August). > >> > >> Please let me know if I can help in any way so that the code > >> transition is digestible for Thunderbird. > >> For this, it could help, if you explain how Thunderbird-code is > >> actually related to Gecko-code. As far as I understand, it uses parts > >> of Gecko as a basis, but some parts are forked and additional code is > >> put on top. It could also help to understand, how you're going to > >> deal with this change. > > > > > > > https://searchfox.org/mozilla-central/search?q=mPreFormattedMail&case=false&regexp=false&path= > > > > seems to be another special code path implemented for Thunderbird, or > > more precisely, implemented 20 years ago in the Netscape Suite for the > > mail part. > > > > As you can understand from the description above, we have no way to > > "deal with this change". We can't simple replace what's happening in a > > single source file, like dom/base/nsPlainTextSerializer.cpp. From the > > query above you can see that there are six code locations where > > mPreFormattedMail is actually checked, so instead of removing the > > code, I suggest to instead understand and document it, and we can > > collaborate on that, of course. You can also add conditional > > compilation with #ifdef MOZ_THUNDERBIRD. > > I this case, the code for mPreFormattedMail is indeed not very > understandable (relating to how Thunderbird handles plain text quotes of > long lines). Seem like it's relying on some magic assumptions atm. So > perhaps there's a better way to go about it nowadays(?) > > Did you already file a bug for this? > > -Magnus > > > >