Launchpad has imported 38 comments from the remote bug at https://bugzilla.mozilla.org/show_bug.cgi?id=494163.
If you reply to an imported comment from within Launchpad, your comment will be sent to the remote bug automatically. Read more about Launchpad's inter-bugtracker facilities at https://help.launchpad.net/InterBugTracking. ------------------------------------------------------------------------ On 2009-05-21T13:24:57+00:00 Jhorak wrote: User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042708 Fedora/3.0.10-1.fc10 Firefox/3.0.10 Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042708 Fedora/3.0.10-1.fc10 Firefox/3.0.10 As gnomeVFS is deprecated we should port gnomevfs extension to GIO/GVFS. Gnomevfs extension allows opening sftp:// or smb:// URI directly in Firefox. Reproducible: Always Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/6 ------------------------------------------------------------------------ On 2009-05-21T13:28:58+00:00 Jhorak wrote: Created attachment 378853 Initial extension patch Use "--enable-extensions=gio" to compile with gio module. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/7 ------------------------------------------------------------------------ On 2009-08-04T21:25:57+00:00 Caillon wrote: closing in favor of the other bug *** This bug has been marked as a duplicate of bug 402892 *** Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/9 ------------------------------------------------------------------------ On 2009-08-10T13:25:23+00:00 Jhorak wrote: Actually this bug/patch is about GnomeVFS extension which allows to open URI like sftp:// smb://. Bug #402892 is concerned about FF integration into Gnome (file association, MIME types, default browser settings, etc.). Therefore the review request is still valid. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/10 ------------------------------------------------------------------------ On 2009-09-06T21:41:42+00:00 Roc-ocallahan wrote: Comment on attachment 378853 Initial extension patch I only took a cursory look at the patch --- i.e. this is really an sr, and really not an r :-) Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/11 ------------------------------------------------------------------------ On 2009-09-12T03:24:11+00:00 Karlt wrote: 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? Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/12 ------------------------------------------------------------------------ On 2009-09-18T13:36:18+00:00 Jhorak wrote: Created attachment 401429 Patch 0.2, fixed according to review comments Thanks a lot for your comments. I've tried to fix problematic issues. I've also dropped from searching for root window (to be able to create modal dialogue). In fact while invoking command 'firefox sftp://etc' failed to get root window anyway. The problem is that if you close the Firefox while the login window is still showed the Firefox keeps naturally running. Right now I'm not sure how to solve this issue. Could you look at this patch once again, please? Also please look at GTK 2.14 checking in configure.in, I'm not very sure if this is the right way to do the version checking. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/15 ------------------------------------------------------------------------ On 2009-10-02T05:49:52+00:00 Karlt wrote: (In reply to comment #6) > The problem is that if you close the Firefox while the login window is > still showed the Firefox keeps naturally running. Right now I'm not sure how > to > solve this issue. What did the gnomevfs extension do here? nsAppStartup counts XUL windows and tries to quit when the last is closed; that may cause some problems here. Would it make sense to implement a GMountOperation (to replace the GtkMountOperation) using nsIAuthPrompt (like the gnomevfs extension used)? One thing we need to tell the user when asking for a password is the host to which they are connecting. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/16 ------------------------------------------------------------------------ On 2010-01-05T19:59:53+00:00 Reed Loden wrote: Jan, any updates on this? Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/19 ------------------------------------------------------------------------ On 2010-01-12T13:25:43+00:00 Jhorak wrote: Created attachment 421233 Patch 0.3, switched from gtk_mount_operation back to nsIAuthPrompt Sorry for long delay. I've removed gtk_mount_operation and put nsIAuthPrompt back. I also switched to using observers for getting dialogue result. Any ideas for?: while (!mMountFinished) { PR_Sleep(1000); // XXX waiting loop } Please could you check it? Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/20 ------------------------------------------------------------------------ On 2010-01-12T14:52:45+00:00 Jhorak wrote: Created attachment 421249 Patch 0.3b, switched from gtk_mount_operation back to nsIAuthPrompt Sorry, bogus diff parts. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/21 ------------------------------------------------------------------------ On 2010-02-10T18:25:16+00:00 Sgautherie-bz wrote: Comment on attachment 421249 Patch 0.3b, switched from gtk_mount_operation back to nsIAuthPrompt >diff -r 1a3d81ba980a config/autoconf.mk.in >@@ -261,16 +261,19 @@ MOZ_GCONF_CFLAGS = @MOZ_GCONF_CFLAGS@ > MOZ_GIO_CFLAGS = @MOZ_GIO_CFLAGS@ > MOZ_GIO_LIBS = @MOZ_GIO_LIBS@ > >+MOZ_GIO_CFLAGS = @MOZ_GIO_CFLAGS@ >+MOZ_GIO_LIBS = @MOZ_GIO_LIBS@ >+ Bad merge? Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/22 ------------------------------------------------------------------------ On 2010-02-11T13:04:02+00:00 Jhorak wrote: Created attachment 426480 Patch 0.4, removed configure.in and autoconf.mk.in diffs. Sorry, you're right. Removed diffs of configure.in and autoconf.mk.in as they are obsolete now. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/23 ------------------------------------------------------------------------ On 2010-05-12T04:07:40+00:00 Karlt wrote: Comment on attachment 426480 Patch 0.4, removed configure.in and autoconf.mk.in diffs. (In reply to comment #9) > I also switched to using observers for getting dialogue result. These observers are retrieving the result of g_file_mount_enclosing_volume (rather than the password dialog). nsIObserverService is really for global notifications. Here IIUC there could be multiple concurrent nsGIOInputStreams and thus multiple GMountOperations. It seems that mount_enclosing_volume_finished needs a reference to the nsGIOInputStream. > Any ideas for?: > while (!mMountFinished) { > PR_Sleep(1000); // XXX waiting loop > } > Please could you check it? mozilla::Monitor should suit this case: http://hg.mozilla.org/mozilla-central/annotate/f7a9b2f21b09/xpcom/glue/Monitor.h#l59 https://developer.mozilla.org/en/NSPR_API_Reference/Introduction_to_NSPR#NSPR_Thread_Synchronization It would be helpful to have some comments explaining on which thread the callbacks from g_file_mount_enclosing_volume will be invoked. (It's a little counter-intuitive that its neither the thread that called g_file_mount_enclosing_volume nor the thread that created the GMountOperation. I don't know that we can be sure that nsGIOInputStream::Read is always called off the main thread. If it gets called on the main thread, MountVolume will lock. I think the right thing to do if G_IO_ERROR_NOT_MOUNTED is received when called on the main thread is just skip the MountVolume and return an error. It looks like "bool NS_IsMainThread()" is the function to use to check this. Can you check the list of headers included please? There have been some modifications so I think some are longer be needed. configure.in should probably have a check that gio is available, similar to the `echo "$MOZ_EXTENSIONS" | grep -c gnomevfs` test. >+ * The Original Code is IBM Corporation. "* The Original Code is the Mozilla gnome-vfs extension. * * The Initial Developer of the Original Code is IBM Corporation." >+ case G_IO_ERROR_NOT_MOUNTED: return NS_ERROR_NOT_CONNECTED >+/* unhandled: >+ G_IO_ERROR_NOT_REGULAR_FILE, >+ G_IO_ERROR_NOT_SYMBOLIC_LINK, >+ G_IO_ERROR_NOT_MOUNTABLE_FILE, >+ G_IO_ERROR_TOO_MANY_LINKS, >+ G_IO_ERROR_NOT_MOUNTED, G_IO_ERROR_NOT_MOUNTED is handled above so remove it from this list. >+ // 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. In spite of the likelihood of >+ // false hits, this check is probably still valuable. >+ // >+ nsCAutoString spec; >+ uri->GetSpec(spec); >+ int uriLen = strlen(default_domain); >+ if (!StringHead(spec, uriLen).Equals(nsDependentCString(default_domain, >uriLen))) >+ { >+ LOG(("gnomevfs: [spec=%s authIn->uri=%s]\n", spec.get(), >default_domain)); >+ NS_ERROR("URI mismatch"); >+ } I wouldn't expect default_domain to be the same as the spec. It seems the only reason this passes (when testing with sftp:) is that default_domain is empty. Might as well just remove the check I guess. It was only run in debug nsGnomeVFSProtocolHandler builds anyway. If G_ASK_PASSWORD_NEED_DOMAIN is requested, should the reply be G_MOUNT_OPERATION_UNHANDLED? >+ const PRUnichar *strings[] = { realm.get(), dispHost.get() }; >+ >bundle->FormatStringFromName(NS_LITERAL_STRING("EnterUserPasswordForRealm").get(), "EnterUserPasswordForRealm" has been changed to "EnterLoginForRealm" Apparently nsGnomeVFSProtocolHandler didn't get updated. http://hg.mozilla.org/mozilla-central/annotate/5b027af3af29/dom/locales/en-US/chrome/prompts.properties http://hg.mozilla.org/mozilla-central/rev/5949243ac286 ++ GMountOperation* mount_op = g_mount_operation_new(); >+ g_signal_connect (mount_op, "ask_password", >+ G_CALLBACK (mount_operation_ask_password), mChannel); Make this "ask-password" for consistency with the documentation, and touch up indentation. It looks like the channel could be destroyed before mount_operation_ask_password is called. Disconnecting the callback from the GMountOperation when the channel is released may be possible somehow. Or perhaps its easier to use the nsGIOInputStream for the user data and to get the channel from that, but then the nsGIOInputStream::mChannel would still need to be NULLed at the right time. Or perhaps it is possible to synchronously cancel g_file_mount_enclosing_volume. I haven't really worked out the best way to handle this. >+ if (error) { >+ g_warning("Error reading directory content: %s", error->message); >+ nsresult rv = MapGIOResult(error); >+ g_error_free(error); >+ g_object_unref(f_enum); >+ return rv; >+ } >+ g_object_unref(f_enum); Instead of having two g_object_unref calls, unref f_enum in one call before the "if (error)" check. >+ //if (mBytesRemaining != PR_UINT64_MAX) I guess there's no need for this with g_file_info_get_size? >+ //AuthCallback((gconstpointer)&mSpec, 0, NULL, 0, mChannel); Remove this. >+ GError *error = NULL; >+ nsresult rv = NS_OK; >+ PRUint32 bytes_read = 0; >+ if (mStream) { >+ // file read >+ bytes_read = g_input_stream_read(G_INPUT_STREAM(mStream), >+ aBuf, >+ aCount, >+ NULL, >+ &error); If bytes_read is only going to be used in the "if (mStream)" block then instead of initializing bytes_read to 0, declare and initialize with the g_input_stream_read call. Similarly, declare |error| inside the block. I suspect NS_OK is not the right thing to return if this function does nothing. >+ const char * fname = g_file_info_get_name(info); >+ if ( fname[0] == '.' >+ && (fname[1] == '\0' || (fname[1] == '.' && fname[2] == '\0')) ) Can we can be sure that fname is non-NULL? Can you touch up the indentation, please? >+ nu->EscapeString(nsDependentCString(g_file_info_get_name(info)), Use fname (if non-NULL). >+ * @param aCount length if aBuf "length of aBuf" >+ // Check if file is already opened, otherwise open it >+ if (!mStream && !mDirOpen) { >+ mStatus = DoOpen(); Should mStatus be checked for previous failure before trying the open again? >+ g_warning("Unable to open location - error code: %x", mStatus); >+ mChannel->Cancel(NS_ERROR_NOT_CONNECTED); >+ return 0; Probably no need for this warning as DoOpen already provides failure warnings. nsIChannel.idl says "This interface must be used only from the XPCOM main thread", which makes me concerned about this. nsGnomeVFSProtocolHandler didn't bother to Cancel the channel. Is this necessary? The return value should be a (failure) nsresult. >+ g_warning("Unable to read from location - error code: %x", mStatus); >+ mChannel->Cancel(mStatus); Similar issues here. >+ uri_scheme_supported = PR_TRUE; >+ //break; It would make sense to keep this break. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/25 ------------------------------------------------------------------------ On 2010-05-12T21:20:50+00:00 Karlt wrote: (In reply to comment #13) > It looks like the channel could be destroyed before > mount_operation_ask_password is called. Sorry, I think I'm wrong here and I expect what you have is fine. I don't completely grasp the relationship between the nsGIOInputStream and the nsIChannel, but I assume that, if a thread is Read()ing the nsGIOInputStream, something must be holding a reference to the nsIChannel. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/26 ------------------------------------------------------------------------ On 2010-05-17T09:27:13+00:00 Jhorak wrote: Created attachment 445660 Patch 0.5, Monitor added, observers removed Removed observers replacing them with user pointer in mount_enclosing_volume_finished. Added comments about threads, Waiting loop replaced by monitor. Usage of Cancel method for channels shows nice error messages to user which depends on gio failure type. Please have a look. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/27 ------------------------------------------------------------------------ On 2010-05-19T04:27:45+00:00 Karlt wrote: Comment on attachment 445660 Patch 0.5, Monitor added, observers removed >+ else >+ if test `echo "$MOZ_EXTENSIONS" | grep -c gio` -ne 0; then >+ PKG_CHECK_MODULES(MOZ_GNOMEVFS, gio-2.0 >= $GIO_VERSION [ >+ MOZ_GIO_LIBS=`echo $MOZ_GIO_LIBS | sed 's/-llinc\>//'` >+ ]) >+ fi This would need a comma after GIO_VERSION, I assume, and the first argument would need to be MOZ_GIO, but this doesn't work anyway because MOZ_EXTENSIONS is not yet set. That means that the same test for MOZ_GNOMEVFS doesn't work either and so I guess no one has used that. Therefore feel free to just leave configure.in as is and --enable-gio will be needed to build the gio extension. >+LOCAL_INCLUDES = $(MOZ_GTK2_CFLAGS) \ >+ $(MOZ_GIO_CFLAGS) >+ >+EXTRA_DSO_LDOPTS = \ >+ $(XPCOM_GLUE_LDOPTS) \ >+ $(NSPR_LIBS) \ >+ $(MOZ_GTK2_LIBS) \ >+ $(MOZ_GIO_LIBS) \ >+ $(NULL) >+#include <gtk/gtk.h> GTK is no longer used. I expect gio/gio.h would be sufficient. >+ mozilla::MonitorAutoEnter mon(mMonitorMountInProgress); >+ /* Waiting for finish of mount operation thread */ >+ mon.Wait(); >+ g_warning("monitor done"); + g_object_unref(mount_op); The state of the mount operation should be checked before waiting in case the operation already Notify/signals before this wait starts. Also pthread_cond_wait() can wakeup spuriously, so the state should be checked on return from Wait(). If G_MOUNT_OPERATION_IN_PROGRESS were a GMountOperationResult this would normally look something like while (mMountRes == G_MOUNT_OPERATION_IN_PROGRESS) mon.Wait(); >+ * nsGIOInputStream. This function is called in main thread as async request >+ * from dbus. How about "main thread as an async request typically from dbus"? IIUC, while gvfs is implemented using dbus, GIO need not necessarily use dbus, for other modules for example. If G_ASK_PASSWORD_NEED_DOMAIN is requested, should the reply be G_MOUNT_OPERATION_UNHANDLED? Please remove the debugging g_warnings. Ideally g_warnings would only be for cases where the code is misused (and normally we use NS_ASSERTION for that). Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/28 ------------------------------------------------------------------------ On 2010-05-19T04:31:12+00:00 Karlt wrote: (In reply to comment #15) > Usage of Cancel method for channels shows nice error messages to user which > depends on gio failure type. Please have a look. How do you get these error messages and where do you see them? I tried sftp://notauser@localhost/etc, cancelling and entering the wrong password 3 times. And how do you know that it is safe to call nsIInputStreamChannel::Cancel() from the Read() thread? Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/29 ------------------------------------------------------------------------ On 2010-05-19T04:51:56+00:00 Bzbarsky wrote: > And how do you know that it is safe to call nsIInputStreamChannel::Cancel() > from the Read() thread? It's not, unless the Read() thread is the main thread. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/30 ------------------------------------------------------------------------ On 2010-05-19T04:57:49+00:00 Bzbarsky wrote: I'd think an error rv from Read() would propagate into the channel status. Does it not? Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/31 ------------------------------------------------------------------------ On 2010-05-20T11:54:54+00:00 Jhorak wrote: Created attachment 446488 Patch 0.6, warnings removed, Cancel removed Sorry for confusion with Cancel. Error reporting works without calling it. Sorry also for g_warning leftovers (they was just for my debug runs). I removed Cancel call, useless g_warnings and configure.in changes. There is still no feedback when user aborts password dialog or fill wrong login/pass. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/32 ------------------------------------------------------------------------ On 2010-05-24T03:24:09+00:00 Karlt wrote: Comment on attachment 446488 Patch 0.6, warnings removed, Cancel removed Thank you, Jan. This is looking good. (In reply to comment #20) > There is still no feedback when user aborts password dialog or fill wrong > login/pass. I don't think we need to sort that out in order to land this. The gnomevfs extension seems to leave no feedback on failure often enough. >+ /* Calling g_file_mount_enclosing_volume creates a new thread */ Is this actually true? I thought typically it would start a new process and send a dbus request. But really it could do anything. The key is that it is asynchronous and the reply arrives on the main thread. >+ /* Waiting for finish of mount operation thread */ >+ while (mMountRes == G_MOUNT_OPERATION_UNHANDLED) I was initially worried that this might sometimes get set to G_MOUNT_OPERATION_UNHANDLED as a GIO result of g_file_mount_enclosing_volume(). But then I remembered that mMountRes is only set to internal values based on the error from g_file_mount_enclosing_volume_finish. I think it would be clearer to use an internal enum type for mMountRes with 3 values for the in-progress/failure/success states. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/33 ------------------------------------------------------------------------ On 2011-01-05T13:49:31+00:00 Jhorak wrote: Created attachment 501303 Patch 0.7, sync to mozilla-central Please have a look at version synced with latest mozilla-central. When review gets positive I'll try to set need-checkin. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/40 ------------------------------------------------------------------------ On 2011-01-05T22:03:38+00:00 Karlt wrote: The merge with m-c looks good, thanks, but, before checkin, can you correct the "Calling g_file_mount_enclosing_volume creates a new thread" comment and use a new distinct enum type for mMountRes, in line with comment 21, please? We'll also need to get approval2.0, but as this is not part of the default build, I don't expect that to be a problem. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/41 ------------------------------------------------------------------------ On 2011-01-05T22:08:21+00:00 Karlt wrote: Comment on attachment 501303 Patch 0.7, sync to mozilla-central >+#include "nsIStandardURL.h" >+#include "nsMimeTypes.h" >+#include "nsNetUtil.h" >+#include "mozilla/Monitor.h" >+#include "nsIStandardURL.h" One of the nsIStandardURL.h includes can be removed too :) Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/42 ------------------------------------------------------------------------ On 2011-01-06T12:24:16+00:00 Jhorak wrote: Created attachment 501632 Patch 0.8, fixed comments and added mount operation enum Here it comes... Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/43 ------------------------------------------------------------------------ On 2011-01-09T23:33:50+00:00 Karlt wrote: Comment on attachment 501632 Patch 0.8, fixed comments and added mount operation enum >+nsGIOInputStream::SetMountResult(GMountOperationResult result, gint >error_code) >+{ >+ mozilla::MonitorAutoEnter mon(mMonitorMountInProgress); >+ mMountRes = (result == G_MOUNT_OPERATION_HANDLED) >+ ? MOUNT_OPERATION_SUCCESS : MOUNT_OPERATION_FAILED; SetMountResult is only used in this file and the parameter is not really a GMountOperationResult. It should be the internal enum "MountOperationResult". >+ , mMountRes(MOUNT_OPERATION_IN_PROGRESS) mMountRes is only used in nsGIOInputStream::MountVolume() so doesn't need to be initialized here. >+ /* A g_file_mount_enclosing_volume is using dbus request to mount volume. "/* g_file_mount_enclosing_volume uses a dbus request to mount the volume." >+ Callback mount_enclosing_volume_finished is called in main thread >+ (not this thread from witch it this method called). */ "(not this thread on which this method is called)." Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/44 ------------------------------------------------------------------------ On 2011-01-10T11:38:57+00:00 Jhorak wrote: Created attachment 502458 Patch 0.9, fixed comments and mount operation enum Ah, sorry about it, I was too hasty with enums. Thanks for fixing comments too. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/45 ------------------------------------------------------------------------ On 2011-01-11T06:56:55+00:00 Karlt wrote: Comment on attachment 502458 Patch 0.9, fixed comments and mount operation enum >+++ b/extensions/gio/Makefile.in Mon Jan 10 12:32:39 2011 +0100 >+# This code is based on original Mozilla gnome-vfs extension. It implements >+# input stream provided by GVFS/GIO. This license header needs to be made consistent with the original gnomevfs Makefile.in, in the same way as you fixed up for nsGIOProtocolHandler.cpp. I think we can request approval2.0 when that's done. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/46 ------------------------------------------------------------------------ On 2011-01-11T10:19:31+00:00 Jhorak wrote: Created attachment 502759 Patch 0.1, fixed license Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/47 ------------------------------------------------------------------------ On 2011-01-11T19:33:33+00:00 Karlt wrote: (In reply to comment #23) > ... this is not part of the default build ... To be more precise: This extension is not enabled by default. The only change to the default build here is a lookup of the service NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX"moz-gio" (which won't normally be present). This is similar to what is done for the gnomevfs extension. The benefit of the patch is that distributions no longer shipping gnomevfs can enable the gio extension for similar behaviour using modern system libraries. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/48 ------------------------------------------------------------------------ On 2011-01-28T14:29:49+00:00 Jhorak wrote: Setting checkin-needed, is it possible? Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/49 ------------------------------------------------------------------------ On 2011-01-28T14:37:48+00:00 Mounir wrote: (In reply to comment #31) > Setting checkin-needed, is it possible? Unfortunately not. This patch need to be approved before landing on mozilla-central because we are too close to the release. If it's not approved, it will be able to be pushed after the release. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/50 ------------------------------------------------------------------------ On 2011-01-28T20:45:18+00:00 Roc-ocallahan wrote: I really think we should take this post-FF4. Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/51 ------------------------------------------------------------------------ On 2011-02-02T22:17:46+00:00 Jst wrote: Comment on attachment 502759 Patch 0.1, fixed license Agreed, this is too big of a change for Firefox 4 at this point. We'll get this in after branching! Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/52 ------------------------------------------------------------------------ On 2011-03-24T03:46:35+00:00 Ehsan-mozilla wrote: http://hg.mozilla.org/projects/cedar/rev/7c42f37e0284 Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/53 ------------------------------------------------------------------------ On 2011-03-24T19:42:40+00:00 Ehsan-mozilla wrote: http://hg.mozilla.org/mozilla-central/rev/7c42f37e0284 Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/54 ------------------------------------------------------------------------ On 2011-03-24T23:42:59+00:00 Chpe wrote: The patch as checked in still is full of debugging g_warnings()s; I think they should be removed, as the reviewer in comment 5 and comment 16 also said. MapGIOResult(gint) converts GIOErrorEnum codes to nsresult, but MapGIOResult(GError*) hands it any GError's ->code without first checking that ->domain == G_IO_ERROR (there's no guarantee all those gio functions only return G_IO_ERROR errors). Reply at: https://bugs.launchpad.net/thunderbird/+bug/105088/comments/55 ** Changed in: thunderbird Status: Unknown => Fix Released ** Changed in: thunderbird Importance: Unknown => Wishlist -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/105088 Title: Thunderbird does not honor gnome-vfs paths To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/105088/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
