Comment on attachment 378853
Initial extension patch

>+REQUIRES	= xpcom \

This can now be removed (since bug 398573).

>+# to install gnome-vfs2 in order to use the rest of mozilla ;-)

s/gnome-vfs2/gio/

>+ * This code is based on original Mozilla gnome-vfs extension. It implements
>+ * input stream provided by GVFS/GIO.

This should match the template at

http://www.mozilla.org/MPL/MPL-1.1.html#exhibit-a

so I think that means "The Original Code is the Mozilla gnome-vfs extension",
unfortunately.  Although the comment is helpful, it would be better separated
from the license block.

>+     case G_IO_ERROR_FAILED:                     return NS_ERROR_UNEXPECTED;

Looks like G_IO_ERROR_FAILED is a generic failure, so "case
G_IO_ERROR_FAILED:" should probably just fall through to the "default:" to
return NS_ERROR_FAILURE.

>+     case G_IO_ERROR_NOT_MOUNTED:
>+     case G_IO_ERROR_HOST_NOT_FOUND:             return NS_ERROR_UNKNOWN_HOST;

G_IO_ERROR_NOT_MOUNTED is here and in unhandled below.  Consider
NS_ERROR_NOT_CONNECTED here, or just remove.

>+  G_IO_ERROR_INVALID_FILENAME,

NS_ERROR_FILE_INVALID_PATH 

>+  G_IO_ERROR_WOULD_BLOCK,

NS_BASE_STREAM_WOULD_BLOCK

>+  G_IO_ERROR_TIMED_OUT,

Maybe NS_ERROR_NET_TIMEOUT.

>+  if (!gdk_win)
>+  return NULL;
>+
>+  // Get the container
>+  gpointer user_data = NULL;
>+  gdk_window_get_user_data(gdk_win, &user_data);
>+  if (!user_data)
>+  return NULL;

Indent the return statements, please.

>+  // Make sure its really a container
>+  MozContainer *parent_container = (MozContainer*) (user_data);
>+  if (!parent_container)
>+  return NULL;

This is barely pretending to "make sure".
It actually only needs to be a GtkWidget.
Either use GTK_IS_WIDGET or skip the check.

>+    nsCOMPtr<nsIBaseWindow> baseWin(do_QueryInterface(reinterpret_cast<nsISupports*> (window->GetDocShell()) ));

>+        baseWin = do_QueryInterface(reinterpret_cast<nsISupports*> (window->GetDocShell()) );

The reinterpret_casts don't look right.
Does including nsIDocShell should fix the problem?

>+  nsCOMPtr<nsIWindowWatcher> wwatch =
>+      do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv);
>+  //NS_ENSURE_SUCCESS(rv, rv);

Should check for failure here somehow.

>+
>+  nsCOMPtr<nsIDOMWindow> active;
>+  wwatch->GetActiveWindow(getter_AddRefs(active));
>+
>+  GtkWindow *parent_wnd = NULL;
>+  parent_wnd = get_gtk_window_for_nsiwidget(DOMWindowToWidget(active));

This doesn't look thread-safe.

And is there always an active window?

We actually have no idea which window is associated with the stream (AFAIK).
Is the transient parent important?

>+  GMountOperation* mount_op = gtk_mount_operation_new(parent_wnd);

gtk_mount_operation_new needs gtk+-2.14 so there needs to be a configure check
for that version.

Is GTK thread-safe?

>+  if (error) {
>+    g_warning("Error reading directory content: %s", error->message);
>+    nsresult rv = MapGIOResult(error);
>+    g_error_free(error);
>+    return rv;
>+  }
>+  g_object_unref(f_enum);

f_enum needs to be freed even if error.

>+    if (mime_type && strcmp(mime_type, APPLICATION_OCTET_STREAM) != 0) {
>+      SetContentTypeOfChannel(mime_type);
>+      g_free(mime_type);
>+    }

mime_type needs to be freed even if it is APPLICATION_OCTET_STREAM, I assume.

>+  mBytesRemaining = (PRUint64) g_file_info_get_size(info);

The cast doesn't look necessary.

>+      // location is not yet mounted, try to mount
>+      g_warning("Unable to get file info: %s", error->message);

I'd rather not have g_warnings unless something has gone wrong with the code
(or external libraries).  Here, at least the error is handled so this error
seems expected.  If you want the user to see the error, then that may be
complicated, but g_warning is not the way to present the error to the user.

>+ * @return NS_OK when read successfully, NS_BASE_STREAM_CLOSED when enf of file,

"end"

>+    *aCountRead = g_input_stream_read(G_INPUT_STREAM(mStream),
>+                                      aBuf,
>+                                      aCount,
>+                                      NULL,
>+                                      &error);

XPCOM methods usually should not set output parameters when throwing an error.
That protocol is not strictly followed, but returning PRUint32(-1) for
aCountRead could be particularly bad.

>+        if ( g_file_info_get_name(info)[0] == '.'
>+          && (g_file_info_get_name(info)[1] == '\0'
>+          || (g_file_info_get_name(info)[1] == '.'
>+          && g_file_info_get_name(info)[2] == '\0')) )

Can you align to make the grouping clearer, please?
Each line should be just inside its innermost opening parenthesis.
I would store the result of g_file_info_get_name in a variable rather than
calling the function 5 times.

>+nsGIOInputStream::Read(char     *aBuf,
>+                       PRUint32  aCount,
>+                       PRUint32 *aCountRead)
>+{
>+  *aCountRead = 0;
>+
>+  if (mStatus == NS_BASE_STREAM_CLOSED)
>+    return NS_OK;
>+  if (NS_FAILED(mStatus)) {
>+    g_warning("Cannot doOpenFIle: %d", mStatus);
>+    return mStatus;
>+  }

Can this warning be closer to the source of the failure?

>+  if (!mStream && !mDirOpen)
>+    mStatus = DoOpen();
>+  if (mStatus != NS_OK) {
>+    g_warning("Cannot DoOpen file - error code: %x", mStatus);
>+    mChannel->Cancel(mStatus);
>+    return mStatus;
>+  }

Should mChannel only be Canceled on the first error rather than each
time a Read is attempted?

Is it safe to Cancel on this thread?

>+  return mStatus;
>+
>+}

>+  return NS_OK;
>+
>+}

The method blocks can be closed on the line following the return (without the
blank line).

>+    PRInt32 colon_location = strchr(flatSpec.get(), ':') - flatSpec.get();
>+    if (colon_location <= 0)

The conversion to PRInt32 isn't necessarily going to be -ve if strchr returns
NULL (and even the ptrdiff_t won't necessarily be -ve, because NULL is not in
the same array as flatSpec).  flatSpec.FindChar(':') looks like what you want.

http://hg.mozilla.org/mozilla-central/annotate/0e157c793c5c/xpcom/glue/nsStringAPI.h#l676

>+      if (strncmp( *uri_schemes, flatSpec.get(), colon_location - 1 ) == 0) {
>+        uri_scheme_supported = PR_TRUE;
>+        break;
>+      }

If *uri_schemes is, for example, "scheme-one", then uri_scheme_supported will
be true even when flatSpec is "sx:something", for example.  I think the
following should work better:

StringHead(flatspec, colon_location).Equals(*uri_schemes)

>+    if (uri_scheme_supported == PR_FALSE) {

Just use PRBool as a boolean type:

  if (!uri_scheme_supported)

>+  // XXX Can we really assume that all gnome-vfs URIs can be parsed using
>+  //     nsStandardURL?  We probably really need to implement nsIURI/nsIURL
>+  //     in terms of the gnome_vfs_uri_XXX methods, but at least this works
>+  //     correctly for smb:// URLs ;-)
>+  //
>+  // FIXME    Also, it might not be possible to fully implement nsIURI/nsIURL in
>+  //     terms of GnomeVFSURI since some Necko methods have no GnomeVFS
>+  //     equivalent.

Does this all apply to GIO also?