Closed Bug 373397 Opened 17 years ago Closed 16 years ago

nsGNOMERegistry could use the mozgnome component

Categories

(Core Graveyard :: File Handling, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 7 obsolete files)

Attached patch patch (obsolete) — 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)
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 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)
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #258110 - Flags: superreview?(cbiesinger)
Attachment #258110 - Flags: review?(chpe)
Attachment #258110 - Flags: review?(bryner)
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-
Attachment #258110 - Flags: superreview?(cbiesinger)
Attached patch Updated patch (obsolete) — Splinter Review
New patch against trunk, with modifications suggested in comment #4. Not tested.
Attachment #258110 - Attachment is obsolete: true
Attachment #284755 - Flags: review?
Attachment #284755 - Flags: review? → review?(chpe)
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?

Hmm actually I misread that; the problem is if aFileExt already starts with a dot; then the new code will look for "..ext". 
Would you be okay to modify nsGnomeVFSService::GetMimeTypeFromExtension so that it doesn't add the "." if aExtension already begins with one ?
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.
Version: 1.8 Branch → Trunk
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.
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+
Do you want me to send an updated patch ?
(In reply to comment #14)
> Do you want me to send an updated patch ?

Yes, please.

Attached patch Slightly updated patch (obsolete) — Splinter Review
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)
Attachment #289957 - Flags: superreview?(cbiesinger)
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?
Attachment #289957 - Flags: superreview?(cbiesinger) → superreview+
(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.
(In reply to comment #18)
> Should I update the patch ?

Yes, you need to.
Attached patch New patch against trunk (obsolete) — Splinter Review
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)
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.
(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.

Attachment #291839 - Attachment is obsolete: true
Attachment #291839 - Flags: superreview?(cbiesinger)
Attachment #291839 - Flags: review?(chpe)
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.
Attached patch New patch (obsolete) — Splinter Review
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)
(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 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-
(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.
(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.
Mike, can you attach an updated patch, please?
Attached patch Final patch (obsolete) — Splinter Review
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)
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+
Attached patch PatchSplinter Review
It should be okay this time
Attachment #299255 - Attachment is obsolete: true
Attachment #299290 - Flags: review?(chpe)
Attachment #299255 - Flags: approval1.9?
Attachment #299290 - Flags: superreview?(cbiesinger)
QA Contact: ian → file-handling
Attachment #299290 - Flags: superreview?(cbiesinger) → superreview+
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 on attachment 299290 [details] [diff] [review]
Patch

a=beltzner
Attachment #299290 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: