Closed Bug 575740 Opened 10 years ago Closed 10 years ago

Make comm-central XPCOM components use new manifests and data tables

Categories

(MailNews Core :: Build Config, defect)

defect
Not set

Tracking

(blocking-seamonkey2.1 a3+)

RESOLVED FIXED
Thunderbird 3.3a1
Tracking Status
blocking-seamonkey2.1 --- a3+

People

(Reporter: kairo, Assigned: Bienvenu)

References

()

Details

(Whiteboard: [bug fixed][see comment 108 for follow-up information])

Attachments

(20 files, 19 obsolete files)

10.05 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
2.07 KB, patch
neil
: review+
Details | Diff | Splinter Review
16.74 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
5.06 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
3.91 KB, patch
Callek
: review+
Details | Diff | Splinter Review
6.85 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
1.27 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
105.17 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
2.15 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.95 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
9.36 KB, patch
standard8
: review+
Details | Diff | Splinter Review
22.39 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
737 bytes, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
72.60 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
7.50 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
2.73 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
2.11 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
4.25 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
6.49 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
920 bytes, patch
Details | Diff | Splinter Review
Bug 568691 is changing XPCOM registration a lot, we should prepare comm-central patches to deal with that so we don't break.

The Emacs macros in attachment 450679 [details] should be helpful there.
blocking-seamonkey2.1: --- → a3+
I wouldn't block on using the emacs macros - I suspect we can do it all by hand in an hour or so.
(In reply to comment #1)
> I wouldn't block on using the emacs macros - I suspect we can do it all by hand
> in an hour or so.

Sure, I just thought being able to use them should ease things for all of us - but it looks like bsmedberg is using an editor that's not too popular among our own users. ;-)

However we do it, we should find something here soon, as all patches over in the original bug have r+ now...
Added a mxr search url to the bug, thanks to bsmedberg and timeless.
taking for now, though until the disable-libxul issue is fixed, I won't be able to compile anything.
Assignee: nobody → bienvenu
You should be able to compile a static build, I think, or at least get it all the way to final link.
this gets the ldap code compiling, but I'm not able to test linking or running until the core linking issues are resolved (I'm doing static builds but even then the core moz code doesn't link).
this gets mork to compile - it also shows pretty much the minimal work required since Mork only exports one component.
Attachment #455771 - Flags: superreview?(bugzilla)
Attachment #455771 - Flags: review?(bugzilla)
Attached patch compose dir fix (obsolete) — Splinter Review
I'm going to have to roll these all together for static builds, but that'll be easier when I've got the non-static builds working.
Attachment #455782 - Flags: superreview?(bugzilla)
Attachment #455782 - Flags: review?(bugzilla)
Attached patch msg db patch (obsolete) — Splinter Review
Attachment #455753 - Flags: superreview?(neil)
Attachment #455753 - Flags: review?(neil)
Comment on attachment 455753 [details] [diff] [review]
fix mork - checked in

>+#include "mozilla/ModuleUtils.h"
> #include "nsIServiceManager.h"
> #include "nsCOMPtr.h"
> #include "nsIModule.h"
>-#include "nsIGenericFactory.h"
I would have just replaced the existing line rather than adding the include at a different place. Anyway, we can probably also get rid of nsIModule.h as well.

>-NS_IMPL_NSGETMODULE(nsMorkModule, components)
>+NSMODULE_DEFN(mork_factory) = &kMorkFactoryModule;
I take it you deliberately renamed the module from Mork to MorkFactory?
[I was expecting NSMODULE_DEFN(nsMorkModule) = &kMorkModule;]

>+  NULL,
>+  NULL,
>+  NULL,
>+  NULL
Nit: don't need these trailing nulls.
Attachment #455753 - Flags: superreview?(neil)
Attachment #455753 - Flags: superreview+
Attachment #455753 - Flags: review?(neil)
Attachment #455753 - Flags: review+
Err, where are the manifest file changes? Or will those be in a follow-up bug/patch?
I'm nowhere near done, so please bear with me. Thx!
Attached patch get extensions compiling (obsolete) — Splinter Review
haven't dealt with the js files in extensions yet...
Attached patch get imap building (obsolete) — Splinter Review
Note that this also includes the C++ changes from bug 450781 that allow the file to compile under either the internal or external API.
Attachment #455848 - Flags: superreview?(bienvenu)
Attachment #455848 - Flags: review?(bienvenu)
This patch swaps the comm-central part of the build system filling out components.list to components.manifest.

It also generates interfaces.manifest, as per mozilla-central, but I haven't worked out why they don't package it (yet?).

This implements the following mozilla-central revisions in comm-central:

http://hg.mozilla.org/mozilla-central/rev/90afd1e80d77
http://hg.mozilla.org/mozilla-central/rev/5e71dd583552
http://hg.mozilla.org/mozilla-central/rev/9657c7161818
http://hg.mozilla.org/mozilla-central/rev/cc155916daa6
http://hg.mozilla.org/mozilla-central/rev/666612addeb8

There may be others to come, but I've not found them yet ;-)
Attachment #455858 - Flags: review?(bugspam.Callek)
This should probably be in a separate bug, but it goes with getting comm-central working again. This fixes up the cpp registrations and compilation in mail/.

With this patch and some of the others on this bug, I can just about get the address book window to come up. xpcshell-tests are still crashing - I haven't determined why that is yet.

I also need to reland the history service patch, but I'll do that as a separate bustage fix checkin (as its simple and didn't have review before).
Attachment #455859 - Flags: review?(bienvenu)
I'll have import and base patches later this morning, and hope to finish off the rest of the c++ today.
Comment on attachment 455771 [details] [diff] [review]
fix address book - checked in.

>+  {&kNS_ABADDRESSCOLLECTOR_CID, false, NULL, nsAbAddressCollectorConstructor },

nit: space after {


>+  { &kNS_ABLDAPDIRFACTORY_CID, false, NULL, nsAbLDAPDirFactoryConstructor },
...
>+  { &kNS_ABLDAPDIRFACTORY_CID, false, NULL, nsAbLDAPDirFactoryConstructor },
>+  { &kNS_ABLDAPDIRFACTORY_CID, false, NULL, nsAbLDAPDirFactoryConstructor },

This same entry appears three times, we only need one of them.

>+static const mozilla::Module kAddressBookModule = {
>+  mozilla::Module::kVersion,
>+  kAddressBookCIDs,
>+  kAddressBookContracts,
>+  NULL,
>+  NULL,
>+  NULL,
>+  msgAbModuleDtor
> };

You need to add the NSMODULE_DEFN here.

This looks ok otherwise, although I can't fully test it yet. So r+sr=Standard8 with the above fixed, and we'll work out more issues as we go.
Attachment #455771 - Flags: superreview?(bugzilla)
Attachment #455771 - Flags: superreview+
Attachment #455771 - Flags: review?(bugzilla)
Attachment #455771 - Flags: review+
Attached patch get local building (obsolete) — Splinter Review
Attached patch get import building (on windows) (obsolete) — Splinter Review
have not checked this on mac/linux yet.
Attached patch get news building (obsolete) — Splinter Review
Comment on attachment 455875 [details] [diff] [review]
get base building (forgot to attach this earlier)

cuz I didn't finish it :-)
Attachment #455875 - Attachment is obsolete: true
Attached patch get base building for real (obsolete) — Splinter Review
Attachment #455898 - Flags: superreview?(bugzilla)
Attachment #455898 - Flags: review?(bugzilla)
Comment on attachment 455859 [details] [diff] [review]
Fix for mail/ - checked in

r=me, modulo the win and gnome fixes, which I've attached in an other patch
Attachment #455859 - Flags: review?(bienvenu) → review+
Attachment #455771 - Attachment description: fix address book → fix address book - checked in.
Attachment #455848 - Flags: superreview?(bienvenu)
Attachment #455848 - Flags: superreview+
Attachment #455848 - Flags: review?(bienvenu)
Attachment #455848 - Flags: review+
Attachment #455753 - Attachment description: fix mork → fix mork - checked in
Attachment #455692 - Flags: superreview?(bugzilla)
Attachment #455692 - Flags: review?(bugzilla)
Attached patch mapi and mime dirs (obsolete) — Splinter Review
with this patch, everything builds for me (non-static, in any case). Things don't run because of a failure to get the history service, but maybe the config changes help with that...trying that now.
Attachment #455918 - Flags: superreview?(bugzilla)
Attachment #455918 - Flags: review?(bugzilla)
this gets it so that a goodly number of the xpcshell tests pass
Attachment #455782 - Attachment is obsolete: true
Attachment #455790 - Attachment is obsolete: true
Attachment #455817 - Attachment is obsolete: true
Attachment #455818 - Attachment is obsolete: true
Attachment #455876 - Attachment is obsolete: true
Attachment #455877 - Attachment is obsolete: true
Attachment #455894 - Attachment is obsolete: true
Attachment #455897 - Attachment is obsolete: true
Attachment #455918 - Attachment is obsolete: true
Attachment #455782 - Flags: superreview?(bugzilla)
Attachment #455782 - Flags: review?(bugzilla)
Attachment #455918 - Flags: superreview?(bugzilla)
Attachment #455918 - Flags: review?(bugzilla)
I seemed to have messed up the bayesian filter plugin stuff somehow, which accounts for a good number of the test failures. I'll have a look at that tomorrow.

fwiw, the app starts up, but shuts down immediately (with the profile manager).
Comment on attachment 455858 [details] [diff] [review]
Swap from components.list to components.manifest - checked in

Please do the config.mk changes from http://hg.mozilla.org/mozilla-central/rev/9657c7161818 so we can also have these options for extension vs in-tree stuff (as that changeset did for reftest) [it is also useful for calendar]

The interfaces.manifest is used in NON packaged builds so we can actually load the .xpt's.

In packaged builds we link xpt's so its not used, *but* it does generate a foo.manifest for the foo.xpt's that we link; see: http://hg.mozilla.org/mozilla-central/rev/b7631123fd4d

Which means that we need to add the *.manifest files that we generate during xptlink into the package-manifests. (like browser.xpt for Firefox)

We can do this in a followup patch.
Attachment #455858 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 455848 [details] [diff] [review]
Fix autocomplete - checked in.

Pushed changeset 55f39d8d866c to mozilla-central.
Attachment #455848 - Attachment description: Fix autocomplete → Fix autocomplete - checked in.
Comment on attachment 455858 [details] [diff] [review]
Swap from components.list to components.manifest - checked in

Checked in: http://hg.mozilla.org/comm-central/rev/55c6c0967898

Agreed we need to do more manifests, we can add them once we get closer to complete builds.
Attachment #455858 - Attachment description: Swap from components.list to components.manifest → Swap from components.list to components.manifest - checked in
Comment on attachment 455859 [details] [diff] [review]
Fix for mail/ - checked in

Checked in without the shell changes: http://hg.mozilla.org/comm-central/rev/5a05b7a2e639
Attachment #455859 - Attachment description: Fix for mail/ → Fix for mail/ - checked in
Attachment #455898 - Flags: superreview?(bugzilla)
Attachment #455898 - Flags: superreview+
Attachment #455898 - Flags: review?(bugzilla)
Attachment #455898 - Flags: review+
(In reply to comment #29)
> with this patch, everything builds for me (non-static, in any case).

Leaving static busted for a bit won't hurt as it'll mean our nightly builds won't succeed (whilst we fix the js modules).

(In reply to comment #31)
> fwiw, the app starts up, but shuts down immediately (with the profile manager).

With the latest patches applied, I can get it to start with thunderbird-bin -js-console. I can then get the main windows open even if they are very broken due to not having the js modules/components registered.

If you want to start get it running "normally", I'd suggest starting with the ones in mail/steel and the mail/components/*.js ones.
Attachment #455692 - Flags: superreview?(bugzilla)
Attachment #455692 - Flags: superreview+
Attachment #455692 - Flags: review?(bugzilla)
Attachment #455692 - Flags: review+
oops, got confused about which patches were reviewed - so I landed the cumulative patch, as well as the others. I'll deal with the comments once they come in...
Attachment #455692 - Attachment description: first pass at ldap code → first pass at ldap code - checked in.
Attachment #455898 - Attachment description: fix windows and gnome shell → fix windows and gnome shell - checked in.
Blocks: 576869
I've just disabled static mail on unit test and debug tinderbox builds so that we can try and verify that the non-static (mail) configuration is working - I think we still need some js changes, but we should be able to show progress on tinderbox.

I'll also re-enable the static mail once we get the non-static version working (and before we re-open the tree).
Attached patch add some backend manifests (obsolete) — Splinter Review
I'm still working on these, but I have to stop for a couple hours. This fixes the trait service, which gets a lot more of the xpcshell tests working. Some of the remaining failures seem to be related to some of the js components I've added manifests for in this patch, so I may not have gotten them quite right (or the issue may be in the ones I haven't gotten to). Also, these manifests have to be added to the TB packages, but I'm leaving that for later.
Attachment #455952 - Flags: superreview?(bugzilla)
Attachment #455952 - Flags: review?(bugzilla)
Comment on attachment 455919 [details] [diff] [review]
cumulative fix, including category registration - checked in

It looks like you missed the import changes when you checked in this patch, so I've checked them in with an additional fix for Mac:

http://hg.mozilla.org/comm-central/rev/5afc8075d2a6

Flagging for review as I'll go through this later to double check we've not missed components even though the tests may pass.
Attachment #455919 - Attachment description: cumulative fix, including category registration → cumulative fix, including category registration - checked in
Attachment #455919 - Flags: superreview?(bugzilla)
Attachment #455919 - Flags: review?(bugzilla)
thx - working on the other js modules in mailnews now. Patch coming up in an hour or so.
Attachment #455952 - Flags: superreview?(bugzilla)
Attachment #455952 - Flags: review?(bugzilla)
Attachment #455952 - Flags: review-
Comment on attachment 455952 [details] [diff] [review]
add some backend manifests

> EXTRA_COMPONENTS += \
> 		nsAbLDAPAttributeMap.js \
>+		nsAbLDAPAttributeMap.manifest \
> 		nsAbAutoCompleteMyDomain.js \
>+		nsAbAutoCompleteMyDomain.manifest \
> 		nsAbAutoCompleteSearch.js \
>+		nsAbAutoCompleteSearch.manifest \
> 		$(NULL)

I'd prefer just to have one manifest for this directory e.g. see http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/src/toolkitplaces.manifest

(ditto in some other places, maybe just make all the added .manifest file names generic even if they currently have just one registration currently?)

>diff --git a/mailnews/addrbook/src/nsAbLDAPAttributeMap.js b/mailnews/addrbook/src/nsAbLDAPAttributeMap.js

I'm not convinced the changes in this file are right. I suggest you leave them to a separate patch which incorporates nsLDAPProtocolHandler.js fixes as well, then we should be able to work out if LDAP is working with this change.

>diff --git a/mailnews/base/src/msgAsyncPrompter.js b/mailnews/base/src/msgAsyncPrompter.js

> msgAsyncPrompter.prototype = {
>-  classDescription: "msgAsyncPrompter",
>   contractID: "@mozilla.org/messenger/msgAsyncPrompter;1",
>-  classID: Components.ID("{49b04761-23dd-45d7-903d-619418a4d319}"),

You wanted to remove the contractID line not the classID one.

>diff --git a/mailnews/base/src/nsCopyMessageStreamListener.cpp b/mailnews/base/src/nsCopyMessageStreamListener.cpp

These changes look like part of another bug.

Definitely seeing an improvement with this patch, I think most of the failures should drop into line as we go on.
Status update: building tinderboxes with non-static mail has made tests go orange on Mac, still red on Windows (ignore bloat passes, they are bogus), builds still won't run (I think the mail/ changes will be needed for that).
(In reply to comment #11)
>(From update of attachment 455753 [details] [diff] [review])
>>-NS_IMPL_NSGETMODULE(nsMorkModule, components)
>>+NSMODULE_DEFN(mork_factory) = &kMorkFactoryModule;
>I take it you deliberately renamed the module from Mork to MorkFactory?
>[I was expecting NSMODULE_DEFN(nsMorkModule) = &kMorkModule;]
In fact static builds require NSMODULE_DEFN(nsMorkModule)
Blocks: 576900
Attached patch mail fixes (obsolete) — Splinter Review
these don't get Thunderbird running, unfortunately, but I don't have time to figure out why right now.
Attached patch mailnews js components, v2 (obsolete) — Splinter Review
this addresses some of your prev comments, but not all - I've run out of time today, but I wanted to attach what I have so far.
(In reply to comment #44)
> In fact static builds require NSMODULE_DEFN(nsMorkModule)

OK, I've pushed that.
Blocks: 576910
Attached patch mail fixes v2 (obsolete) — Splinter Review
This fixes a couple of errors from the previous version of this patch (two in mailComponents.manifest and one in nsMailDefaultHandler.js).

With this and the latest mailnews one, I can start up TB, but it appears that STEEL isn't working for some reason. Still trying to figure out why.
Attachment #455986 - Attachment is obsolete: true
Attachment #455952 - Attachment is obsolete: true
Depends on: 576940
Attached patch mail fixes v3 (obsolete) — Splinter Review
This adds an extra line to steelApplication.manifest which gets steel working. So with these patches on a shared build I can start up Thunderbird and roughly get it working.

We still need to do phishing protection module (in mail/) at least, and some of the xpcshell-tests still fail, not sure why that is yet.

MozMill also fails to connect, but I think that's the mozmill extensions, so I filed bug 576940 on that.
Attachment #456025 - Attachment is obsolete: true
This fixes the LDAP js component under directory/ - it makes the directory/ and mailnews/addrbook/ ldap related tests pass.

Note that I think the old directory protocol handler was being incorrectly registered as it was using the same class ID for both ldap and ldaps instances. I've fixed that here as well (which helps to make the tests pass).
Attachment #456028 - Flags: superreview?(bienvenu)
Attachment #456028 - Flags: review?(bienvenu)
I finally figured out what was up with the import tests - you'd switched the category registrations to use contract IDs instead of CIDs, but you hadn't updated the code that used that.

I took a dive into fixing the code, and it turned out it touched lots of things, so I decided we'll just use the string CIDs direct for now, and we can hook in the contract IDs later.
Attachment #456032 - Flags: superreview?(bienvenu)
Attachment #456032 - Flags: review?(bienvenu)
Attachment #456032 - Attachment is patch: true
Attachment #456032 - Attachment mime type: application/octet-stream → text/plain
Status update:

With the current patches, fully shared builds will at least run. I'm not sure what the exact status of functionality is, but it at least brings up the folder pane and address book etc.

xpcshell tests: Majority are now passing, gloda ones fail with an assertion around "OnDataAvailable", some imap ones also fail (plus one other in mailnews/base/test which I think is related to the gloda issue).

I've not debugged those yet.

Mozmill: not running, see bug 576940

We've also got one core "make check" failure in TestStaticAtoms - need to file a bug for that.
Duplicate of this bug: 576758
Attachment #456028 - Flags: superreview?(bienvenu)
Attachment #456028 - Flags: superreview+
Attachment #456028 - Flags: review?(bienvenu)
Attachment #456028 - Flags: review+
Attachment #456032 - Flags: superreview?(bienvenu)
Attachment #456032 - Flags: superreview+
Attachment #456032 - Flags: review?(bienvenu)
Attachment #456032 - Flags: review+
Blocks: 576745
Comment on attachment 456032 [details] [diff] [review]
Fix import category registrations - checked in

Checked in: http://hg.mozilla.org/comm-central/rev/43417c1f109f
Attachment #456032 - Attachment description: Fix import category registrations → Fix import category registrations - checked in
Comment on attachment 456028 [details] [diff] [review]
Directory js component fix - checked in

Checked in: http://hg.mozilla.org/comm-central/rev/9be34c7e92c2
Attachment #456028 - Attachment description: Directory js component fix → Directory js component fix - checked in
Version 4 of the mail fixes - I realised the rule about manifests is actually that we need a manifest file in every directory that has a js component.

I've not actually enabled enforcing of that rule yet (as it stops the build) but that's what we should be doing.

I took your original mail fixes patch, added a couple of review comments and landed it - this is what I landed.

http://hg.mozilla.org/comm-central/rev/1eb74b7e9acb
Attachment #456027 - Attachment is obsolete: true
Attachment #456060 - Flags: review+
Attachment #455987 - Flags: superreview?(bugzilla)
Attachment #455987 - Flags: review?(bugzilla)
Comment on attachment 455987 [details] [diff] [review]
mailnews js components, v2

I'm not sure where we are with this patch, in relation to the fixes you've done...I'm not going to have much time to tweak this before I have to leave, but it would be nice to get the tree working to some extent
I took a look at the v2 patch, and I think its good enough with a few modifications (correction of a manifest, and a couple of formatting diffs in Makefile.ins). I also dropped the newsblog extension changes for now as I'm not convinced they are right, however, this helps clean up the mailnews picture so I've checked it in as well:

http://hg.mozilla.org/comm-central/rev/5c63d717ff1e
Attachment #455987 - Attachment is obsolete: true
Attachment #456064 - Flags: superreview+
Attachment #456064 - Flags: review+
Attachment #455987 - Flags: superreview?(bugzilla)
Attachment #455987 - Flags: review?(bugzilla)
Directories (under mailnews and mail) with js components that currently also do not have a manifest:

mailnews/extensions/newsblog
mail/components/shell
mail/components/phishing

I will also be having to leave in a while. As I've no idea about the current xpcshell hangs (which will need a debugger = lots of battery), if I work on anything it will be getting --enable-static-mail to work, i.e. the component translations in mailnews/build.
OK, if I can, I'll try to look at the xpcshell hangs.
(In reply to comment #60)
> OK, if I can, I'll try to look at the xpcshell hangs.

Just thinking, I guess there's a remote chance they could be linked to the TestStaticAtoms failure in make check. There's also a failure in test_resolve_uri.js, but I suspect that's something different.
Hi guys.  Depending on which config flags I use (I think it's enable-static) there is still one file breaking the build of comm-central because it's still using nsIGenericFactory.h:  mailnews/build/nsMailModule.cpp

Many thanks for your hard work on this bug.
(In reply to comment #62)
> Hi guys.  Depending on which config flags I use (I think it's enable-static)
> there is still one file breaking the build of comm-central because it's still
> using nsIGenericFactory.h:  mailnews/build/nsMailModule.cpp

Thanks, we already know about that (see comment 59). It is not just that as well, even with fixing that we've got various bustages in unit tests which may means things won't work properly even if it does build.

We know what the issues are (tinderbox and our own builds tell us) so please just bear with us and we'll fix it up over the next few days.
Attached patch newsblog patch (obsolete) — Splinter Review
Uses _xpcom_factory to do the stuff in newsblog.js, rest is as per bienvenu's patch for newsblog. Not been able to test it as yet.
Attachment #456116 - Flags: feedback?(bugspam.Callek)
+      if (!aIID.equals(Components.interfaces.nsINewsBlogFeedDownloader) &&
+          !aIID.equals(Components.interfaces.nsISupports))
+        throw Components.results.NS_ERROR_INVALID_ARG;

+      if (!aIID.equals(Components.interfaces.nsIMsgAccountManagerExtension) &&
+          !aIID.equals(Components.interfaces.nsISupports))
+        throw Components.results.NS_ERROR_INVALID_ARG;

Shouldn't we be using NS_ERROR_NO_INTERFACE instead? I don't see anywhere in mozilla-central where C.r.NS_ERROR_INVALID_ARG is used in a QI.
Comment on attachment 456116 [details] [diff] [review]
newsblog patch

>+EXTRA_COMPONENTS = js/newsblog.js \
>+		js/newsblog.manifest

Please make this:

EXTRA_COMPONENTS \
  js/newsblog.js \
  js/newsblog.manifest \
  $(NULL)
this fixes all but the ab autocomplete search test failures, which I'll look at next. I'll also check if I made the same mistake in any other manifests...
Attachment #456175 - Flags: superreview?(bugzilla)
Attachment #456175 - Flags: review?(bugzilla)
Attached patch nsMailModule fixes WIP (obsolete) — Splinter Review
This should be most of the hard work done on this, just needs a little tidying up.
Sorry Ian, I'm going to trump you on this - I did say I was working on mailnews/build next.

David: I constructed this by visiting all the other shared libraries module files and copy and pasting the data into the single file. AFAICT it passes all xpcshell tests to the same extent as the shared version of the build currently does.
Attachment #456210 - Attachment is obsolete: true
Attachment #456211 - Flags: superreview?(bienvenu)
Attachment #456211 - Flags: review?(bienvenu)
Comment on attachment 456175 [details] [diff] [review]
fix jsmime emitter - checked in

ab autocomplete search works for me - maybe something to do with the outlook case?

The bit that still doesn't work is test_imapAttachmentSave.js, however this is still a great improvement. I'll commit in a few if you're not about.
Attachment #456175 - Flags: superreview?(bugzilla)
Attachment #456175 - Flags: superreview+
Attachment #456175 - Flags: review?(bugzilla)
Attachment #456175 - Flags: review+
Comment on attachment 456211 [details] [diff] [review]
Static mailnews fix - checked in

>@@ -600,688 +775,420 @@ static NS_IMETHODIMP nsVCardMimeContentT
This changed to static nsresult in the nsVCardFactory.cpp changes.

>-// XXX These files are not being built as they don't work. Bug 311632 should
>-// fix them.
>-//    { "Address LDAP ChangeLog Query Interface", NS_ABLDAP_CHANGELOGQUERY_CID,
>-//      NS_ABLDAP_CHANGELOGQUERY_CONTRACTID, nsAbLDAPChangeLogQueryConstructor },
>-//    { "Address LDAP ChangeLog Processor Interface", NS_ABLDAP_PROCESSCHANGELOGDATA_CID,
>-//      NS_ABLDAP_PROCESSCHANGELOGDATA_CONTRACTID, nsAbLDAPProcessChangeLogDataConstructor },
Is it right to remove all trace of this comment and associated code?

When I was doing my patch I did notice there was a lot of whitespace inconsistencies with the original code which could be corrected in the patch too.
Comment on attachment 456175 [details] [diff] [review]
fix jsmime emitter - checked in

Checked in: http://hg.mozilla.org/comm-central/rev/fbc439f4cf46
Attachment #456175 - Attachment description: fix jsmime emitter → fix jsmime emitter - checked in
Comment on attachment 456211 [details] [diff] [review]
Static mailnews fix - checked in

need space after { here - 
+  {&kNS_STOPWATCH_CID, false, NULL, nsStopwatchConstructor},
probably should figure this out one way or the other:

+/*
+// XXX Why ifdef suite?
 #ifdef MOZ_SUITE

this all looks fine; I'm building it now (last night's build failed, sadly) and will + it after I try the tests...
Comment on attachment 456211 [details] [diff] [review]
Static mailnews fix - checked in

tests pass, yay! r/sr=me, modulo prev comments. Test failures I see are probably because of OS/X address book.
Attachment #456211 - Flags: superreview?(bienvenu)
Attachment #456211 - Flags: superreview+
Attachment #456211 - Flags: review?(bienvenu)
Attachment #456211 - Flags: review+
Check the newsblog patch for trailing whitespaces, I got a few warnings with my lint script there :-)
Comment on attachment 456064 [details] [diff] [review]
mailnews js components v3 - checked in

>diff --git a/mailnews/extensions/offline-startup/js/offlineStartup.js b/mailnews/extensions/offline-startup/js/offlineStartup.js
>--- a/mailnews/extensions/offline-startup/js/offlineStartup.js
>+++ b/mailnews/extensions/offline-startup/js/offlineStartup.js

This file seems to be missing an import of XPCOMUtils, I get:

Error: XPCOMUtils is not defined
Source File: file:///home/kewisch/mozilla/comm-central/obj-i686-pc-linux-gnu/mozilla/dist/bin/components/offlineStartup.js
Line: 253
(In reply to comment #65)
> +      if (!aIID.equals(Components.interfaces.nsINewsBlogFeedDownloader) &&
> +          !aIID.equals(Components.interfaces.nsISupports))
> +        throw Components.results.NS_ERROR_INVALID_ARG;
> 
> +      if (!aIID.equals(Components.interfaces.nsIMsgAccountManagerExtension) &&
> +          !aIID.equals(Components.interfaces.nsISupports))
> +        throw Components.results.NS_ERROR_INVALID_ARG;
> 
> Shouldn't we be using NS_ERROR_NO_INTERFACE instead? I don't see anywhere in
> mozilla-central where C.r.NS_ERROR_INVALID_ARG is used in a QI.

http://mxr.mozilla.org/mozilla-central/source/xpinstall/test/testXPIDialogService.js#166
http://mxr.mozilla.org/mozilla-central/source/embedding/browser/activex/src/plugin/nsAxSecurityPolicy.js#187

Anyway for the moment I think we should leave it as is and if need be create a bug about it and other places it happens.
Attachment #456116 - Flags: feedback?(bugspam.Callek)
Attached patch newsblog patch v1.1 (obsolete) — Splinter Review
Attachment #456116 - Attachment is obsolete: true
Attachment #456332 - Flags: superreview?(bugzilla)
Attachment #456332 - Flags: review?(bugzilla)
5(In reply to comment #71)
> >-// XXX These files are not being built as they don't work. Bug 311632 should
> >-// fix them.
> >-//    { "Address LDAP ChangeLog Query Interface", NS_ABLDAP_CHANGELOGQUERY_CID,
> >-//      NS_ABLDAP_CHANGELOGQUERY_CONTRACTID, nsAbLDAPChangeLogQueryConstructor },
> >-//    { "Address LDAP ChangeLog Processor Interface", NS_ABLDAP_PROCESSCHANGELOGDATA_CID,
> >-//      NS_ABLDAP_PROCESSCHANGELOGDATA_CONTRACTID, nsAbLDAPProcessChangeLogDataConstructor },
> Is it right to remove all trace of this comment and associated code?

It is very easy to bring back if we fix that up so I don't really care at this stage.

(In reply to comment #73)
> (From update of attachment 456211 [details] [diff] [review])
> +// XXX Why ifdef suite?
>  #ifdef MOZ_SUITE

I believe these were the registrations required for the old xpfe commandline handling stuff, so I dropped these.
Comment on attachment 456211 [details] [diff] [review]
Static mailnews fix - checked in

http://hg.mozilla.org/comm-central/rev/06e8af9f7f6d
Attachment #456211 - Attachment description: Static mailnews fix → Static mailnews fix - checked in
(In reply to comment #78)
> Created an attachment (id=456332) [details]
> newsblog patch v1.1

This fixes the trailing white spaces and corrects the Makefile.in
Attachment #456332 - Flags: superreview?(bugzilla)
Attachment #456332 - Flags: review?(bugzilla)
Remember to import XPCOMUtils.jsm module
Attachment #456332 - Attachment is obsolete: true
Attachment #456338 - Flags: superreview?(bugzilla)
Attachment #456338 - Flags: review?(bugzilla)
Attachment #456338 - Flags: superreview?(bugzilla)
Attachment #456338 - Flags: superreview+
Attachment #456338 - Flags: review?(bugzilla)
Attachment #456338 - Flags: review+
Comment on attachment 456338 [details] [diff] [review]
newsblog patch v1.2 [Checkin: Comment 84]

>diff --git a/mailnews/extensions/newsblog/js/newsblog.manifest b/mailnews/extensions/newsblog/js/newsblog.manifest

>+category mailnews-accountmanager-extensions newsblog @mozilla.org/accountmanager/extension;1?name=newsblog

Drop the extra "newsblog" - it doesn't seem to be required and probably breaks the category registration - e.g. see attachment 456175 [details] [diff] [review].

r/sr=Standard8 with that fixed.
Comment on attachment 456338 [details] [diff] [review]
newsblog patch v1.2 [Checkin: Comment 84]

http://hg.mozilla.org/comm-central/rev/9414216829f6 with correction to newsblog.manifest
Attachment #456338 - Attachment description: newsblog patch v1.2 → newsblog patch v1.2 [Checkin: Comment 84]
(In reply to comment #76)
> (From update of attachment 456064 [details] [diff] [review])
> >diff --git a/mailnews/extensions/offline-startup/js/offlineStartup.js b/mailnews/extensions/offline-startup/js/offlineStartup.js
> >--- a/mailnews/extensions/offline-startup/js/offlineStartup.js
> >+++ b/mailnews/extensions/offline-startup/js/offlineStartup.js
> 
> This file seems to be missing an import of XPCOMUtils, I get:
> 
> Error: XPCOMUtils is not defined
> Source File:
> file:///home/kewisch/mozilla/comm-central/obj-i686-pc-linux-gnu/mozilla/dist/bin/components/offlineStartup.js
> Line: 253

Looks like there may be a few more things to tidy up in this file too.
This patch:
* Adds missing import of XPCOMUtils
* Does some more tidy up of nsOfflineStartupModule
Attachment #456358 - Flags: superreview?
Attachment #456358 - Flags: review?
Attachment #456358 - Flags: superreview?(bienvenu)
Attachment #456358 - Flags: superreview?
Attachment #456358 - Flags: review?(bienvenu)
Attachment #456358 - Flags: review?
Status update:

- Shared builds (with static or non-static mailnews) now compile and run. There's a couple of modules still being fixed up (e.g. offline startup), but Thunderbird should largely be usable.

- Static builds are still being worked on, Neil has some patches up for review and David and I are working on remaining issues in getting them to work.

- Packaging will need fixing up - I'll do that once we get static builds running.

- TestStaticAtoms is still failing, not sure why yet.

- MozMill needs fixing, I'll possibly take a look at that in the next couple of days if the mozmill guys don't beat me to it.
Static builds require a specific module name - this patch fixes that.
Attachment #456365 - Flags: superreview?(bienvenu)
Attachment #456365 - Flags: review?(bienvenu)
Comment on attachment 456358 [details] [diff] [review]
more work on offlineStartup.js - checked in

I think you can just say "quiet warnings" - thx for the patch
Attachment #456358 - Flags: superreview?(bienvenu)
Attachment #456358 - Flags: superreview+
Attachment #456358 - Flags: review?(bienvenu)
Attachment #456358 - Flags: review+
Comment on attachment 456365 [details] [diff] [review]
Fix module naming of mailnews components - checked in

did you mean to include the mozmill change? Are those the only NSMODULE_DEFNs that I messed up?
Attachment #456365 - Flags: superreview?(bienvenu)
Attachment #456365 - Flags: superreview+
Attachment #456365 - Flags: review?(bienvenu)
Attachment #456365 - Flags: review+
in any case, it all links with that patch and the PIC one in chrome!
I now have a hacked up version of MozMill working for Thunderbird trunk, and the good news is that all the trunk tests appear to be working. I'll get the patches up in various places over the next couple of days.
(In reply to comment #83)
> (From update of attachment 456338 [details] [diff] [review])
> >diff --git a/mailnews/extensions/newsblog/js/newsblog.manifest b/mailnews/extensions/newsblog/js/newsblog.manifest
> 
> >+category mailnews-accountmanager-extensions newsblog @mozilla.org/accountmanager/extension;1?name=newsblog
> 
> Drop the extra "newsblog" - it doesn't seem to be required and probably breaks
> the category registration - e.g. see attachment 456175 [details] [diff] [review].
> 
> r/sr=Standard8 with that fixed.

Is this really true? Looking at https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_1.9.3 it seems the correct syntax for the category line is:

category <category-name> <category entry> <contract id>

This would also mean that attachment 456175 [details] [diff] [review] needs to be fixed.

(Not relevant in this case, but from testing it seems for the contract id we need to drop the "service," prefix for services)
Which is the Bug that will fix the sustained libxpcom_core issue (couldn't find it). Thanks. :-)
Comment on attachment 456365 [details] [diff] [review]
Fix module naming of mailnews components - checked in

Checked in: http://hg.mozilla.org/comm-central/rev/88bbf0eb57d7
Attachment #456365 - Attachment description: Fix module naming of mailnews components → Fix module naming of mailnews components - checked in
Depends on: 577496
(In reply to comment #93)
> > Drop the extra "newsblog" - it doesn't seem to be required and probably breaks
> > the category registration - e.g. see attachment 456175 [details] [diff] [review] [details].
...
> Is this really true? Looking at
> https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_1.9.3 it seems
> the correct syntax for the category line is:

You're right, that is wrong. I've pushed a fix: http://hg.mozilla.org/comm-central/rev/42d2d973ca51

> This would also mean that attachment 456175 [details] [diff] [review] needs to be fixed.

No, because in this case the <category entry> does want to be a contract id as that's what the code requires.
Comment on attachment 456358 [details] [diff] [review]
more work on offlineStartup.js - checked in

Checked in: http://hg.mozilla.org/comm-central/rev/5c6ffe86e3a0
Attachment #456358 - Attachment description: more work on offlineStartup.js → more work on offlineStartup.js - checked in
Status update:

- Shared builds pretty much same status as before (run, one check failure in TestStaticAtoms, mozmill tests not run) except all modules should now be working.

- Static builds now compile. However they do not start. On a debug static build, I'm getting the following - I'm still trying to work out what is causing this.

###!!! ASSERTION: I/O service not registered or available early enough?: 'nsCOMPtr<nsIIOService>(mozilla::services::GetIOService())', file /Users/moztest/comm/main/src/mozilla/chrome/src/nsChromeRegistry.cpp, line 185
WARNING: Could not get pref service!: file /Users/moztest/comm/main/src/mozilla/chrome/src/nsChromeRegistryChrome.cpp, line 187
WARNING: No IO service trying to process chrome manifests: file /Users/moztest/comm/main/src/mozilla/chrome/src/nsChromeRegistryChrome.cpp, line 787
Blocks: 577859
Do the leftover stuff in mailnews/ (I'm not certain when these are built, but figured it would be good to convert them)
Attachment #456714 - Flags: superreview?(bienvenu)
Attachment #456714 - Flags: review?(bienvenu)
Status: NEW → ASSIGNED
This fixes the shell and phishing services in mail/. It also enables the line of build checking so that if we have a js component without the manifest we'll get a build error (same as mozilla-central).
Attachment #456838 - Flags: review?(bienvenu)
Attachment #456838 - Attachment is patch: true
Attachment #456838 - Attachment mime type: application/octet-stream → text/plain
This time with the manifest files added to the patch...
Attachment #456838 - Attachment is obsolete: true
Attachment #456849 - Flags: review?(bienvenu)
Attachment #456838 - Flags: review?(bienvenu)
Comment on attachment 456714 [details] [diff] [review]
mailnews/mime/cthandlers [Checked in Comment 104]

these aren't built; I wonder if they should get hg removed, i.e., if no one is using them.
Attachment #456714 - Flags: superreview?(bienvenu)
Attachment #456714 - Flags: superreview+
Attachment #456714 - Flags: review?(bienvenu)
Attachment #456714 - Flags: review+
Attachment #456849 - Flags: review?(bienvenu) → review+
Comment on attachment 456849 [details] [diff] [review]
Further fixes to js modules in mail/ v2 - checked in

Checked in: http://hg.mozilla.org/comm-central/rev/43506d7392a0
Attachment #456849 - Attachment description: Further fixes to js modules in mail/ v2 → Further fixes to js modules in mail/ v2 - checked in
Comment on attachment 456714 [details] [diff] [review]
mailnews/mime/cthandlers [Checked in Comment 104]

http://hg.mozilla.org/comm-central/rev/5485ed369f42
Attachment #456714 - Attachment description: mailnews/mime/cthandlers → mailnews/mime/cthandlers [Checked in Comment 104]
Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100713 Shredder/3.2a1pre
Actually runs :)
Tested with a limited "test" profile, but feeds and newsgroups look OK
(In reply to comment #105)
> Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100713
> Shredder/3.2a1pre
> Actually runs :)

Yeah, I forgot to put out an update as that was covered in a separate bug. So quick status update:

- Both shared and static builds now run, we are re-enabling nightlies as from today.

- We've still got the TestStaticAtoms failure (bug 577500) to resolve

- We've provided a patch for the MozMill changes, we are waiting for a new version of MozMill to ship, then we need to deploy it on our build machines.

The tree will remain closed until we've done those, especially as MozMill is a large part of our test coverage (although running locally, I know MozMill is passing which is why we can re-enable nightlies).
Comment on attachment 455919 [details] [diff] [review]
cumulative fix, including category registration - checked in

Ok, I've now been through the patch and it generally looks fine. There were some minor whitespace issues that I thought it was just easiest if I tidied up as I did them, so as this patch has already landed, I've just landed the review comments in the tree:

http://hg.mozilla.org/comm-central/rev/89de54f97780
Attachment #455919 - Flags: superreview?(bugzilla)
Attachment #455919 - Flags: superreview+
Attachment #455919 - Flags: review?(bugzilla)
Attachment #455919 - Flags: review+
Blocks: 578675
AFAIK we have now transitioned everything mailnews and mail to the new
component registration format (afaik suite/ has been completely done in another
bug).

Therefore I'm closing this bug as fixed. Outstanding issues before we can
re-open the tree:

- Bug 577500 TestStaticAtoms failure (patch is up, pending confirmation of
correct thing to do for now at least).
- Bug 578675 Upgrade Mozmill on the builders - this will be done once a new
MozMill version is released.

Separate to those, bug 576746 covers the required Calendar fix ups.

Please file any issues not mentioned above but which may be resulting from this
change as separate bugs.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [bug fixed][see comment 108 for follow-up information]
Target Milestone: --- → Thunderbird 3.2a1
(In reply to comment #99)
> Created attachment 456714 [details] [diff] [review]
> mailnews/mime/cthandlers [Checked in Comment 104]
> 
> Do the leftover stuff in mailnews/ (I'm not certain when these are built, but
> figured it would be good to convert them)

(In reply to comment #104)
> Comment on attachment 456714 [details] [diff] [review]
> mailnews/mime/cthandlers [Checked in Comment 104]
> 
> http://hg.mozilla.org/comm-central/rev/5485ed369f42

Both me and bienvenu should be shamed... I *just* realized I never checked in, (or had reviewed) the manifest for directory/xpcom/datasource....

Sounds ominous, I'm going to file a new bug and figure out where I was going with that (I don't have anything substantive locally for it, except a hg-unknown file for the .manifest)
(In reply to comment #109)
 
> Both me and bienvenu should be shamed... I *just* realized I never checked in,
> (or had reviewed) the manifest for directory/xpcom/datasource....
> 

I don't think that directory gets built or used currently.
(In reply to comment #110)
> (In reply to comment #109)
> 
> > Both me and bienvenu should be shamed... I *just* realized I never checked in,
> > (or had reviewed) the manifest for directory/xpcom/datasource....
> > 
> 
> I don't think that directory gets built or used currently.

...Well I did check in its Makefile.in change, which is how I noticed.
Using address book fails the check added June 27 for bug 568691,

  http://hg.mozilla.org/mozilla-central/rev/4a2f9f03b589

  In generateNSGetFactory, classID missing or incorrect for component ...
(XPCOMUtils was not even visible in the first place).
Ilguiz: If you are referring to the fact that LDAP search in the address book is broken, then that is bug 585917.

If you are referring to something else, please file a follow-up bug - comments on closed bugs frequently get lost.

Also be aware that various changes and corrections were made after that attachment, so checking the latest source is needed as well.
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/c266a6c18236
Switch xpfe autocomplete to the new component registration r=bienvenu
You need to log in before you can comment on or make changes to this bug.