GNOME Bugzilla – Bug 144025
Close new tab if the document couldn't be displayed inline
Last modified: 2007-02-20 18:25:46 UTC
Often I click on links as "open in new tab" without realising that it's a pdf file. The result is, there will be a new tab opened, but since the pdf file needs a helper application, the new tab remains blank. I should have just clicked the link instead. Wouldn't it be convenient if that new tab was automatically closed once it was discovered that the document needed a helper app. Another method could be to not open the new tab until this check has been done, but if it's a slow link to load this could be seen as an error by the user.
Mass reassigning of Epiphany bugs to epiphany-maint@b.g.o
Created attachment 51859 [details] [review] epiphany-1.8.0-new-tab.patch Proposed patch. Adds an enum EPHY_EMBED_DOCUMENT_EXTERNAL to EphyEmbedDocumentType and reports that as ge_document_type in GContentHandler::Show.
Thanks for the patch! It's an interesting idea. I was hoping there was some way to get notified from gecko when the embed's document changes, but in absence of that, this approach looks okay. Unfortunately, the patch doesn't work quite right for the case that the new window was opened from JS. In this case, the EPHY_NEW_TAB_OPEN_PAGE flags isn't set (see ephy-shell.c:ephy_shell_new_window_cb). Then there's the problem that when the signal emission from ContentHandler causes the window to be closed, it needs to set mContext to nsnull or otherwise we'll crash later on. And the new_tab_got_type_cb handler needs to be disconnected, or else the tab/window will also close when you later on (i.e. not as first thing in that tab) load a page which goes to the external handler (I don't think we want to close the tab there). That could get tricky... is the document-type signal with EXTERNAL the first emission of that signal on the new tab?
> Unfortunately, the patch doesn't work quite right for the case that the new > window was opened from JS. In this case, the EPHY_NEW_TAB_OPEN_PAGE flags > isn't set (see ephy-shell.c:ephy_shell_new_window_cb). OK. I think in this case we want to add the signal handler from ephy_shell_new_window_cb() (not ephy_shell_new_tab_full()) as that way it is clearly bound to the code path from a javascript window launch. (In the user-initiated case, the signal handler needs to be set from inside ephy_shell_new_tab_full() because it has to be set before ephy_embed_load_url() is called to load the new url into the new tab) > Then there's the problem that when the signal emission from ContentHandler > causes the window to be closed, it needs to set mContext to nsnull or > otherwise we'll crash later on. Not sure I understand the problem here. I'm not too hot on C++/XPCOM but I'd have guessed the nsCOMPtr magic would keep mContext alive until the ContextHandler is destroyed - ? > And the new_tab_got_type_cb handler needs to be disconnected, or else the > tab/window will also close when you later on (i.e. not as first thing in that > tab) load a page which goes to the external handler (I don't think we want to > close the tab there). That could get tricky... is the document-type signal > with EXTERNAL the first emission of that signal on the new tab? Oops. How'd I miss that? (Middle click to open a new tab, browse for a while, download a file, tab disappears - ouch!) Yes, the handler needs to be disconnected. The way it should work is that when a new tab is opened with EPHY_NEW_TAB_OPEN_PAGE or from JS the handler is connected; we close the tab if the very first document-type is EXTERNAL; later document-type signals are irrelevant so the handler should disconnect itself immediately it is invoked.
Created attachment 51901 [details] [review] epiphany-1.8.0-new-tab.patch * Also connect new_tab_got_type_cb() in ephy_shell_new_window_cb() * Disconnect new_tab_got_type_cb() immediately it is invoked (using g_signal_handlers_disconnect_by_func() to avoid having to store the handler id)
>Not sure I understand the problem here. I'm not too hot on C++/XPCOM but I'd >have guessed the nsCOMPtr magic would keep mContext alive until the >ContextHandler is destroyed - ? That's correct. However the problem is that we use that interface to get the GtkWindow on which to parent the dialogue on. So: signal emission closes window -> window pointer gotten (EphyUtils::FindGtkParent) from the mContext is not a toplevel window -> gtk_dialog_run complains (warnings on console). Another thing that's necessary is to make EphyTab and EphyWindow handle the new document type EXTERNAL (for the case that this wasn't the initial load, so the tab wasn't closed). I think it can treat that type just like OTHER.
Ed, are you still working on this?
Ed, ping?
Created attachment 58568 [details] [review] 144025.bugzilla.gnome.org-close-tab-for-external.patch OK. This patch is slightly changed. First difference: we can no longer rely on EPHY_EMBED_DOCUMENT_EXTERNAL being the first ge_document_type signal emission; EPHY_EMBED_DOCUMENT_HTML happens twice before (I think this is to do with the in-browser error pages, but not sure). However, we can rely on ge_content_change being sent on initial load in embeds with content. So the tactic is to wait for ge_document_type = EPHY_EMBED_DOCUMENT_EXTERNAL, but only until ge_content_change arrives. (Is this overly brittle?) Second: The whole EphyUtils::FindGtkParent business is somewhat annoying. Warnings on console don't happen any more (EphyUtils::FindGtkParent returns a toplevel or nsnull) but this is still not what we want, as it leads to dialogs not being parented. We only want this to happen in the new-window case. The solution is to check whether the new tab has any siblings; if there are any we set the Gtk parent window on the new tab after removing it from that window. (Strange, yeah, but it's legal and it works.) This fixes comment 6 part 1; part 2 is no longer an issue (grep for EPHY_EMBED_DOCUMENT_OTHER). I'll try to keep this patch buildable.
Created attachment 58610 [details] [review] 144025.bugzilla.gnome.org-close-tab-for-external.patch The previous patch didn't quite work. This patch takes the strategy of placing a GtkWindow *parentWindow on GContentHandler, kept updated with a weak pointer. This works well: as expected, dialogs from closed new tabs are parented on the current window, and dialogs from closed new windows and new windows launched externally are unparented. I'm actively using and testing this patch; I think it's correct.
Note: this patch applies clean against 1.9.6. Would welcome test reports.
Great. Seems to work fine over here.
We'll try this in 1.11.
*** Bug 320625 has been marked as a duplicate of this bug. ***
Ok, I found a BIG problem with the current patch. If you don't have any epiphany windows open and you click, saym on a link to a PDF file from, say, a mail application, Epiphany opens, shows the download dialog for a moment and INSTANTLY CLOSES before the download finishes. I guess something like "don't close the last tab" (and quit Ephy) should be added.
Reproduced; I had to disable all extensions to get the bug to show up (otherwise the download window appears and stays until the download completes). Also got a segfault, which should help.
Created attachment 62078 [details] [review] 144025.bugzilla.gnome.org-close-tab-for-external.patch Revised patch; refs embed_shell before emitting ge_document_type to ensure that it is available later. Could you see if this patch resolves your issue?
Wouter, any input?
I've not yet looked into this. I don't run Ephy HEAD at the moment...
(In reply to comment #19) > I've not yet looked into this. I don't run Ephy HEAD at the moment... The patch should apply to 2.14 just fine.
Created attachment 66993 [details] [review] 144025.bugzilla.gnome.org-close-tab-for-external.patch Updated to apply cleanly to 2.15.2.
Ok, let's try this. It's not the best possible solution, but we have to make do with what interfaces mozilla does (not) offer us. Ok to commit to HEAD only.
There's a better patch upstream @ https://bugzilla.mozilla.org/show_bug.cgi?id=241972 -> NOTGNOME
Created attachment 82963 [details] [review] 144025.bugzilla.gnome.org-close-tab-for-external.patch Updated for 2.17.91, because epiphany moves faster than upstream.
This is fixed upstream in gecko 1.8.1.