Closed Bug 76339 Opened 23 years ago Closed 23 years ago

get rid of nsIAppShellComponent

Categories

(SeaMonkey :: UI Design, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.6

People

(Reporter: alecf, Assigned: alecf)

References

Details

(Keywords: arch, qawanted)

Attachments

(5 files, 3 obsolete files)

We don't need this API anymore now that we have the app-startup mechanism

the only two consumers left are:
- xpinstall (dveditz?)
- accessability (aaronl)

I'll try to get to this, but dveditz/aaronl if you get a chance I'd appreciate
the switch (and I'll sr= very quickly)

until we switch over, your component won't be started automatically by the
embedded apps.
oops, adding dveditz/aaronl to the bug for real. Guys, take a look at this.. the
goal is to remove nsAppShellComponentImpl.h from the tree.
Status: NEW → ASSIGNED
There are a number of people who #include this file because they really want
nsAppShellCIDs.h:

http://lxr.mozilla.org/seamonkey/search?string=nsiappshellcomponentimpl
Don't forget the commercial tree (which I put here because I know Alec is 
@netscape.com -- no need for Mozillians to shoot me). Talkback is an actual 
AppShellComponent, net2phone includes the header for the contractID, and AIM is 
infested with #include nsIAppShellComponentImpl.h
Keywords: meta
OS: Linux → All
Hardware: PC → All
Keywords: arch
Priority: -- → P2
Target Milestone: --- → mozilla0.9.1
I see errors when from allmakefiles.sh:

updating xpfe/components/sample/Makefile
can't read ../xpfe/components/sample/Makefile.in: No such file or directory
updating xpfe/components/sample/public/Makefile
can't read ../xpfe/components/sample/public/Makefile.in: No such file or
directory
updating xpfe/components/sample/src/Makefile
can't read ../xpfe/components/sample/src/Makefile.in: No such file or directory
updating xpfe/components/sample/resources/Makefile
can't read ..//resources/Makefile.in: No such file or directory

Seems that you forget to remove xpfe/components/sample/src from allmakefiles.sh
when you removed files.
doh! Thanks. I'll fix that.
there's an AIM patch too, that I sent to syd...
Keywords: meta
sr=waterson
Attached patch remove stuff from mail (obsolete) — Splinter Review
r/sr=sspitzer on the second patch (for mail).

thanks for cleaning house for us, alecf.
I have removed the #include "nsIAppShellComponentImpl.h" from gfx/src/beos/nsDeviceContextSpecB.cpp. It wasn't being used, anyway. Checked in.
thanks wade!
Target Milestone: mozilla0.9.1 → mozilla0.9.2
nav triage: this is not a m0.9.2 stopper, moving to mozilla1.0. 
Target Milestone: mozilla0.9.2 → mozilla1.0
nav triage team:

Not a 0.9.4 stopper, leaving at mozilla1.0 for now
moving back onto my radar
Target Milestone: mozilla1.0 → mozilla0.9.6
I think talkback might be the only user of this left, and it could really
benefit from being an appStartupNotifier. Since that's fired off quite a bit
earlier it would be able to catch profile startup crashes.
xpinstall includes it, but I forget if it's important. it's been a while :)
XPInstall isn't using it anymore, thanks to jst's external nameset registration
we no longer load xpinstall unless it's explicitly used. The headerfile can go.

Using Regexport, QFA is the only one registered in recent builds. AIM must have
switched to something else as well.
Blocks: 100107
Attached patch clean up xpinstall (obsolete) — Splinter Review
anyone want to r=/sr=?
Attachment #33065 - Attachment is obsolete: true
adding some super reviewers - anyone mind a quick sr= to remove this lame
dependency?
Comment on attachment 49806 [details] [diff] [review]
clean up xpinstall

r=dveditz for this patch.

Don't you want to nuke the #include nsIAppShellComponent.h (not ComponentImpl.h) in nsSoftwareUpdate.h too? If you do you have to fix up #defines for two contractID's which are based on the appshell contractID. I have no idea why, I don't care if you make up something different. I'd have thought "@mozilla.org/xpinstall..." would be good enough without having to stick "appshell/component" in between there.
Attachment #49806 - Flags: review+
I filed bugscape bug http://bugscape.mcom.com/show_bug.cgi?id=9552 about the
commercial tree. Looking for reviewers over there too.
Whiteboard: fix in hand
Target Milestone: mozilla0.9.6 → mozilla0.9.5
Do you also need to fix the accessproxy stuff?

http://lxr.mozilla.org/seamonkey/source/extensions/access-builtin/accessproxy/
nsAccessProxyRegistration.cpp#50
yep, that's next..
Attachment #49806 - Attachment is obsolete: true
ignore that nsFTPConn patch - that's garbage from some previous stuff I was
working on
Comment on attachment 50627 [details] [diff] [review]
what the heck, clean up all of xpinstall

wait, actually don't ignore that FTPConn thing - it just cleans up code, and I'd appreciate a review there too
Attachment #50627 - Attachment description: what the heck, clean up all of appshell → what the heck, clean up all of xpinstall
-    sprintf(cmd, "USER anonymous\r\n");
-    err = IssueCmd(cmd, resp, kRespBufSize, mCntlSock);
+    err = IssueCmd("USER anonymous\r\n", resp, kRespBufSize, mCntlSock);
 
     /* issue PASS command on control connection */
-    sprintf(cmd, "PASS -linux@installer.sbg\r\n");
-    ERR_CHECK(IssueCmd(cmd, resp, kRespBufSize, mCntlSock));
+    ERR_CHECK(IssueCmd("PASS -linux@installer.sbg\r\n", resp, kRespBufSize, 
mCntlSock));

These look a bit suspect. Is the first param to IssueCmd a const char*? If not, 
then IssueCmd would be allowed to change the string (strtok it or something), and 
you'd now crash.
simon: good point - so I changed the API slightly.. and then verified that there
was no funky casting going on. The sprintf was just a bad use of a pattern that
was used elsewhere where string substitution was actually used.
Comment on attachment 50630 [details] [diff] [review]
slight update to address simons comments about nsFTPConn

sr=sfraser
Attachment #50630 - Flags: superreview+
Priority: P2 → P3
Comment on attachment 50627 [details] [diff] [review]
what the heck, clean up all of xpinstall

r=dveditz
Attachment #50627 - Flags: review+
the only non-commercial consumer is accessibility.. moving out to 0.9.6 to
finish that up
Whiteboard: fix in hand
Target Milestone: mozilla0.9.5 → mozilla0.9.6
*** Bug 100441 has been marked as a duplicate of this bug. ***
Comment on attachment 52630 [details] [diff] [review]
Patch to make accessproxy use nsIAppStartupNotifer doesn't work - Observe is never called - alecf can you advise?

>+/*
> // This method gets called on application shutdown
> NS_IMETHODIMP nsAccessProxy::Shutdown()
> {
>   Release();
>   return NS_OK;
> }
>+*/

If you need this done you can set yourself up as an XPCOM shutdown observer.

>+  rv = categoryManager->AddCategoryEntry(APPSTARTUP_CATEGORY, "service", NS_ACCESSPROXY_CONTRACTID, 
>+    PR_TRUE, PR_TRUE, getter_Copies(previous));

This is your problem. You have no category item name, and the comma in
"service," leaked outside the quoted string. The service string is
concatenated to your contractid, it's not another argument. See for example

http://lxr.mozilla.org/mozilla/source/intl/chardet/src/nsCharDetModule.cpp#137
Attachment #52630 - Flags: needs-work+
Depends on: 104173
ok, I finally checked in the commercial build stuff, so now I can remove all of
nsIAppShellComponent!!

aaronl, I'm going to go ahead and remove it, since you've got accessproxy fixes
in your tree, and that stuff is not on by default...
Attachment #52630 - Attachment is obsolete: true
all right, you KNOW you want to review this. Just think, no more "Unable to
enumerator app shell components"!
Comment on attachment 53749 [details] [diff] [review]
finally rip out nsIAppShellComponent!

Oooh ya. sr=sfraser
Attachment #53749 - Flags: superreview+
QA Contact: sairuh → nobody
adding pink for r= cuz I know how much he loves this warning..
Comment on attachment 53749 [details] [diff] [review]
finally rip out nsIAppShellComponent!

r=dveditz
Attachment #53749 - Flags: review+
yah, checked in the patch. no more warning. there was lots of cheering.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
http://lxr.mozilla.org/mozilla/search?string=nsIAppShellComponent

Not completely removed, it looks like you just busted Qt, i'll try building
sometime w/in the next two weeks.
Blocks: qt
Status: RESOLVED → REOPENED
QA Contact: nobody → timeless
Resolution: FIXED → ---
we're not supporting QT. The only reason it's in the tree is because there is
some dumb mac project using it...
Status: REOPENED → ASSIGNED
> because there is some dumb mac project using it
Huh? QT != QuickTime.
no no.. the only reason I brought the file back from the Attic was to avoid
breaking mac.
we don't support Qt so I don't care if it uses nsIAppShellComponent :)
Well I see 4 problems:
1. you need a mac buddy to fix the mac project so it doesn't try to make a
header out of the idl you're trying to kill.
2. someone should remove references to the file from tinderbox2.
3. Qt the toolkit port (jcg owns) should be broken.
4. QNX the OS (*@qnx.com) should be broken.

I can try to help on 2-4. I've cc'd people for 1 and 4.
I'll be my own mac buddy if I can get an open tree today or tomorrow.....:)
Comment on attachment 54044 [details] [diff] [review]
QNX patch, uploaded using patched QNX build

sr=jst
Attachment #54044 - Flags: superreview+
Comment on attachment 54044 [details] [diff] [review]
QNX patch, uploaded using patched QNX build

r=dbradley
Attachment #54044 - Flags: review+
ok, turns out last night I inadvertently removed nsIAppShellComponent.idl from
the mac project - wasn't that convenient? :)
anyway, now I've removed it from the tree, along with the reference in IIDS.h...
that last and final reference in LXR is bogus, this bug should stay FIXED.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
vrfy fixed (yes the tinderbox2 thing is bogus, but i'll try to get someone to
fix it)
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: