Closed
Bug 373397
Opened 17 years ago
Closed 16 years ago
nsGNOMERegistry could use the mozgnome component
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9beta3
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 7 obsolete files)
16.20 KB,
patch
|
Biesinger
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
As said in comment #33 on bug #273524, nsGNOMERegistry could be rewritten to use the nsGConfService and the nsGnomeVFSService provided in the mozgnome component. That would make the code cleaner, and the dynamic loading of libraries useless. Patch to bug #273524 could be adapted to use it too, but that would require some additions to the nsGnomeVFSService. The patch I'm attaching implements this, though I'm not really sure about myself for some points such as nsGnomeVFSMimeApp being correctly auto-destroyed, or how to (cleanly) make sure headers from toolkit/components/gnome are exported before building uriloader/exthandler.
Attachment #258051 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 1•17 years ago
|
||
Comment on attachment 258051 [details] [diff] [review] patch The patch is against 1.8.0 branch, but I guess it can apply as is on all branches.
Comment 2•17 years ago
|
||
Comment on attachment 258051 [details] [diff] [review] patch I'm sorry, but I really don't know the GNOME code well enough to review. Bryner's the right man for this... Or possibly biesi if he knows this stuff.
Attachment #258051 -
Flags: review?(bzbarsky) → review?(bryner)
Assignee | ||
Comment 3•17 years ago
|
||
nsIGnomeVFSService::GetMimeTypeFromExtension already adds a "." before the extension, so it's not necessary to do it
Assignee: file-handling → mh+mozilla
Attachment #258051 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #258110 -
Flags: review?(bryner)
Attachment #258051 -
Flags: review?(bryner)
Updated•17 years ago
|
Attachment #258110 -
Flags: superreview?(cbiesinger)
Attachment #258110 -
Flags: review?(chpe)
Attachment #258110 -
Flags: review?(bryner)
Comment 4•17 years ago
|
||
Comment on attachment 258110 [details] [diff] [review] patch Unfortunately the patch has bitrot, it doesn't apply cleanly anymore. +gconf->GetAppForProtocol(nsDependentCString(aProtocolScheme), &isEnabled, handler); [...] >+ return isEnabled; You need to check the rv of GetAppForProtocol here. >+ PRBool isEnabled; >+ nsCAutoString handler; >+ gconf->GetAppForProtocol(aScheme, &isEnabled, handler); And here too. >+ nsCAutoString name; >+ handlerApp->GetName(name); >+ mimeInfo->SetDefaultDescription(NS_ConvertUTF8toUCS2(name)); NS_ConvertUTF8toUTF16.
Attachment #258110 -
Flags: review?(chpe) → review-
Updated•17 years ago
|
Attachment #258110 -
Flags: superreview?(cbiesinger)
Assignee | ||
Comment 5•17 years ago
|
||
New patch against trunk, with modifications suggested in comment #4. Not tested.
Attachment #258110 -
Attachment is obsolete: true
Attachment #284755 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #284755 -
Flags: review? → review?(chpe)
Comment 6•17 years ago
|
||
I'm a bit concerned about this change: +nsGNOMERegistry::GetFromExtension(const nsACString& aFileExt) [...] - nsCAutoString fileExtToUse; - if (aFileExt && aFileExt[0] != '.') - fileExtToUse = '.'; - - fileExtToUse.Append(aFileExt); - - const char *mimeType = _gnome_vfs_mime_type_from_name(fileExtToUse.get()); - if (!strcmp(mimeType, "application/octet-stream")) + nsCAutoString mimeType; + vfs->GetMimeTypeFromExtension(aFileExt, mimeType); and nsGnomeVFSService::GetMimeTypeFromExtension has this; nsCAutoString fileExtToUse("."); fileExtToUse.Append(aExtension); const char *mimeType = gnome_vfs_mime_type_from_name(fileExtToUse.get()); So for aFileExt = "", the old code searched for "" ext, while the new code searches for "." . This code is called from nsExternalHelperAppService::GetFromTypeAndExtension which _is_ called with "" in some places. It's not clear from the docs that those will give the same results; can you test this?
Comment 7•17 years ago
|
||
Hmm actually I misread that; the problem is if aFileExt already starts with a dot; then the new code will look for "..ext".
Assignee | ||
Comment 8•17 years ago
|
||
Would you be okay to modify nsGnomeVFSService::GetMimeTypeFromExtension so that it doesn't add the "." if aExtension already begins with one ?
Comment 9•17 years ago
|
||
I guess my question is whether nsGNOMERegistry::GetFromExtension is (or is supposed to be) called with "ext" or ".ext". If ".ext" ever occurs, we should fix that in nsGnomeVFSService::GetMimeTypeFromExtension, yes.
Updated•17 years ago
|
Version: 1.8 Branch → Trunk
Assignee | ||
Comment 10•17 years ago
|
||
Reading the code, it pretty much seems to be only about "ext" and not ".ext", but i'd prefer someone that actually knows the code to confirm. I don't remember if I wrote the patch with that in mind or not, as it was more than 6 months ago.
Comment 11•17 years ago
|
||
Extensions passed to GetFromExtension should not have a leading dot, no.
So the patch is ok then, except maybe we should add an assertion that |aFileExt| does not start with a dot.
Comment on attachment 284755 [details] [diff] [review] Updated patch r=me with the nit in comment 12 fixed.
Attachment #284755 -
Flags: review?(chpe) → review+
Assignee | ||
Comment 14•17 years ago
|
||
Do you want me to send an updated patch ?
Comment 15•17 years ago
|
||
(In reply to comment #14) > Do you want me to send an updated patch ? Yes, please.
Assignee | ||
Comment 16•17 years ago
|
||
Added the assertion discussed in comment #12. Also, this one is against today's trunk (only line numbers updates)
Attachment #284755 -
Attachment is obsolete: true
Attachment #289957 -
Flags: review?(chpe)
Updated•17 years ago
|
Attachment #289957 -
Flags: superreview?(cbiesinger)
Comment 17•17 years ago
|
||
Comment on attachment 289957 [details] [diff] [review] Slightly updated patch + aDesc.Assign(NS_ConvertUTF8toUCS2(handler)); UTF16, not UCS2 how did that compile? and in fact it should be: CopyUTF8toUTF16(handler, aDesc); - mimeInfo->AppendExtension(nsDependentCString((const char *) extension->data)); you don't have a replacement for this?
Updated•17 years ago
|
Attachment #289957 -
Flags: superreview?(cbiesinger) → superreview+
Assignee | ||
Comment 18•17 years ago
|
||
(In reply to comment #17) > (From update of attachment 289957 [details] [diff] [review]) > + aDesc.Assign(NS_ConvertUTF8toUCS2(handler)); > > UTF16, not UCS2 > > how did that compile? > > > and in fact it should be: > > CopyUTF8toUTF16(handler, aDesc); Should I update the patch ? > - mimeInfo->AppendExtension(nsDependentCString((const char *) > extension->data)); > > you don't have a replacement for this? IIRC (I kind of not clearly remember, 9 months after the facts), that's because the gnome registry doesn't use this information.
Comment 19•17 years ago
|
||
(In reply to comment #18) > Should I update the patch ? Yes, you need to.
Assignee | ||
Comment 20•17 years ago
|
||
Updated according to comment #17, and also updated to fit to latest trunk (there were changes in nsGNOMERegistry::GetAppDescForScheme). These changes have not been tested, I won't have time to do so for a few days.
Attachment #289957 -
Attachment is obsolete: true
Attachment #291839 -
Flags: superreview?(cbiesinger)
Attachment #291839 -
Flags: review?(chpe)
Attachment #289957 -
Flags: review?(chpe)
Comment 21•17 years ago
|
||
Mike, if you believe you've addressed both chpe and biesi's comments, there's no need to get review again, as they already granted you review with the understanding you would fix the problems they found. If you'll just test it to make sure it works as expected, then we can move forward on getting this landed once the tree reopens from the b2 freeze.
Assignee | ||
Comment 22•17 years ago
|
||
(In reply to comment #21) > Mike, if you believe you've addressed both chpe and biesi's comments, there's > no need to get review again, as they already granted you review with the > understanding you would fix the problems they found. If you'll just test it to > make sure it works as expected, then we can move forward on getting this landed > once the tree reopens from the b2 freeze. I'd prefer the additional code to be reviewed, as I'm not yet very familiar with the ns*String API.
Assignee | ||
Updated•17 years ago
|
Attachment #291839 -
Attachment is obsolete: true
Attachment #291839 -
Flags: superreview?(cbiesinger)
Attachment #291839 -
Flags: review?(chpe)
Assignee | ||
Comment 23•17 years ago
|
||
I'm currently doing a build, and had to fix a few issues: - Missing parenthesis in 2 places in nsGNOMERegistry.cpp - The gnome component directory changed so this needed a change in Makefile.in I'll test if it works properly at runtime later today.
Assignee | ||
Comment 24•17 years ago
|
||
This is a revised patch, fixing all issues that raised when testing the previous patch. The newly added code in nsGNOMERegistry::GetAppDescForScheme used Cut quite badly, and there was a null pointer dereference possibility in nsGNOMERegistry::GetFromType.
Attachment #292145 -
Flags: superreview?(cbiesinger)
Attachment #292145 -
Flags: review?(chpe)
Comment 25•17 years ago
|
||
(In reply to comment #18) > > - mimeInfo->AppendExtension(nsDependentCString((const char *) > > extension->data)); > > > > you don't have a replacement for this? > > IIRC (I kind of not clearly remember, 9 months after the facts), that's because > the gnome registry doesn't use this information. Well, that kind of information is used by other code...
Comment 26•17 years ago
|
||
Comment on attachment 292145 [details] [diff] [review] New patch + $(MAKE) -C $(DEPTH)/toolkit/system/gnome export so, why's that needed? The headers are in xpcom/system, right? (so I also don't think the REQUIRES change is correct) app.Cut(firstSpace, app.Length() - firstSpace); that should be app.Truncate(firstSpace); + ! handlerApp) don't put a space after unary operators
Attachment #292145 -
Flags: superreview?(cbiesinger) → superreview-
Assignee | ||
Comment 27•17 years ago
|
||
(In reply to comment #25) > > IIRC (I kind of not clearly remember, 9 months after the facts), that's because > > the gnome registry doesn't use this information. > > Well, that kind of information is used by other code... Actually, if my cheese memory is not wrong, the gnome vfs api doesn't provide the information any more... (and hasn't been for a very long time) (In reply to comment #26) > (From update of attachment 292145 [details] [diff] [review]) > + $(MAKE) -C $(DEPTH)/toolkit/system/gnome export > > so, why's that needed? The headers are in xpcom/system, right? (so I also don't > think the REQUIRES change is correct) I must say I've not checked if the requirements for this are still the same. It was needed before, I just ported it that way, and it builds fine. At the time, it was required so that the gnome component headers would be generated before building this, which was not the case because the gnome registry was in a lesser tier than the gnome component. I don't know if this is still true.
Comment 28•17 years ago
|
||
(In reply to comment #27) > Actually, if my cheese memory is not wrong, the gnome vfs api doesn't provide > the information any more... (and hasn't been for a very long time) Ah, OK. > (In reply to comment #26) > I must say I've not checked if the requirements for this are still the same. It > was needed before, I just ported it that way, and it builds fine. At the time, > it was required so that the gnome component headers would be generated before > building this, which was not the case because the gnome registry was in a > lesser tier than the gnome component. I don't know if this is still true. Well, lower tiers should not depend on higher tiers. So please remove those lines, and if they were still required then it needs some other fix.
Comment 29•16 years ago
|
||
Mike, can you attach an updated patch, please?
Assignee | ||
Comment 30•16 years ago
|
||
I finally had time to finalize this patch. I applied changes suggested in comment #26. It appears no change to Makefile was necessary at all, everything builds fine without the hackish rules that were necessary before.
Attachment #292145 -
Attachment is obsolete: true
Attachment #299255 -
Flags: approval1.9?
Attachment #292145 -
Flags: review?(chpe)
Comment 31•16 years ago
|
||
Where is this patch carrying it's review over from? Does it need a re-review?
Comment on attachment 299255 [details] [diff] [review] Final patch + nsCAutoString mimeType; + vfs->GetMimeTypeFromExtension(aFileExt, mimeType); Shouldn't assume that this can't fail (the current impl. doesn't, but that's not guaranteed by the contract). + if (mimeType.Equals("application/octet-stream")) EqualsLiteral.
Attachment #299255 -
Flags: review+
Assignee | ||
Comment 33•16 years ago
|
||
It should be okay this time
Attachment #299255 -
Attachment is obsolete: true
Attachment #299290 -
Flags: review?(chpe)
Attachment #299255 -
Flags: approval1.9?
Assignee | ||
Updated•16 years ago
|
Attachment #299290 -
Flags: superreview?(cbiesinger)
Updated•16 years ago
|
QA Contact: ian → file-handling
Updated•16 years ago
|
Attachment #299290 -
Flags: superreview?(cbiesinger) → superreview+
Comment 34•16 years ago
|
||
Comment on attachment 299290 [details] [diff] [review] Patch Makes nsGNOMERegistry use the nsGConfService and the nsGnomeVFSService provided in the mozgnome component, which makes the code cleaner and the dynamic loading of libraries useless, which is much better than now.
Attachment #299290 -
Flags: review?(chpe) → approval1.9?
Comment 35•16 years ago
|
||
Comment on attachment 299290 [details] [diff] [review] Patch a=beltzner
Attachment #299290 -
Flags: approval1.9? → approval1.9+
Updated•16 years ago
|
Keywords: checkin-needed
Comment 36•16 years ago
|
||
Checking in uriloader/exthandler/unix/nsGNOMERegistry.cpp; /cvsroot/mozilla/uriloader/exthandler/unix/nsGNOMERegistry.cpp,v <-- nsGNOMERegistry.cpp new revision: 1.18; previous revision: 1.17 done Checking in uriloader/exthandler/unix/nsGNOMERegistry.h; /cvsroot/mozilla/uriloader/exthandler/unix/nsGNOMERegistry.h,v <-- nsGNOMERegistry.h new revision: 1.4; previous revision: 1.3 done Checking in uriloader/exthandler/unix/nsOSHelperAppService.cpp; /cvsroot/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp,v <-- nsOSHelperAppService.cpp new revision: 1.72; previous revision: 1.71 done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta3
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•