233305 - (gnomevfs) Support gnome-vfs's smb: URL scheme [improve gnome integration]

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Darin Fisher]

Support gnome-vfs's smb: URL scheme [improve gnome integration].

This either means creating a new protocol handler for gnome's smb: URL scheme,
or it means that we should extend the unknown protocol handler to try gnome-vfs
in some cases.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

is this meant as a general bug to support all gnome-vfs schemes (that mozilla
can't handle itself), or do you really want to keep it specific to smb?

Comment 2

[Darin Fisher]

sorry, i should have specified that "i am not sure" ... after talking a bit more
with bryner, i think we probably want to make the ucth poke gnome-vfs and return
a real channel (probably a nsInputStreamChannel instance) if it can use
gnome-vfs to load the URL.

my concern is that there might be some protocols that we don't want to execute
in this way.  i don't know enough about gnome-vfs to understand what it would
mean for a web page to redirect to one of those protocols (for example).

Comment 3

[Darin Fisher]

Attachment: v0 patch

This patch works, but still has a lot of issues.  Here are some of the issues:

 - no support for directories (does this matter?)

 - authentication does not use nsIAuthPrompt from nsIChannel (uses
windowwatcher instead)

 - this patch enables support for all protocol schemes supported by GnomeVFS. 
should we whitelist/blacklist instead?	are there protocols we would not want
enabled in this way?

 - no way to determine how much data is available from a GnomeVFSHandle.  that
makes it impossible to implement nsIInputStream::Available.  i just have it
returning PR_UINT32_MAX, and that seems to be ok.

 - using necko's thread pool could be problematic here if a bunch of GnomeVFS
operations clog the thread pool, which could impact the browser UI.

otherwise, this patch works great.  with this patch i was able to drag-n-drop
icons from Nautilus while viewing a directory on a Samba server.  for some
reason, GnomeVFS seems to require authentication even for public shares...
Nautilus has the same problem, so whatever.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

+  authOut->username = g_strdup(NS_LossyConvertUTF16toASCII(user).get());

wouldn't UTF-8 be a better choice?

+  *aProtocolFlags = URI_NORELATIVE | URI_NOAUTH;

noauth? norelative? why?
smb: for example should support relative uris just fine, as does it support
auth, no?

+  const nsPromiseFlatCString &flatSpec = PromiseFlatCString(aSpec);

you seem to use this variable only once, why not put the PromiseFlatCString into
the gnome_vfs_uri_new call?

Comment 5

[Darin Fisher]

(In reply to comment #4)
> +  authOut->username = g_strdup(NS_LossyConvertUTF16toASCII(user).get());
> 
> wouldn't UTF-8 be a better choice?

i don't know.  would it?  do you know what encoding GnomeVFS uses?  i should ask
the gnome-vfs devs.


> +  *aProtocolFlags = URI_NORELATIVE | URI_NOAUTH;
> 
> noauth? norelative? why?
> smb: for example should support relative uris just fine, as does it support
> auth, no?

yeah, this is bogus.  it's leftover from when i was using nsSimpleURI.  i'll fix
that.  thx.


> +  const nsPromiseFlatCString &flatSpec = PromiseFlatCString(aSpec);
> 
> you seem to use this variable only once, why not put the PromiseFlatCString into
> the gnome_vfs_uri_new call?

ditto.  i used to need it in two places.  actually, i meant to pass the
flattened version of the string to necko... since i know that it needs to
flatten the nsACString too ;-)

thanks biesi!

Comment 6

[Roland Mainz]

Darin Fisher wrote:
> (In reply to comment #4)
> > +  authOut->username = g_strdup(NS_LossyConvertUTF16toASCII(user).get());
> > 
> > wouldn't UTF-8 be a better choice?

I guess you want the ConvertUTFBlablaToNativeEncoding() here. Converting to
ASCII is WRONG since it assumes that the OS's current charset is a subset of
ASCII which is NOT guranteed.

Comment 7

[Darin Fisher]

(In reply to comment #6)
> Darin Fisher wrote:
> > (In reply to comment #4)
> > > +  authOut->username = g_strdup(NS_LossyConvertUTF16toASCII(user).get());
> > > 
> > > wouldn't UTF-8 be a better choice?
> 
> I guess you want the ConvertUTFBlablaToNativeEncoding() here. Converting to
> ASCII is WRONG since it assumes that the OS's current charset is a subset of
> ASCII which is NOT guranteed.

how do you know that converting to the "native" encoding (defined as what
PR_Open uses) is correct?  how do you know that every GnomeVFS module expects
that?  maybe GnomeVFS wants UTF-8, or maybe it wants ASCII.  how do you know? 
the HTTP GnomeVFS module from GnomeVFS-2.4.2 expects only ASCII (otherwise it is
violating RFC 2617).

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 141636 [details] [diff] [review]
v0 patch

hm... with your patch, configure fails if I don't have gnome-vfs installed.
wouldn't it be better to just remove gnomevfs from the extensions list?
(you should probably specify an action if not found here:
+	 PKG_CHECK_MODULES(MOZ_GNOMEVFS2, gnome-vfs-2.0 >= $GNOMEVFS2_VERSION)
see "man pkg-config")

you also need to change allmakefiles.sh

nsIOService:
+	   nsCAutoString spec;
+	   spec.Append(scheme);
use .Assign, or even better, put scheme in the constructor, like:
nsCAutoString spec(scheme);
spec.Append(':');

+	   rv = (*result)->NewURI(spec, nsnull, nsnull, getter_AddRefs(uri));
+	   if (NS_SUCCEEDED(rv))
+	       return NS_OK;

should this not call CacheProtocolHandler?

+      // XXX should this be generalized into something that searches a
category?
can you file a bug for that and reference it in this comment? (said category
could also cover the default protocol handler)

the Makefile for the component needs to force a shared library (as discussed on
irc)

+# The Original Code is mozilla.org code.
I like this line to contain a less broad statement :) like "the gnome-vfs
handler" or something

+    nsGnomeVFSInputStream(const nsCString &uriSpec)
why not nsACString?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 141636 [details] [diff] [review]
v0 patch

+  AppendUTF8toUTF16(prepath, key);

this will append a possibly existing username:password@ in the url... you
should strip this out 

+    key.AppendWithConversion(authIn->realm);

realm is ascii? oh well, afaict gnome-vfs ignores intl anyway

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

as for getting the authprompt off the channel,
http://developer.gnome.org/doc/API/gnome-vfs/gnome-vfs-gnome-vfs-module-callback.html#GNOME-VFS-MODULE-CALLBACK-PUSH
may be interesting:
"The temporary handler is set per-thread."

just something to keep in mind, it is no condition for a r+ from me ;)

Comment 11

[Darin Fisher]

(In reply to comment #10)
> as for getting the authprompt off the channel,
>
http://developer.gnome.org/doc/API/gnome-vfs/gnome-vfs-gnome-vfs-module-callback.html#GNOME-VFS-MODULE-CALLBACK-PUSH
> may be interesting:
> "The temporary handler is set per-thread."
> 
> just something to keep in mind, it is no condition for a r+ from me ;)

good suggestion!!  this worked great.  i just put a call to push prior to
calling "open" and followed that with a pop ;-)  very simple... just a little
work to make sure i don't access the nsIChannel on a background thread, but that
was easy enough.  thx biesi!

Comment 12

[Darin Fisher]

(In reply to comment #9)
> (From update of attachment 141636 [details] [diff] [review])
> +  AppendUTF8toUTF16(prepath, key);
> 
> this will append a possibly existing username:password@ in the url... you
> should strip this out 

i've converted the code to build this string: "scheme://hostport", where scheme
is nsIURI.scheme and hostport is nsIURI.hostPort.


> +    key.AppendWithConversion(authIn->realm);
> 
> realm is ascii? oh well, afaict gnome-vfs ignores intl anyway

right.  i don't know what else to assume.  in RFC 2617, the realm is either
7-bit ASCII or that funky mime encoding.  we don't support anything but ASCII
realms in our HTTP code, and it seems like a realm parameter will only be
specified by GnomeVFS if doing a HTTP connection.  that said, i decided to
support the realm string just because i don't know if other GnomeVFS modules
might make use of that API.  it's unclear if i can assume that the realm will be
null in all cases except for HTTP auth.

Comment 13

[Darin Fisher]

(In reply to comment #8)
> (From update of attachment 141636 [details] [diff] [review])
> hm... with your patch, configure fails if I don't have gnome-vfs installed.
> wouldn't it be better to just remove gnomevfs from the extensions list?
> (you should probably specify an action if not found here:
> +	 PKG_CHECK_MODULES(MOZ_GNOMEVFS2, gnome-vfs-2.0 >= $GNOMEVFS2_VERSION)
> see "man pkg-config")

for this reason, i provided a --disable-gnomevfs configure option, but i like
your idea of just automatically disabling it.


> you also need to change allmakefiles.sh

thanks for reminding me!

> nsIOService:
> +	   nsCAutoString spec;
> +	   spec.Append(scheme);
> use .Assign, or even better, put scheme in the constructor, like:
> nsCAutoString spec(scheme);
> spec.Append(':');

i used Append because unlike Assign it doesn't have to call Truncate first, but
yeah... it does look funny.


> +	   rv = (*result)->NewURI(spec, nsnull, nsnull, getter_AddRefs(uri));
> +	   if (NS_SUCCEEDED(rv))
> +	       return NS_OK;
> 
> should this not call CacheProtocolHandler?

no need.  we only cache a small subset of the built-in protocol handlers.


> +      // XXX should this be generalized into something that searches a
> category?
> can you file a bug for that and reference it in this comment? (said category
> could also cover the default protocol handler)

see bug 234714


> the Makefile for the component needs to force a shared library (as discussed on
> irc)

done.  thx


> +# The Original Code is mozilla.org code.
> I like this line to contain a less broad statement :) like "the gnome-vfs
> handler" or something

good idea.  i chose: "the Mozilla gnome-vfs extension"


> +    nsGnomeVFSInputStream(const nsCString &uriSpec)
> why not nsACString?

because it doesn't need to be so generic.

Comment 14

[Brendan Eich [:brendan]]

Someone tell the gnome-vfs doc author that stacks are last-in-first-out.

/be

Comment 15

[Darin Fisher]

(In reply to comment #14)
> Someone tell the gnome-vfs doc author that stacks are last-in-first-out.
> 
> /be

heh.. i know, it's a very lame API.  the assumption is that there is only one
consumer of gnome-vfs per thread or that the consumer will tighly bound calls
that matter.  in which case, support for a stack of callbacks seems pointless
since there will never be more than one on the stack.

Comment 16

[Darin Fisher]

er... you were referring to the fact that he says this:

> The temporary handlers are treated as a first-in first-out stack.

can you tell i'm frustrated with their API?  it's really strange that they don't
give you a way to associate your own context and callback function directly with
the object that is responsible for the callback.

Comment 17

[Darin Fisher]

Attachment: v1 patch

revised patch.

Comment 18

[Mike Shaver (:shaver emeritus)]

WRT auto-disabling: if the user wants gnome-vfs, and says so explicitly with
--enable-gnomevfs, then I think it should be an error to not build it.  Having
extensions auto-selected is a handy and clever, but can also result in a very
frustrating experience for the builder who has to restart their RPM build of the
Suite after they didn't get the extensions they expected to get, due to a
missing gnomevfs-devel, or some such.

Comment 19

[Darin Fisher]

shaver: that's a good point, but i think that sounds like another bug report to
me.  we have lot's of extensions that get auto-disabled based on libraries not
being available.  xmlterm comes to mind, but there are plenty of others :-/

Comment 20

[Christopher Blizzard (:blizzard)]

As opposed to "I built with --enable-gnomevfs and it doesn't work"?  If someone
wants something explicit and it's not possible to build it, it _certainly_
should give you an error.  Every other --enable does this, why should this be
different?

Comment 21

[Darin Fisher]

(In reply to comment #20)
> As opposed to "I built with --enable-gnomevfs and it doesn't work"?  If someone
> wants something explicit and it's not possible to build it, it _certainly_
> should give you an error.  Every other --enable does this, why should this be
> different?

yeah, hmm... i was thinking more in terms of the --enable-extensions.  there are
definitely examples of cases where extensions are auto-disabled even when
explicitly requested by the builder.  that said, this patch does more in that it
adds the --enable/disable-gnomevfs option.  ok, i'll come up with a better
solution.  

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 141656 [details] [diff] [review]
v1 patch

darin is telling me he'll attach a new patch

Comment 23

[Darin Fisher]

Attachment: v1.1 patch

here we go... i meant to post this a few days back, but i got distracted by a
certain string patch ;-)

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 142075 [details] [diff] [review]
v1.1 patch

nsIOService.cpp:
+	   nsCAutoString spec;
+	   spec.Append(scheme);

reminder: please make that nsCAutoString spec(scheme);

+   
bundle->FormatStringFromName(NS_LITERAL_STRING("EnterUserPasswordFor").get(),
+				  strings, 1, getter_Copies(message));
you have to get such a string to prompt for a password?  this sucks :(

+  char *mimeType = gnome_vfs_get_mime_type(spec.get());
ok it turns out that this may very well make a network request. this is bad,
this is on the ui thread...

per irc discussion, best would probably be to get the type off the handle in
Read after the gnome_vfs_open call

Comment 25

[Darin Fisher]

(In reply to comment #24)
> bundle->FormatStringFromName(NS_LITERAL_STRING("EnterUserPasswordFor").get(),
> +				  strings, 1, getter_Copies(message));
> you have to get such a string to prompt for a password?  this sucks :(

yeah, we need a better (simpler) API for auth prompting.


> +  char *mimeType = gnome_vfs_get_mime_type(spec.get());
> ok it turns out that this may very well make a network request. this is bad,
> this is on the ui thread...
> 
> per irc discussion, best would probably be to get the type off the handle in
> Read after the gnome_vfs_open call

it turns out that smb:// doesn't support gnome_vfs_get_file_info_from_handle. 
i'm exploring alternatives.  for the moment, i'm just setting
UNKNOWN_CONTENT_TYPE on the channel, which enables our mime-type detection code.

i'm also toying with supporting directory listings, but i'll probably save that
for another round.

Comment 26

[Darin Fisher]

Attachment: v1.2 patch

This patch cleans up biesi's nits and adds support for directory listings.  (I
changed my mind and decided to go ahead and implement directory listing support
since it affects the way we resolve mime-types.)

Comment 27

[Darin Fisher]

please ignore the garbage "dnl "-prefixed lines in configure.in -- i've removed
those in my local tree.

Comment 28

[timeless]

darin see nsIPromptService2, hopefully it'll be the solution

Comment 29

[Darin Fisher]

Attachment: v1.3 patch

removed the bogus "dnl " lines in configure.in and corrected one mistake in
configure.in -- i made sure to set MOZ_ENABLE_GNOMEVFS=1 when it was previously
equal to "force" in the case where we successfully detect gnome-vfs-2.0 on the
system.

Comment 30

[Brian Ryner (not reading)]

Comment on attachment 142273 [details] [diff] [review]
v1.3 patch

>--- configure.in	21 Feb 2004 20:41:42 -0000	1.1307
>+++ configure.in	25 Feb 2004 23:04:12 -0000
>@@ -3641,21 +3672,21 @@ MOZ_COMPOSER=1
> MOZ_ARG_DISABLE_BOOL(composer,
> [  --disable-composer      Disable building of Composer],
>     MOZ_COMPOSER=,
>     MOZ_COMPOSER=1 )
> AC_SUBST(MOZ_COMPOSER)
> 
> dnl ========================================================
> dnl = Enable compilation of specific extension modules
> dnl ========================================================
> 
>-MOZ_EXTENSIONS_DEFAULT=" cookie wallet content-packs xml-rpc xmlextras help p3p pref transformiix venkman inspector irc universalchardet typeaheadfind webservices spellcheck"
>+MOZ_EXTENSIONS_DEFAULT=" cookie wallet content-packs xml-rpc xmlextras help p3p pref transformiix venkman inspector irc universalchardet typeaheadfind webservices spellcheck gnomevfs"
> 

This will cause lots of platforms to output "Cannot build gnomevfs without
gnome-vfs-2 toolkit." during configure if you don't override the extensions
list.  I'd say don't mess with MOZ_EXTENSIONS_DEFAULT and instead just append
to MOZ_EXTENSIONS if MOZ_ENABLE_GNOMEVFS is set.


>--- netwerk/base/src/nsIOService.cpp	28 Jan 2004 23:45:14 -0000	1.168
>+++ netwerk/base/src/nsIOService.cpp	25 Feb 2004 23:04:19 -0000
>@@ -335,20 +336,44 @@ nsIOService::GetProtocolHandler(const ch
> 
>         rv = CallGetService(contractID.get(), result);
> 
>         // If the pref for this protocol was explicitly set to false,
>         // stop here and do not invoke the default handler.
>         if (NS_FAILED(rv) && listedProtocol)
>             return NS_ERROR_UNKNOWN_PROTOCOL;
>     }
>     
>     if (externalProtocol || NS_FAILED(rv)) {
>+#ifdef XP_UNIX

Maybe use MOZ_X11 instead, to match the check in configure?


>--- extensions/ddd/Makefile.in	1969-12-31 16:00:00.000000000 -0800
>+++ extensions/gnomevfs/Makefile.in	2004-02-23 16:32:51.000000000 -0800
>+MODULE		= nkgnomevfs
>+LIBRARY_NAME	= nkgnomevfs
>+SHORT_LIBNAME	= nkgnomevfs
>+EXPORT_LIBRARY	= 1

Don't set EXPORT_LIBRARY.  If you do, this component will be statically linked
in static builds.

>diff -Nu extensions/ddd/nsGnomeVFSProtocolHandler.cpp extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp
>--- extensions/ddd/nsGnomeVFSProtocolHandler.cpp	1969-12-31 16:00:00.000000000 -0800
>+++ extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp	2004-02-24 16:05:49.000000000 -0800
>+#ifdef DEBUG
>+  {
>+    //
>+    // Make sure authIn->uri is consistent with the channel's URI.
>+    //
>+    // XXX This check is probably not IDN safe, and it might incorrectly
>+    //     fire as a result of escaping differences.  It's unclear what
>+    //     kind of transforms GnomeVFS might have applied to the URI spec
>+    //     that we originally gave to it.  Inspite of the likelihood of

"In spite"

>+  // Construct the single signon key.  Altering the value of this key will
>+  // cause peoples remembered passwords to be forgotten.  Think carefully
>+  // before changing the way this key is constructed.
>+  nsAutoString key, dispHost, realm;
>+  AppendUTF8toUTF16(scheme + NS_LITERAL_CSTRING("://") + hostPort, dispHost);
>+  key = dispHost;
>+  if (authIn->realm)
>+  {
>+    // We assume the realm string is ASCII.  That might be a bogus assumption,
>+    // but we have no idea what encoding GnomeVFS is using, so for now we'll
>+    // limit ourselves to ISO-Latin-1.  XXX What is a better solution?

So, I thought all the GNOME stuff was UTF-8 now.  Is it not?

>+    realm.Append(PRUnichar('\"'));
>+    realm.AppendWithConversion(authIn->realm);
>+    realm.Append(PRUnichar('\"'));
>+    key += NS_LITERAL_STRING(" ") + realm;
>+  }
>+
>+  // Construct the message string...
>+  //
>+  // We use Necko's string bundle here.  This code really should be encapsulated
>+  // behind some Necko API, afterall this code is based closely on the code in

"after all"

Looks good other than that.

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #29)
> i made sure to set MOZ_ENABLE_GNOMEVFS=1 when it was previously
> equal to "force" in the case where we successfully detect gnome-vfs-2.0 on the
> system.

Hm... does that matter?

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 142273 [details] [diff] [review]
v1.3 patch

+MAKEFILES_nkgnomevfs="xpcom/Makefile xpcom/string/Makefile
modules/zlib/Makefile modules/libimg/png/Makefile intl/strres/Makefile
db/mdb/public/Makefile db/mork/src/Makefile db/mork/build/Makefile
rdf/util/Makefile gfx/src/xprintutil/Makefile editor/composer/public/Makefile
expat/Makefile modules/libutil/Makefile intl/uconv/native/Makefile
webshell/public/Makefile js/src/fdlibm/Makefile js/src/Makefile
js/src/xpconnect/Makefile embedding/components/windowwatcher/public/Makefile
ipc/ipcd/Makefile embedding/components/profilesharingsetup/Makefile
profile/dirserviceprovider/Makefile xpfe/components/shistory/Makefile
intl/unicharutil/Makefile embedding/browser/webBrowser/Makefile
embedding/browser/build/Makefile xpcom/obsolete/Makefile
profile/pref-migrator/Makefile profile/Makefile
embedding/components/ui/progressDlg/Makefile netwerk/mime/Makefile
xpfe/components/download-manager/Makefile directory/xpcom/base/Makefile
editor/txmgr/Makefile xpfe/components/directory/Makefile
xpfe/components/find/public/Makefile xpfe/components/find/src/Makefile
xpfe/components/search/public/Makefile xpfe/components/search/src/Makefile
xpfe/components/related/public/Makefile xpfe/components/related/src/Makefile
xpfe/components/intl/Makefile xpfe/components/windowds/Makefile
xpfe/components/autocomplete/public/Makefile
xpfe/components/autocomplete/src/Makefile
xpfe/components/bookmarks/public/Makefile
xpfe/components/bookmarks/src/Makefile
xpfe/components/urlbarhistory/public/Makefile
xpfe/components/urlbarhistory/src/Makefile xpfe/components/build/Makefile
xpfe/browser/public/Makefile xpfe/browser/Makefile xpfe/appshell/Makefile
xpfe/components/xremote/public/Makefile xpfe/components/xremote/Makefile
widget/src/gtksuperwin/Makefile widget/public/Makefile
widget/src/xpwidgets/Makefile widget/src/support/Makefile
widget/src/gtk/Makefile widget/src/xremoteclient/Makefile
uriloader/base/Makefile embedding/components/webbrowserpersist/Makefile
intl/lwbrk/Makefile content/xul/content/Makefile content/xul/templates/Makefile
content/xul/document/Makefile embedding/components/commandhandler/Makefile
embedding/components/jsconsole/public/Makefile embedding/base/Makefile
embedding/components/find/Makefile
embedding/components/windowwatcher/src/Makefile
embedding/components/appstartup/src/Makefile
embedding/components/jsconsole/src/Makefile
embedding/components/printingui/src/unixshared/Makefile
embedding/components/build/Makefile intl/chardet/public/Makefile
intl/chardet/src/Makefile intl/uconv/Makefile htmlparser/Makefile
embedding/components/ui/helperAppDlg/Makefile editor/txtsvc/Makefile
editor/Makefile uriloader/prefetch/Makefile docshell/Makefile
dom/src/jsurl/Makefile sun-java/Makefile modules/libjar/Makefile
js/src/liveconnect/Makefile modules/oji/Makefile dom/Makefile view/Makefile
gfx/src/xlibrgb/Makefile gfx/Makefile intl/locale/Makefile accessible/Makefile
netwerk/protocol/viewsource/Makefile netwerk/protocol/gopher/Makefile
netwerk/build2/Makefile layout/Makefile rdf/chrome/src/Makefile
rdf/chrome/build/Makefile content/Makefile rdf/Makefile
xpfe/components/history/public/Makefile xpfe/components/history/Makefile
uriloader/exthandler/Makefile widget/src/gtkxtbin/Makefile
modules/plugin/Makefile caps/Makefile modules/libpref/Makefile
netwerk/cache/Makefile jpeg/Makefile modules/libpr0n/Makefile netwerk/Makefile
extensions/gnomevfs/Makefile"

hm... what's this used for?

+    // Make GCC happy

given that you commented out a lot of gnomevfs error codes, I think this
comment should be removed.

+    case GNOME_VFS_ERROR_NOT_PERMITTED:
NS_ERROR_FILE_ACCESS_DENIED ?

+    realm.Append(PRUnichar('\"'));
do you need to escape " here?

hm, I'm noticing nsIAuthPrompt is not frozen. it could be changed to not
require strings to be passed in :)
(just a random thought. not condition for r+ :) )

+  gnome_vfs_module_callback_push(GNOME_VFS_MODULE_CALLBACK_AUTHENTICATION,
hm... you actually could get your nsIAuthPrompt here instead of in the auth
callback... then again, that doesn't really make a difference.

+  //	  Unfortunately, the error code GNOME_VFS_ERROR_IS_DIRECTORY is not
+  //	  always returned by gnome_vfs_open when we pass it a URI to a
directory!
please file a bug with gnome-vfs about the issues mentioned in this comment.
ideally, cite the bug number here :)

+      mDirBuf.Append(NS_LITERAL_CSTRING("\n"));
.Append('\n') ?

+      // Write charset (assume UTF-8)
+      // XXX is this correct?
that should be easy to verify... just create a file with a non-ascii name and
check what happens :)

+      *aCountRead = (PRUint32) bytesRead;

you better not load files >4 GB, I guess... I love our freezing of APIs with
such known problems :(

+	   nsCRT::free(escName);
not nsMemory::Free? nsEscape's source code seems to use nsMemory::Alloc

+	 // XXX truncates size from 64-bit to 32-bit
do you want to add such a comment to the other place that does this?

+	 // The "last-modified" field (NSPR promises: PRTime == time_t)

uh. PRTime is a 64 bit value. time_t is generally not. time_t is in seconds,
PRTime64 is in usec. This comment is not correct. See:
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prtime.h#77

did you really get correct last-modified times?

+nsGnomeVFSInputStream::Available(PRUint32 *aResult)
+{
+  // There is no way to implement this using GnomeVFS, but fortunately
+  // that doesn't matter.  We'll just fudge it here...

hm. actually there is. if you use gnome_vfs_get_file_info instead of
get_mime_type, you get the total size of the file if you request it, which is
what you need here, I think.
You just have to substract the amount already read.

this won't work for directories, but oh well.


r+ with the above changes, especially the PRTime thing

Comment 33

[Darin Fisher]

(In reply to comment #31)
> (In reply to comment #29)
> > i made sure to set MOZ_ENABLE_GNOMEVFS=1 when it was previously
> > equal to "force" in the case where we successfully detect gnome-vfs-2.0 on the
> > system.
> 
> Hm... does that matter?

no, it does not really matter.. not with the patch as is.  it does however seem
like a good idea since we might someday inspect MOZ_ENABLE_GNOMEVFS down the
line, and "force" is a little bit odd to have as a value on a boolean.  i'd
rather confine the hack.

Comment 34

[Darin Fisher]

(In reply to comment #32)
> (From update of attachment 142273 [details] [diff] [review])
> +MAKEFILES_nkgnomevfs="xpcom/Makefile xpcom/string/Makefile
> 
> hm... what's this used for?

ask cls... i think it is meant to allow you to build a module and its
dependencies only.  as far as i can tell, it isn't really used by anything normally.


> +    // Make GCC happy
> 
> given that you commented out a lot of gnomevfs error codes, I think this
> comment should be removed.

the comment is there to warn someone against removing the "default:" case since
that will cause GCC to complain about unhandled enum values.  yet, there is a
default case, which is to return NS_ERROR_FAILURE.  as the function is written
now, the final return statement in the function will never be reached.  oh well.


> +    case GNOME_VFS_ERROR_NOT_PERMITTED:
> NS_ERROR_FILE_ACCESS_DENIED ?

sure, that seems reasonable.


> +    realm.Append(PRUnichar('\"'));
> do you need to escape " here?

no, just force of habbit.  i can change it.

 
> hm, I'm noticing nsIAuthPrompt is not frozen. it could be changed to not
> require strings to be passed in :)
> (just a random thought. not condition for r+ :) )

yes, one day.

 
> +  gnome_vfs_module_callback_push(GNOME_VFS_MODULE_CALLBACK_AUTHENTICATION,
> hm... you actually could get your nsIAuthPrompt here instead of in the auth
> callback... then again, that doesn't really make a difference.

no, that would be difficult considering that i cannot access the channel on that
thread.  it's best to wait until the ProxiedAuthCallback because then i'm on the
right thread.


> +  //	  Unfortunately, the error code GNOME_VFS_ERROR_IS_DIRECTORY is not
> +  //	  always returned by gnome_vfs_open when we pass it a URI to a
> directory!
> please file a bug with gnome-vfs about the issues mentioned in this comment.
> ideally, cite the bug number here :)

yes, i will do so once i test with a recent build of gnome-vfs and gnome-vfs-extras.


> +      mDirBuf.Append(NS_LITERAL_CSTRING("\n"));
> .Append('\n') ?

yup.


> +      // Write charset (assume UTF-8)
> +      // XXX is this correct?
> that should be easy to verify... just create a file with a non-ascii name and
> check what happens :)

yeah, well... maybe.  my samba server uses UTF-8 as the filesystem charset. 
perhaps if i had a samba server running on a system with a ISO-8859-1 charset, i
could try to verify that gnome-vfs converts to UTF-8.  because otherwise my test
might be inconclusive.


> +	   nsCRT::free(escName);
> not nsMemory::Free? nsEscape's source code seems to use nsMemory::Alloc

hmm... yeah.  i should have verified that.  i just copied code out of
nsIndexedToHTML.cpp and nsCRT::free is what it uses.  oops!


> +	 // XXX truncates size from 64-bit to 32-bit
> do you want to add such a comment to the other place that does this?

yes, will do.


> +	 // The "last-modified" field (NSPR promises: PRTime == time_t)
> 
> uh. PRTime is a 64 bit value. time_t is generally not. time_t is in seconds,
> PRTime64 is in usec. This comment is not correct. See:
> http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prtime.h#77

from prtime.h:
 "Notes on porting: PRTime corresponds to time_t in ANSI C."

maybe i took that too literally...


> did you really get correct last-modified times?

i probably did not.  i'll verify.


> +nsGnomeVFSInputStream::Available(PRUint32 *aResult)
> +{
> +  // There is no way to implement this using GnomeVFS, but fortunately
> +  // that doesn't matter.  We'll just fudge it here...
> 
> hm. actually there is. if you use gnome_vfs_get_file_info instead of
> get_mime_type, you get the total size of the file if you request it, which is
> what you need here, I think.
> You just have to substract the amount already read.

yeah, i had that thought too... i will try to make that change.


> this won't work for directories, but oh well.

yeah, but it matters more for files since downloads otherwise have no indication
of %-complete.


thanks for the thorough code review!

Comment 35

[Darin Fisher]

fixed-on-trunk

biesi: gnome_vfs_get_file_info did work out well, thanks!

Comment 36

[R.K.Aa.]

I'm building from CVS (clobbered) and getting this:

[dark@localhost bin]$ ./mozilla

libgnomevfs-WARNING **: Unable to create ~/.gnome directory: No such file or
directory
libgnomevfs-WARNING **: Unable to create ~/.gnome directory: No such file or 
directory
./run-mozilla.sh: line 451: 10847 Segmentation fault      "$prog" ${1+"$@"}

[dark@localhost bin]$ cd ~/.gnome
[dark@localhost .gnome]$ rpm -qa |grep gnome-vfs
gnome-vfs2-2.2.5-0.ximian.5.14
gnome-vfs-extras-0.2.0-3.ximian.5.4
gnome-vfs-devel-1.0.5-6.ximian.5.11
gnome-vfs-1.0.5-6.ximian.5.11
gnome-vfs2-devel-2.2.5-0.ximian.5.14
gnome-vfs2-extras-0.99.10-0.ximian.5.5

Comment 37

[Darin Fisher]

(In reply to comment #36)
> I'm building from CVS (clobbered) and getting this:
> 
> [dark@localhost bin]$ ./mozilla
> 
> libgnomevfs-WARNING **: Unable to create ~/.gnome directory: No such file or
> directory
> libgnomevfs-WARNING **: Unable to create ~/.gnome directory: No such file or 
> directory
> ./run-mozilla.sh: line 451: 10847 Segmentation fault      "$prog" ${1+"$@"}

is there any way you can generate a stack trace for this crash?  (moving this
into a new bug would be good too.)  thx!

Comment 38

[R.K.Aa.]

To comment 37:
Sorry - haven't been able to debug Mozilla since Redhat broke glibc:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=110038

Comment 39

[R.K.Aa.]

Sorry for the spam. Downloaded an official trunk build which runs OK, so it must
be a local misconfig of sorts.

Comment 40

[Christopher Blizzard (:blizzard)]

I've seen this crash in regxpcom, which apparently fires up the gnome-vfs stuff.
 If you don't have a home directory the code gets _really pissed off_.  I
actually have a patch in our red hat rpm that disables the gnome-vfs code if you
set a magic env var.