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.
>+ // 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,
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.
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.
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.
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:
>+ // 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.
(From update of attachment 378853)
>+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: HOST_NOT_ FOUND: return NS_ERROR_ UNKNOWN_ HOST;
>+ case G_IO_ERROR_
G_IO_ERROR_ NOT_MOUNTED is here and in unhandled below. Consider NOT_CONNECTED here, or just remove.
NS_ERROR_
>+ 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) get_user_ data(gdk_ win, &user_data);
>+ return NULL;
>+
>+ // Get the container
>+ gpointer user_data = NULL;
>+ gdk_window_
>+ 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_QueryInterfa ce(reinterpret_ cast<nsISupport s*> (window- >GetDocShell( )) ));
>+ baseWin = do_QueryInterfa ce(reinterpret_ cast<nsISupport s*> (window- >GetDocShell( )) );
The reinterpret_casts don't look right.
Does including nsIDocShell should fix the problem?
>+ nsCOMPtr< nsIWindowWatche r> wwatch = NS_WINDOWWATCHE R_CONTRACTID, &rv); SUCCESS( rv, rv);
>+ do_GetService(
>+ //NS_ENSURE_
Should check for failure here somehow.
>+ nsIDOMWindow> active; >GetActiveWindo w(getter_ AddRefs( active) ); window_ for_nsiwidget( DOMWindowToWidg et(active) );
>+ nsCOMPtr<
>+ wwatch-
>+
>+ GtkWindow *parent_wnd = NULL;
>+ parent_wnd = get_gtk_
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) { error); free(error) ; unref(f_ enum);
>+ g_warning("Error reading directory content: %s", error->message);
>+ nsresult rv = MapGIOResult(
>+ g_error_
>+ return rv;
>+ }
>+ g_object_
f_enum needs to be freed even if error.
>+ if (mime_type && strcmp(mime_type, APPLICATION_ OCTET_STREAM) != 0) { fChannel( mime_type) ;
>+ SetContentTypeO
>+ 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] == '.' info_get_ name(info) [1] == '\0' info_get_ name(info) [1] == '.' info_get_ name(info) [2] == '\0')) )
>+ && (g_file_
>+ || (g_file_
>+ && g_file_
Can you align to make the grouping clearer, please? info_get_ name in a variable rather than
Each line should be just inside its innermost opening parenthesis.
I would store the result of g_file_
calling the function 5 times.
>+nsGIOInputStr eam::Read( char *aBuf, STREAM_ CLOSED) mStatus) ) {
>+ PRUint32 aCount,
>+ PRUint32 *aCountRead)
>+{
>+ *aCountRead = 0;
>+
>+ if (mStatus == NS_BASE_
>+ return NS_OK;
>+ if (NS_FAILED(
>+ g_warning("Cannot doOpenFIle: %d", mStatus);
>+ return mStatus;
>+ }
Can this warning be closer to the source of the failure?
>+ if (!mStream && !mDirOpen) >Cancel( mStatus) ;
>+ mStatus = DoOpen();
>+ if (mStatus != NS_OK) {
>+ g_warning("Cannot DoOpen file - error code: %x", mStatus);
>+ mChannel-
>+ 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 FindChar( ':') looks like what you want.
NULL (and even the ptrdiff_t won't necessarily be -ve, because NULL is not in
the same array as flatSpec). flatSpec.
http:// hg.mozilla. org/mozilla- central/ annotate/ 0e157c793c5c/ xpcom/glue/ nsStringAPI. h#l676
>+ if (strncmp( *uri_schemes, flatSpec.get(), colon_location - 1 ) == 0) { supported = PR_TRUE;
>+ uri_scheme_
>+ 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?