Closed
Bug 489864
Opened 15 years ago
Closed 15 years ago
get rid of nsIInternetConfigService
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jaas, Assigned: smichaud)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(5 files, 6 obsolete files)
1.60 KB,
patch
|
standard8
:
review+
standard8
:
superreview+
|
Details | Diff | Splinter Review |
1.55 KB,
text/plain
|
Details | |
4.57 KB,
text/plain
|
Details | |
853 bytes,
text/plain
|
Details | |
88.85 KB,
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
nsIInternetConfigService is an interface built around a deprecated and not-awesome Mac system API. We should get rid of it.
This does it, haven't tested thoroughly yet so not requesting review yet. Drops 1300 lines of deprecated junk. The only behavioral change worth noting is that we won't ever set type codes on downloaded files with this patch. I'd rather not do that anyway. We only did that for files that didn't have an extension and had mime types we could convert to type codes, which we can't figure out reliably anyway and it isn't our problem. There was a bunch of discussion about this in bug 236389, more time has passed and type codes are even more outdated than they were then.
This also fixes bug 488899.
Attachment #374358 -
Flags: review?(smichaud)
Assignee | ||
Comment 3•15 years ago
|
||
Comment on attachment 374358 [details] [diff] [review] fix v1.0 I haven't finished my review yet. But I've noticed that there are a bunch of files under mailnews (in the comm-central tree) that reference nsMIMEInfo::GetMacType(), nsMIMEInfo::SetMacType(), nsMIMEInfo::GetMacCreator() or nsMIMEInfo::SetMacCreator(): mailnews/base/src/nsMessenger.cpp mailnews/compose/src/nsMsgAttachment.cpp mailnews/compose/src/nsMsgSend.cpp mailnews/mime/src/mimedrft.cpp So this patch will presumably break Thunderbird and/or Seamonkey builds. For that reason I'm 'r-'ing it.
Attachment #374358 -
Flags: review?(smichaud) → review-
Comment on attachment 374358 [details] [diff] [review] fix v1.0 Yes, I know they use it. I'll help them stop using it if this is going to land on mozilla-central.
Attachment #374358 -
Flags: review- → review?
Attachment #374358 -
Flags: review? → review?(smichaud)
Assignee | ||
Comment 5•15 years ago
|
||
Note that there are some official builds (or at least semi-official builds) of Thunderbird and/or Seamonkey that use mozilla-central. These builds would be broken by this patch landing on trunk (aka mozilla-central). See bug 489659.
I know but I'm not going to do anything with those products unless this patch is ready, so we need to review this first. I don't want to do work with those products and then throw it all out or change it if this is a no go for some reason.
Assignee | ||
Updated•15 years ago
|
Attachment #374358 -
Flags: review?(smichaud)
Assignee | ||
Comment 7•15 years ago
|
||
I've already mentioned above (in comment #3) one issue I have with Josh's patch. I'll address that myself in a subsequent patch (I hope I can post it here tomorrow). I also think nsOSHelperAppService::GetMIMEInfoFromOS() can do a better job of gleaning MIME information from the OS, so I've rewritten it. Aside from that, my v2.0 patch is just an update of Josh's patch for current trunk code. My revised GetMIMEInfoFromOS() is more careful about checking for a default application (it uses both the MIME type and the extension to look for it). And it takes into account the possibility that aMIMEType and aFileExt won't match (as happens with bug 488899). It also sets several additional nsMIMEInfo fields that used to be set in nsInternetConfigService::FillMIMEInfoForICEntry(). A tryserver build will follow in a few hours.
Attachment #376320 -
Flags: review?(joshmoz)
Assignee | ||
Comment 8•15 years ago
|
||
Here's a tryserver build made with my v2.0 patch: https://build.mozilla.org/tryserver-builds/2009-05-07_14:51-smichaud@pobox.com-bugzilla489864-v2.0/smichaud@pobox.com-bugzilla489864-v2.0-firefox-try-mac.dmg
Attachment #376320 -
Flags: review?(joshmoz) → review-
Comment on attachment 376320 [details] [diff] [review] v2.0 patch: More thorough GetMIMEInfoFromOS() + CFStringRef CFAppName = NULL; This will leak if LSCopyItemAttribute sets it. Fix that and then this looks good.
Assignee | ||
Comment 10•15 years ago
|
||
Here's a revision that fixes the leak (and also checks if CFAppName is NULL). Thanks for catching the leak, Josh.
Attachment #376320 -
Attachment is obsolete: true
Attachment #376461 -
Flags: review?(joshmoz)
Attachment #376461 -
Flags: superreview?(roc)
Attachment #376461 -
Flags: review?(joshmoz)
Attachment #376461 -
Flags: review+
Assignee | ||
Comment 11•15 years ago
|
||
Mark, this bug's patch (which removes Internet Config Services) will break the Thunderbird build that uses mozilla-central (as part of comm-central), thanks to a mailnews dependency on nsMIMEInfo::GetMacType() and nsMIMEInfo::GetMacCreator(). But that dependency is very minor, and I think unnecessary. This patch removes it.
Attachment #376494 -
Flags: review?(bugzilla)
Comment on attachment 376461 [details] [diff] [review] v2.1 patch (fix leak) + * Furthermore WebKit has three public methods (in WebKitSystemInterface.h) + * that are thin wrappers around this interface's last three methods. Please file a bug with Apple to get this interface documented. If they don't, we should escalate it.
Attachment #376461 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 13•15 years ago
|
||
Landed on mozilla-central (trunk): http://hg.mozilla.org/mozilla-central/rev/add33a95e3ef
Assignee | ||
Comment 14•15 years ago
|
||
> Please file a bug with Apple to get this interface documented. If they don't,
> we should escalate it.
I'll get to this as soon as I can (probably later this week).
Comment 15•15 years ago
|
||
(In reply to comment #11) > Created an attachment (id=376494) [details] > Get rid of mailnews dependency on nsMIMEInfo::GetMacType and GetMacCreator ... > But that dependency is very minor, and I think unnecessary. > > This patch removes it. I don't understand what MacType and MacCreator are, this code seems to have been added by bug 122815 which looks like a OS X version 9 bug. Is that why we can now remove this code, because we don't need it for 10.x? I guess that would make bug 123630 invalid as well?
Assignee | ||
Comment 16•15 years ago
|
||
(In reply to comment #15) > I don't understand what MacType and MacCreator are They're Mac-specific, and very old. OS X still supports them, but even Apple says extensions should be given precedence over the creator and/or type (see bug 123630 comment #6). > this code seems to have been added by bug 122815 which looks like a > OS X version 9 bug. Is that why we can now remove this code, because > we don't need it for 10.x? We shouldn't need this code for OS X. There may still be subtle dependencies on an attachment's 'creator' or 'type', in the Mozilla tree, in other programs (that might load the attachments), or even in Apple code. I think that's unlikely ... but we should keep our eyes open. The main issue of bug 122815 was fixed by Apple long ago -- OS X supports file/directory names far longer than 31 characters. > I guess that would make bug 123630 invalid as well? I think so (especially in light of bug 123630 comment #6). But I can't really follow everything in bug 123630.
Assignee | ||
Comment 17•15 years ago
|
||
Note that my revisions to nsOSHelperAppService::GetMIMEInfoFromOS() should do a better job that previous code of setting an appropriate extension when the original filename doesn't have one.
The patch that just landed for this bug is causing talos crashes and should be backed out or fixed.
Assignee | ||
Comment 19•15 years ago
|
||
Backed out: http://hg.mozilla.org/mozilla-central/rev/aa3a28d8eb76 I'll make sure tests pass locally before resubmitting a revised version :-(
Assignee | ||
Comment 20•15 years ago
|
||
This is work in progress. Due to test flakiness, I still haven't been able to get through a run of mochitests (with or without the patch).
Comment 22•15 years ago
|
||
Comment on attachment 376494 [details] [diff] [review] Get rid of mailnews dependency on nsMIMEInfo::GetMacType and GetMacCreator Ok, this looks reasonable, I can't see it doing anything in particular. r=Standard8
Attachment #376494 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 23•15 years ago
|
||
(In reply to comment #22) Who should do the superreview? Do you want to do the honors?
Reporter | ||
Comment 24•15 years ago
|
||
Reporter | ||
Comment 25•15 years ago
|
||
Fixes unit test failure. The problem is that the MIME structure is expected to have an empty string for a description when the MIME is bogus/unknown. Mac OS X gives generic descriptions like "Document" when it doesn't recognize the MIME type, so only ask if an application was actually found for the MIME type.
Attachment #374358 -
Attachment is obsolete: true
Attachment #376461 -
Attachment is obsolete: true
Attachment #376928 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #376494 -
Flags: superreview+
Reporter | ||
Comment 26•15 years ago
|
||
There is another tbox unit test failure holding up this patch, I can't reproduce locally.
Assignee | ||
Comment 27•15 years ago
|
||
I see consistent local failures in the following Mochitests, on two different machines (one running OS X 10.5.7 and the other running 10.4.11): content/media/video/test/test_audio1.html content/media/video/test/test_audio2.html content/media/video/test/test_bug461281.html toolkit/content/tests/widgets/test_videocontrols.html I'll be trying to uncover the reasons for them. Josh, are these the failures you saw?
Reporter | ||
Comment 28•15 years ago
|
||
Those aren't the failures I was seeing. I can't find a log with the right failures though.
Assignee | ||
Comment 29•15 years ago
|
||
Here's another revision, which deals with the mochitest failures listed in comment #27 and an unrelated chrome test failure. Both sets of failures are partially caused by Apple bugs. But they can be easily worked around by making nsOSHelperAppService::GetMIMEInfoFromOS() stricter -- by making it check more thoroughly for problems, and having it then set *aFound to PR_FALSE and return only a minimal nsIMIMEInfo structure. Mochitest Failures from Comment #27 Turns out these are triggered by having RealPlayer installed. RealPlayer registers itself with Launch Services to handle the "ogg" extension and the audio/x-ogg and application/x-ogg MIME types. But [NSURLFileTypeMappings MIMETypeForExtension:] returns nil when called with the "ogg" extension (this is the Apple bug). In previous code (v2.2 and v2.3) this caused GetMIMEInfoFromOS() to return an nsIMIMEInfo structure with a null MIME type, while still setting *aFound to PR_TRUE. This is what caused all the comment #27 test failures. Chrome Test Failure toolkit/mozapps/downloads/tests/chrome/test_unkownContentType_dialog_layout.xul On OS X 10.5.7, TextEdit gets registered as the handler for the application/octet-stream MIME type (indirectly, as a result of being registered as the handler for the public.data UTI). Possibly not quite a bug, but definitely weird. During the unknownContentType_dialog_layout tests, GetMIMEInfoFromOS() gets called twice with aFileExt set to "pif" (a deliberately invalid, imaginary extension). The first of these calls makes aMIMEType blank; the other sets it to "application/octet-stream". Previous code sets *aFound to PR_TRUE on the second call (since, on OS X 10.5.7, LSGetApplicationForInfo() does tell us that the application/octet-stream MIME type has a default application). This causes failures in two out of the four unknownContentType_dialog_layout tests. Current code sets *aFound to PR_FALSE even on the second call (since aMIMEType and aFileExt don't match), and makes all the unknownContentType_dialog_layout tests pass.
Attachment #377265 -
Attachment is obsolete: true
Assignee | ||
Comment 30•15 years ago
|
||
My v2.4 patch passes all tests locally. But Josh mentions he's seen some failures (presumably on the tryservers) that he didn't see locally. I'm trying to get meaningful data from the tryservers ... but they take so long (and are so flaky) that this isn't easy. I'm now waiting for the results of my current round's "OS X 10.5.2 try hg unit test". I'll say more when it comes in.
Assignee | ||
Comment 31•15 years ago
|
||
Here's a log of the single legitimate (or seemingly legitimate) test failure that happens on the tryservers -- in the xpcshell test_handlerService.js test on the "OS X 10.5.2 try hg unit test" machine. I've now seen this failure (on the tryservers) three times over the last three days. I don't see it in other people's logs (on the same machine) from around the same times. I can't reproduce it locally, but the log provides enough information that I might be able to figure out what's going on.
Assignee | ||
Comment 32•15 years ago
|
||
I just realized that this is probably the same failure as Josh posted in comment #24. Josh thought he'd fixed that. I guess it must also have another cause. Josh, I assume this isn't the same failure you mention in comment #26.
Assignee | ||
Comment 33•15 years ago
|
||
Assignee | ||
Comment 34•15 years ago
|
||
(Following up comment #31) By sheer good luck, I'm able to reproduce this error locally on a partition to which I just installed OS X 10.5.1. So it shouldn't be too hard to figure out.
Assignee | ||
Updated•15 years ago
|
Assignee: joshmoz → smichaud
Assignee | ||
Comment 35•15 years ago
|
||
This new revision fixes the failure in test_handlerService.js. It's also caused by an Apple bug, but that was very easy to work around. Turns out the failure has nothing to do with which version of OS X is installed. Rather, it happens on systems where the default web browser has never been explicitly set (programmatically or from the UI). On a newly installed copy of OS X, Safari is always the default web browser. But this is (presumably) a characteristic of the Launch Services database copied over by the installer, and not the result of any explicit settings change. So (and here's the Apple bug) LSCopyDefaultHandlerForURLScheme() (in nsOSHelperAppService::OSProtocolHandlerExists()) doesn't find Safari when called with aProtocolScheme == "http". And so test_handlerService.js fails at line 155 ("do_check_false(protoInfo.alwaysAskBeforeHandling);"). I'm waiting for the results of the "OS X 10.5.2 try hg unit test" tryserver build. This should come in a few hours ... and hopefully will confirm that I've fixed the test_handlerService.js failure :-)
Attachment #383983 -
Attachment is obsolete: true
Assignee | ||
Comment 36•15 years ago
|
||
The "OS X 10.5.2 try hg unit test" just finished, and the test_handlerService.js test passed. (There was a leak, but it was clearly spurious: The same leak also happened during the two previous runs of the "OS X 10.5.2 try hg unit test".)
Assignee | ||
Updated•15 years ago
|
Attachment #384174 -
Flags: review?(joshmoz)
Assignee | ||
Comment 37•15 years ago
|
||
> The "OS X 10.5.2 try hg unit test" just finished, and the > test_handlerService.js test passed. > > (There was a leak, but it was clearly spurious: The same leak also > happened during the two previous runs of the "OS X 10.5.2 try hg > unit test".) Oops, I was looking at the wrong machine. The "OS X 10.5.2 try hg unit test" finished without any problems at all!
Reporter | ||
Comment 38•15 years ago
|
||
Comment on attachment 384174 [details] [diff] [review] Fix v2.5 (fix test_handlerService.js failure) Thanks for figuring this stuff out!
Attachment #384174 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 39•15 years ago
|
||
> Thanks for figuring this stuff out!
You're quite welcome.
Does this need sr? One interface did get changed (nsIMIMEInfo), and
another got dropped altogether (nsIInternetConfigService).
Reporter | ||
Comment 40•15 years ago
|
||
No interfaces have changed since last time this got sr, iirc, so that sr is still valid.
Assignee | ||
Comment 41•15 years ago
|
||
OK, thanks. I'll land both my patch from comment #11 (suitably updated) and the v2.5 patch.
Assignee | ||
Comment 42•15 years ago
|
||
Comment on attachment 376494 [details] [diff] [review] Get rid of mailnews dependency on nsMIMEInfo::GetMacType and GetMacCreator Landed on comm-central: http://hg.mozilla.org/comm-central/rev/16d18598cb19
Assignee | ||
Comment 43•15 years ago
|
||
Comment on attachment 384174 [details] [diff] [review] Fix v2.5 (fix test_handlerService.js failure) Landed on trunk (mozilla-central): http://hg.mozilla.org/mozilla-central/rev/2816cca3be16
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 44•15 years ago
|
||
Improvement: Ts Shutdown decrease 40.96% on Leopard Firefox Previous results: 288.211 from build 20090623110728 of revision 0cb931e6aaa7 at 2009-06-23 11:07:00 on talos-rev2-leopard13 run # 0 New results: 170.158 from build 20090623121841 of revision 2816cca3be16 at 2009-06-23 12:18:00 on talos-rev2-leopard05 run # 0 Is that for real?
Assignee | ||
Comment 45•15 years ago
|
||
> Improvement: Ts Shutdown decrease 40.96% on Leopard Firefox > Is that for real? It wasn't intentional :-) And there's no obvious reason this patch should make any difference. Has the change persisted? Do you see it on more than one machine? If the answer to both questions is "yes", I'll dig further. It's possible I'll uncover some sort of bug (e.g. lots of calls to the helper app service on shutdown).
Comment 46•15 years ago
|
||
I don't know, that was an automated message from catlee's regression finder script: http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/9604368e2a2487e1# Note that that's a claimed 40% *improvement* in shutdown time on Leopard, from 288.211ms to 170.158ms.
Assignee | ||
Comment 47•15 years ago
|
||
> that was an automated message from catlee's regression finder script I didn't know about that. Thanks. > Note that that's a claimed 40% *improvement* in shutdown time on > Leopard, from 288.211ms to 170.158ms. Yes. I wasn't complaining :-) Such a dramatic change needs explaining, though ... if it's for real.
Comment 48•15 years ago
|
||
The graph looks pretty real, and the same drop isn't on the Tiger graphs: http://graphs-new.mozilla.org/graph.html#tests=[{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%2263%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%2264%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%2265%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%2266%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22168%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22169%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22170%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22171%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22172%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22173%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22174%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22175%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22176%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22177%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22178%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22179%22},{%22test%22:%2236%22,%22branch%22:%221%22,%22machine%22:%22180%22}]&sel=1245698280,1245871080
Assignee | ||
Comment 49•15 years ago
|
||
Ted, your link points to the wrong graph. But I've found the Ts Shutdown improvement on two Leopard machines running talos tests on mozilla-central -- talos-rev2-leopard09 (aka MacOSX Darwin 9.0.0 mozilla-central talos) and talos-rev2-leopard05 (aka MacOSX Darwin 9.0.0 mozilla-central talos nochrome). Like you, I don't see it on the Tiger machines running talos tests on mozilla-central. I'll look further into this when I have time -- maybe later this week.
Assignee | ||
Comment 50•15 years ago
|
||
I wonder if my patch has somehow wrongly been credited with the Ts Shutdown improvement. The next patch to land (http://hg.mozilla.org/mozilla-central/rev/7a8502b70fdf) seems more likely ... at least to me.
Assignee | ||
Comment 51•15 years ago
|
||
(Following up comment #51) Though the other patch (changeset 7a8502b70fdf) isn't Mac-specific, which means it's unlikely to have a different effect on Leopard than it does on Tiger.
Updated•15 years ago
|
Keywords: dev-doc-needed
Updated•15 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
(In reply to comment #12) > (From update of attachment 376461 [details] [diff] [review]) > + * Furthermore WebKit has three public methods (in WebKitSystemInterface.h) > + * that are thin wrappers around this interface's last three methods. > > Please file a bug with Apple to get this interface documented. If they don't, > we should escalate it. (In reply to comment #14) > > Please file a bug with Apple to get this interface documented. If they don't, > > we should escalate it. > > I'll get to this as soon as I can (probably later this week). Did this rdar:// ever get filed?
You need to log in
before you can comment on or make changes to this bug.
Description
•