Closed
Bug 76339
Opened 23 years ago
Closed 23 years ago
get rid of nsIAppShellComponent
Categories
(SeaMonkey :: UI Design, defect, P3)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.6
People
(Reporter: alecf, Assigned: alecf)
References
Details
(Keywords: arch, qawanted)
Attachments
(5 files, 3 obsolete files)
4.24 KB,
patch
|
Details | Diff | Splinter Review | |
6.50 KB,
patch
|
dveditz
:
review+
|
Details | Diff | Splinter Review |
1.79 KB,
patch
|
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
12.08 KB,
patch
|
dveditz
:
review+
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
2.29 KB,
patch
|
dbradley
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•23 years ago
|
||
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
Assignee | ||
Comment 2•23 years ago
|
||
There are a number of people who #include this file because they really want nsAppShellCIDs.h: http://lxr.mozilla.org/seamonkey/search?string=nsiappshellcomponentimpl
Assignee | ||
Comment 3•23 years ago
|
||
Comment 4•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
Comment 5•23 years ago
|
||
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.
Assignee | ||
Comment 6•23 years ago
|
||
doh! Thanks. I'll fix that.
Assignee | ||
Comment 7•23 years ago
|
||
there's an AIM patch too, that I sent to syd...
Comment 8•23 years ago
|
||
sr=ben@netscape.com for the above patch.
Comment 9•23 years ago
|
||
sr=waterson
Assignee | ||
Comment 10•23 years ago
|
||
Comment 11•23 years ago
|
||
r/sr=sspitzer on the second patch (for mail). thanks for cleaning house for us, alecf.
Comment 12•23 years ago
|
||
I have removed the #include "nsIAppShellComponentImpl.h" from gfx/src/beos/nsDeviceContextSpecB.cpp. It wasn't being used, anyway. Checked in.
Assignee | ||
Comment 13•23 years ago
|
||
thanks wade!
Comment 14•23 years ago
|
||
nav triage: this is not a m0.9.2 stopper, moving to mozilla1.0.
Target Milestone: mozilla0.9.2 → mozilla1.0
Comment 15•23 years ago
|
||
nav triage team: Not a 0.9.4 stopper, leaving at mozilla1.0 for now
Assignee | ||
Comment 16•23 years ago
|
||
moving back onto my radar
Target Milestone: mozilla1.0 → mozilla0.9.6
Comment 17•23 years ago
|
||
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.
Assignee | ||
Comment 18•23 years ago
|
||
xpinstall includes it, but I forget if it's important. it's been a while :)
Comment 19•23 years ago
|
||
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.
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
anyone want to r=/sr=?
Assignee | ||
Updated•23 years ago
|
Attachment #33065 -
Attachment is obsolete: true
Assignee | ||
Comment 22•23 years ago
|
||
adding some super reviewers - anyone mind a quick sr= to remove this lame dependency?
Comment 23•23 years ago
|
||
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+
Assignee | ||
Comment 24•23 years ago
|
||
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
Comment 25•23 years ago
|
||
Do you also need to fix the accessproxy stuff? http://lxr.mozilla.org/seamonkey/source/extensions/access-builtin/accessproxy/ nsAccessProxyRegistration.cpp#50
Assignee | ||
Comment 26•23 years ago
|
||
yep, that's next..
Assignee | ||
Comment 27•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #49806 -
Attachment is obsolete: true
Assignee | ||
Comment 28•23 years ago
|
||
ignore that nsFTPConn patch - that's garbage from some previous stuff I was working on
Assignee | ||
Comment 29•23 years ago
|
||
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
Comment 30•23 years ago
|
||
- 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.
Assignee | ||
Comment 31•23 years ago
|
||
Assignee | ||
Comment 32•23 years ago
|
||
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 33•23 years ago
|
||
Comment on attachment 50630 [details] [diff] [review] slight update to address simons comments about nsFTPConn sr=sfraser
Attachment #50630 -
Flags: superreview+
Assignee | ||
Updated•23 years ago
|
Priority: P2 → P3
Comment 34•23 years ago
|
||
Comment on attachment 50627 [details] [diff] [review] what the heck, clean up all of xpinstall r=dveditz
Attachment #50627 -
Flags: review+
Assignee | ||
Comment 35•23 years ago
|
||
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
Comment 36•23 years ago
|
||
*** Bug 100441 has been marked as a duplicate of this bug. ***
Comment 37•23 years ago
|
||
Comment 38•23 years ago
|
||
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+
Assignee | ||
Comment 39•23 years ago
|
||
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...
Assignee | ||
Updated•23 years ago
|
Attachment #52630 -
Attachment is obsolete: true
Assignee | ||
Comment 40•23 years ago
|
||
Assignee | ||
Comment 41•23 years ago
|
||
all right, you KNOW you want to review this. Just think, no more "Unable to enumerator app shell components"!
Comment 42•23 years ago
|
||
Comment on attachment 53749 [details] [diff] [review] finally rip out nsIAppShellComponent! Oooh ya. sr=sfraser
Attachment #53749 -
Flags: superreview+
Updated•23 years ago
|
QA Contact: sairuh → nobody
Assignee | ||
Comment 43•23 years ago
|
||
adding pink for r= cuz I know how much he loves this warning..
Comment 44•23 years ago
|
||
Comment on attachment 53749 [details] [diff] [review] finally rip out nsIAppShellComponent! r=dveditz
Attachment #53749 -
Flags: review+
Assignee | ||
Comment 45•23 years ago
|
||
yah, checked in the patch. no more warning. there was lots of cheering.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 46•23 years ago
|
||
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.
Assignee | ||
Comment 47•23 years ago
|
||
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
Comment 48•23 years ago
|
||
> because there is some dumb mac project using it
Huh? QT != QuickTime.
Assignee | ||
Comment 49•23 years ago
|
||
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 :)
Comment 50•23 years ago
|
||
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.
Assignee | ||
Comment 51•23 years ago
|
||
I'll be my own mac buddy if I can get an open tree today or tomorrow.....:)
Comment 52•23 years ago
|
||
Comment 53•23 years ago
|
||
Comment on attachment 54044 [details] [diff] [review] QNX patch, uploaded using patched QNX build sr=jst
Attachment #54044 -
Flags: superreview+
Comment 54•23 years ago
|
||
Comment on attachment 54044 [details] [diff] [review] QNX patch, uploaded using patched QNX build r=dbradley
Attachment #54044 -
Flags: review+
Assignee | ||
Comment 55•23 years ago
|
||
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 ago → 23 years ago
Resolution: --- → FIXED
Comment 56•23 years ago
|
||
vrfy fixed (yes the tinderbox2 thing is bogus, but i'll try to get someone to fix it)
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
You need to log in
before you can comment on or make changes to this bug.
Description
•