(Not really backend, but about shared mailnews code.) SeaMonkey and Thunderbird still share quite some UI located under /mailnews. While it makes sense to share rather independent subcomponents like the address book, the account manager or the mail widget definitions, sharing smaller parts is not good idea: eg. it's quite hard to keep the view overlay stuff in a state where it applies cleanly to both SM and TB UI. There may be different thoughts about which information to show, eg. bug 369393/180546. Especially since SeaMonkey intends to move its non-shared UI from /mailnews to /suite anyway, we should also unshare all those minor files not part of address book/account manager/mail widgets and move them to mail/suite respectively. This query <http://mxr-test.landfill.bugzilla.org/seamonkey/search?string=mailnews&find=mail%2F.*jar.mn&findi=&filter=&hitlimit=&tree=seamonkey> lists all UI files located under /mailnews which are packaged by TB and should be the base for a discussion of what to share (and keep in /mailnews) and what to fork (and move out of /mailnews).

Just as a note, bug 367034 is filed about the mentioned moving of SeaMonkey-specific UI files from mailnews/ to suite/

This patch is a first go in moving SM's MailNews UI out of /mailnews. Ideally, in future, /mailnews may only contain the shared files itself, but no jar.mn anymore. I tried to disentangle both UIs based upon the question whether sharing was actually helpful or not. IMO, sharing a frontend file only makes sense, if: - it's a window/dialog - it's only included by a shared window/dialog - provides functionality not tied (too hard) to specific frontend features Basically, sharing XUL overlays (like mailviews) is not a good idea, it just makes fixes harder on both sides. Same goes for JS files (like threadPane.js) which make too specific assumptions about the UI they're touching.

Comment on attachment 363479 [details] [diff] [review] Disentanglement, v1 >+ suite/mailnews/extensions/smime/Makefile >+ suite/mailnews/extensions/mailviews/Makefile Is there any real reason for those being in an "extensions" subdir? I'd rather not have such a subdir in suite/mailnews/ if (reasonably) possible. Also, I'm not yet completely convinced on two of your points: 1) Why can't any overlay be shared? We surely want to support extensions working on both products, which also integrate via overlays, so I wouldn't make that rules so strict (and smime actually sounds like something that could potentially be shared, IMHO). 2) I still think we could or even should have jar.mn files in mailnews for the shared stuff, I really dislike those cross-major-dir jar.mn pointers we have. They basically mean we never can reasonably share L10n parts for those files.

(In reply to comment #3) > (From update of attachment 363479 [details] [diff] [review]) > >+ suite/mailnews/extensions/smime/Makefile > >+ suite/mailnews/extensions/mailviews/Makefile > > Is there any real reason for those being in an "extensions" subdir? I'd rather > not have such a subdir in suite/mailnews/ if (reasonably) possible. Mailviews and smime are - along with certain other stuff like MDN - optional extensions when building the backend and thus are found under /mailnews/extensions/. We should mirror that on our side to keep confusion at a minimum - or possibly move them out of */extensions/* everywhere. > Also, I'm not yet completely convinced on two of your points: > 1) Why can't any overlay be shared? That wasn't meant as a superstrict rule, more a guideline: - If the overlay fits in at a well-defined (toplevel?) "mount point" - fine, use it! - If it tries to alter subtle details in the UI, it's not good idea to share it. I only touched those pertaining to the latter category. I hope. > smime actually sounds like something that could potentially be shared S/MIME UI as used in the account manager is still shared, see am-smime* files. Sharing S/MIME overlays for the rapidly diverging header pane implementations is not useful, IMO. Sharing UI supporting JS files is difficult/dangerous, if they should support diverging XUL files. I don't think that S/MIME as such is a good candidate for complete sharing. Account Manager/Wizard and Addressbook are better examples, I think. > 2) I still think we could or even should have jar.mn files in mailnews for the > shared stuff, I really dislike those cross-major-dir jar.mn pointers we have. > They basically mean we never can reasonably share L10n parts for those files. I wasn't aware of the L10N angle here, could you elaborate, please? I'm fine either way.

Comment on attachment 363479 [details] [diff] [review] Disentanglement, v1 >- content/messenger/addressbook/pref-directory-add.js (/mailnews/addrbook/prefs/resources/content/pref-directory-add.js) ... >+ content/messenger/addressbook/pref-directory-add.js (content/pref-directory-add.js) >+ content/messenger/addressbook/pref-directory-add.xul (content/pref-directory-add.xul) >+ content/messenger/addressbook/pref-editdirectories.js (content/pref-editdirectories.js) >+ content/messenger/addressbook/pref-editdirectories.xul (content/pref-editdirectories.xul) I really don't understand the logic of this change. Why split these out? These are the LDAP pref dialogs, and are referenced from both applications. The only time I can see these changing is if we rework how our prefs setup for those are done, but I still can't see why we'd ever separate the UI for those even if we did change them. >diff --git a/mailnews/extensions/smime/resources/content/certFetchingStatus.js b/mail/extensions/smime/content/certFetchingStatus.js Like KaiRo mentioned, I can't see a reason to fork these - if we change the UI and break the integration points, then we can fork or fix the integration points. I certainly think we can be more restrictive than you are suggesting for initial forking. I think I also agree with KaiRo re keeping jar.mn files in mailnews/. Obviously we should end up with no app-specific ifdefs in those files, and that will make it clear that the particular files are being included in both applications (as well as having the build logic close to the real files).

(In reply to comment #4) > Mailviews and smime are - along with certain other stuff like MDN - optional > extensions when building the backend and thus are found under > /mailnews/extensions/. We should mirror that on our side to keep confusion at a > minimum - or possibly move them out of */extensions/* everywhere. The old Mozilla code base had the bad habit of creating too many possibly useless levels in the directory tree, one example is resources/ and extensions/ is another that we got both rid of in the new suite/ hierarchy. In many cases, stuff in extensions/ directories was integrated far enough over time that optional building of those was only theoretically or not at all supported any more, and today, an "extension" is understood as something that is also shipped as one. For those concrete cases, I don't think we realistically do or should support optional building of mailviews in SeaMonkey any more, and Thunderbird has already integrated it in an non-optional way (they package but don't use the overlay, which is something you could perhaps even fix in this patch). Mailviews has become an integrated piece of our UI and we shouldn't treat it as anything else. I'm somewhat impartial to the question if S/MIME should be optional on the build side, but if so, suite/mailnews/smime/ would be enough of a directory depth to make it separate if we want to. I'd be a lot for doing away with extensions/ in /mailnews/ as well, but I think we should leave that to a different patch (hg at least enables us to reorg it without losing history). > Account Manager/Wizard and Addressbook are better examples, I think. abook is IMHO a good candidate for unforking some time ;-) For the record, I fully I agree that making the whole forked and shared file story cleaner is a very good thing and that some additional forks are a good thing, I also agree on the guidelines you set up there. As most of the time, the discussion items are some border cases (like S/MIME) :) > > 2) I still think we could or even should have jar.mn files in mailnews for the > > shared stuff, I really dislike those cross-major-dir jar.mn pointers we have. > > They basically mean we never can reasonably share L10n parts for those files. > > I wasn't aware of the L10N angle here, could you elaborate, please? > I'm fine either way. I see that my statement jumped over a few explanation steps ;-) Basically, I think we want to go and share L10n files that are only used by shared code in the future, and put them under a /mailnews/locales/ directory in both comm(-central) and the L10n repos. That also means that we'll need a /mailnews/locales/jar.mn for those. That in turn means that the supposed benefit of needing a "make chrome" or such in /mail only if you're changing any chrome files affecting Thunderbird will be gone anyway. That in turn means that we can go for a clean approach that only refers to and packages shared files in a single place in a mailnews/jar.mn right away and have people do a "make chrome" (or whatever) in mailnews/ when changing any shared files. I agree with Standard8 that we should end up with no app-specific ifdefs there (ideally, none anywhere in mailnews/, I guess).

(In reply to comment #6) > > - or possibly move them out of */extensions/* everywhere. > > The old Mozilla code base had the bad habit of creating too many possibly > useless levels in the directory tree, one example is resources/ and > extensions/ is another that we got both rid of in the new suite/ hierarchy. So you'd propose moving /extensions' subdirs "up one level"? (Would be fine by me.) > For those concrete cases, I don't think we realistically do or should support > optional building of mailviews in SeaMonkey True. > any more, and Thunderbird has already integrated it in an non-optional way > (they package but don't use the overlay, which is something you could > perhaps even fix in this patch). I already did. :) > I'm somewhat impartial to the question if S/MIME should be optional on the > build side, but if so, suite/mailnews/smime/ would be enough of a directory > depth to make it separate if we want to. Basically I'd say that building S/MIME should not be optional, but I'd like to hear more voices on that before changing it. > I'd be a lot for doing away with extensions/ in /mailnews/ as well, but I > think we should leave that to a different patch Yes, definitely. > > Account Manager/Wizard and Addressbook are better examples, I think. > > abook is IMHO a good candidate for unforking some time ;-) There are just seven files forked, and most of these only have minimal differences which might as well be cooked down to a single file. But that, too, is another bug... > Basically, I think we want to go and share L10n files that are only used by > shared code in the future, and put them under a /mailnews/locales/ directory > in both comm(-central) and the L10n repos. That also means that we'll need a > /mailnews/locales/jar.mn for those. Okay. > That in turn means that we can go for a clean approach that only refers to > and packages shared files in a single place in a mailnews/jar.mn right away > and have people do a "make chrome" (or whatever) in mailnews/ when changing > any shared files. To let get this straight: this would mean that even stuff like addressbook or account manager would not have their own jar.mn under /mailnews, but would be packeaged in /mailnews/jar.mn? > I agree with Standard8 that we should end up with no app-specific ifdefs there > (ideally, none anywhere in mailnews/, I guess). Sure, it wouldn't make much sense else. ;-)

(In reply to comment #7) > (In reply to comment #6) > > > - or possibly move them out of */extensions/* everywhere. > > > > The old Mozilla code base had the bad habit of creating too many possibly > > useless levels in the directory tree, one example is resources/ and > > extensions/ is another that we got both rid of in the new suite/ hierarchy. > > So you'd propose moving /extensions' subdirs "up one level"? > (Would be fine by me.) If you mean with "up one level" to e.g. move "mailnews/extensions/smime" to "mailnews/smime", then yes, that's what I propose. We probably should put off parts that don't directly affect the "disentanglement" targeted in here to a separate bug/patch though. > > That in turn means that we can go for a clean approach that only refers to > > and packages shared files in a single place in a mailnews/jar.mn right away > > and have people do a "make chrome" (or whatever) in mailnews/ when changing > > any shared files. > > To let get this straight: this would mean that even stuff like addressbook or > account manager would not have their own jar.mn under /mailnews, but would be > packeaged in /mailnews/jar.mn? Er, no, we can of course still have more than a single jar.mn for anything else than locales. I meant "one single place per packaged file", not "one single place for all files". mailnews/jar.mn was just an example.

(In reply to comment #8) > (In reply to comment #7) > > To let get this straight: this would mean that even stuff like addressbook or > > account manager would not have their own jar.mn under /mailnews, but would be > > packeaged in /mailnews/jar.mn? > > Er, no, we can of course still have more than a single jar.mn for anything else > than locales. I meant "one single place per packaged file", not "one single > place for all files". mailnews/jar.mn was just an example. I'd certainly want to restrict the number of jar.mn files that we have, the more we have increases the times we have to open and add to messenger.jar.

> I'd certainly want to restrict the number of jar.mn files that we have, the > more we have increases the times we have to open and add to messenger.jar. Sure, but is this a real world problem? kd@aupertir:~/moz/src/trunk$ find mail -name jar.mn | wc -l 14 kd@aupertir:~/moz/src/trunk$ find mailnews -name jar.mn | wc -l 7 kd@aupertir:~/moz/src/trunk$ find suite -name jar.mn | wc -l 11

Comment on attachment 363479 [details] [diff] [review] Disentanglement, v1 >diff --git a/mailnews/extensions/mailviews/jar.mn b/mailnews/extensions/mailviews/jar.mn >deleted file mode 100644 No related Makefile.in changes? >diff --git a/mailnews/jar.mn b/mailnews/jar.mn >deleted file mode 100644 I think it would be worthwhile keeping unforked files here. >+PARALLEL_DIRS = addrbook extensions jar.mn files are more expensive when there isn't anything else to recurse for. >- content/messenger/mailWidgets.xml >+ content/messenger/mailWidgets.xml (base/mailWidgets.xml) I don't see the point of a "base" subdirectory. On the other hand, "addrbook" makes sense because that mirrors the final layout. >+* content/messenger/am-identity-edit.js (/mailnews/base/prefs/resources/content/am-identity-edit.js) [Odd. Why is this file preprocessed?]

(In reply to comment #3) > They basically mean we never can reasonably share L10n parts for those files. In some cases we can't share l10n anyway, e.g. Flagged vs. Starred.

(In reply to comment #12) > (In reply to comment #3) > > They basically mean we never can reasonably share L10n parts for those files. > In some cases we can't share l10n anyway, e.g. Flagged vs. Starred. Those cases should not be files that affect only the shared backend though, but files that affect the forked frontend, right? In any case, that's probably fodder for a different bug, I just wanted to mention it here so that the direction for that is in mind for this bug here.

(In reply to comment #11) > >+* content/messenger/am-identity-edit.js (/mailnews/base/prefs/resources/content/am-identity-edit.js) > [Odd. Why is this file preprocessed?] Some certain individual introduced two #ifndef blocks in bug 455915. :-P

Addressed the review comments: * left S/MIME alone for now (but we should have a bug for TB's deforkment) * deextensioned mailviews UI for SM * kept mailnews/jar.mn for shared files * left LDAP "pref-*" dialogs shared (but I think they're a bit misnamed) * dropped the /base/ directory path for SM UI * still left am-id-edit.js preprocessing as is, a possible depreprocessing should probably go into another bug

Mac OS X/Thunderbird Try Server build of attachment #369593 [details] [diff] [review] can be found here: <http://s3.mozillamessaging.com/build/try-server/2009-04-02_13:41-Philippe%20M.%20Chiasson-attachment-369593/Philippe%20M.%20Chiasson-attachment-369593-mail-try-mac.dmg>

The reordering of the lines in jar.mn makes the diff harder to review :s (I didn't investigate whether there was a reason for the reordering.)

> (I didn't investigate whether there was a reason for the reordering.) No real reason, apart from some cleanup - I just fixed the last patch by re-moving parts back, I did not actually rewrite the patch. :|

Comment on attachment 369593 [details] [diff] [review] Disentanglement, v2 I built and ran with this and things seem to work...

Comment on attachment 369593 [details] [diff] [review] Disentanglement, v2 >diff --git a/mailnews/jar.mn b/mailnews/jar.mn >--- a/mailnews/jar.mn >+++ b/mailnews/jar.mn >@@ -1,226 +1,97 @@ The new jar.mn should have somewhat more than 97 lines I hope ;-) >- content/messenger/addressbook/abEditCardDialog.xul (addrbook/resources/content/abEditCardDialog.xul) Should not have been removed. >- content/messenger/messengercompose/mailComposeEditorOverlay.xul (compose/resources/content/mailComposeEditorOverlay.xul) >-* content/messenger/importDialog.js (import/resources/content/importDialog.js) >-* content/messenger/importDialog.xul (import/resources/content/importDialog.xul) >- content/messenger/fieldMapImport.xul (import/resources/content/fieldMapImport.xul) >- content/messenger/fieldMapImport.js (import/resources/content/fieldMapImport.js) >- content/messenger/downloadheaders.js (news/resources/content/downloadheaders.js) >- content/messenger/downloadheaders.xul (news/resources/content/downloadheaders.xul) Should not have been removed. >+ content/messenger/am-junk.js (base/prefs/resources/content/am-junk.js) Nit: trailing whitespace. That's your fault for moving lines around ;-) >+ content/messenger/messengercompose/askSendFormat.js (compose/resources/content/askSendFormat.js) >+ content/messenger/messengercompose/askSendFormat.xul (compose/resources/content/askSendFormat.xul) >+ content/messenger/messengercompose/sendProgress.xul (compose/resources/content/sendProgress.xul) >+ content/messenger/messengercompose/sendProgress.js (compose/resources/content/sendProgress.js) >+ content/messenger/messengercompose/MsgAttachPage.xul (compose/resources/content/MsgAttachPage.xul) >+ content/messenger/messengercompose/MsgAttachPage.js (compose/resources/content/MsgAttachPage.js) >+ content/messenger/messengercompose/askSendFormat.js (compose/resources/content/askSendFormat.js) >+ content/messenger/messengercompose/askSendFormat.xul (compose/resources/content/askSendFormat.xul) >+ content/messenger/messengercompose/sendProgress.xul (compose/resources/content/sendProgress.xul) >+ content/messenger/messengercompose/sendProgress.js (compose/resources/content/sendProgress.js) >+ content/messenger/messengercompose/MsgAttachPage.xul (compose/resources/content/MsgAttachPage.xul) >+ content/messenger/messengercompose/MsgAttachPage.js (compose/resources/content/MsgAttachPage.js) Duplications. Also caused by moving lines around, perhaps? r=me with these fixed (and preferably fewer moves, too...)

Fixed Neil's nits and took over r/sr. Landed on trunk as <http://hg.mozilla.org/comm-central/rev/87feec865948>