GNOME Bugzilla – Bug 748339
Notifications permissions not remembered
Last modified: 2017-01-27 15:59:53 UTC
Hi I'm using web 3.16.0 in fedora 22 beta (x86_64) and everytime I visit a page that supports html desktop notifications I have to gran the web permissions to access them, i.e. my chosen option is not remembered. This happens in both webapps and accesing the web app url with regular web (epiphany) application. Thanks,
Yes, we should probably add the option "Always for this host" or something like that. Bu then, we would probably need a way to admin all the permissions, so that you can revert a decision.
Summoning Allan.
Before designing an UI for this, maybe a gsetting could be used to store the info and restore it ?
FWIW, calendar.google.com, with its "Gentle Notifications" (in the "Labs" settings) is a good testcase for this.
I would suggest that web applications (epiphany running with --application-mode) have notifications enabled by default.
(In reply to scorpy_sk from comment #5) > I would suggest that web applications (epiphany running with > --application-mode) have notifications enabled by default. That is a great idea! That will make Gnome Web a lot more popular for Web Apps and it seems natural.
I also agree with that suggestion, but we still need UI for notifications permissions regardless.
(In reply to Michael Catanzaro from comment #7) > I also agree with that suggestion, but we still need UI for notifications > permissions regardless. Ok, I made a new bug report for the notifications by default on web apps #753744
Created attachment 317046 [details] [review] Save decision to allow or deny notifications The decision is stored in the hosts history database, alongside the zoom level. A new management dialog has been added to forget decisions. This dialog should probably be shared with other permission decisions which we should also remember such as geolocation, in the future.
First impressions of the patch: * I think the 'p' in "Manage permissions..." should be uppercase, to match the other buttons on the dialog. * Privacy is not the right place for these settings. I guess we don't really have a right place, though.... * I see a warning when opening the dialog: (epiphany:27997): Gtk-WARNING **: Attempting to add a widget with type GtkImage to a GtkButton, but as a GtkBin subclass a GtkButton can only contain one widget at a time; it already contains a widget of type GtkLabel
I'm not very happy with either using the history service nor with the management dialog. I am pondering an alternative solution that uses gsettings for saving the decision and that adds 'forget permissions for this page' to the hamburger menu instead, what do you think of those ideas?
(In reply to Gustavo Noronha (kov) from comment #11) > I'm not very happy with either using the history service nor with the > management dialog. I am pondering an alternative solution that uses > gsettings for saving the decision Either way is fine by me. > and that adds 'forget permissions for this > page' to the hamburger menu instead, what do you think of those ideas? This seems fine too!
Created attachment 317799 [details] [review] Save decision to allow or deny notifications The decision is stored in the hosts history database, alongside the zoom level. A new menu item was added to the hamburger menu to allow forgeting permissions.
Review of attachment 317799 [details] [review]: Great job with EphyHostsManager, that's good code! The commit message is wrong now, though. ::: embed/Makefile.am @@ +47,3 @@ ephy-file-monitor.c \ ephy-find-toolbar.c \ + ephy-hosts-manager.c \ Don't hold back on the tabs, you need one more there! ::: embed/ephy-embed-shell.c @@ +1167,3 @@ + + if (!priv->hosts_manager) + priv->hosts_manager = EPHY_HOSTS_MANAGER (g_object_new (EPHY_TYPE_HOSTS_MANAGER, NULL)); Personally, I prefer to create an ephy_hosts_manager_new() function instead of using g_object_new() directly. ::: embed/ephy-hosts-manager.c @@ +15,3 @@ + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. Please grab the newer form of the license header (should be used by all files now) that links to the FSF website instead of giving their mailing address (which is more volatile). @@ +111,3 @@ + backend = g_memory_settings_backend_new (); + else + backend = g_settings_backend_get_default (); You were careful to make sure not to mix incognito mode settings with normal profiles, but ideally each profile would have its own settings, saved in the profile dir. You can get that by using g_keyfile_settings_backend_new() with some filename under ephy_dot_dir(). Then you don't even need the conditional for incognito mode, since incognito mode uses a temporary profile dir. ::: embed/ephy-hosts-manager.h @@ +15,3 @@ + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. Ditto regarding the license header. @@ +34,3 @@ + EPHY_HOST_PERMISSION_DENY = 0, + EPHY_HOST_PERMISSION_ALLOW = 1, +} EphyHostPermission; I like it. We can and will reuse this for much more than just notifications permissions. ::: embed/ephy-web-view.c @@ +1404,3 @@ + webkit_permission_request_allow (decision); + return TRUE; + } else { We prefer not to use else blocks after a return statement, to save indentation and braces. @@ +1458,2 @@ /* Ref the decision, to keep it alive while we decide */ + data = g_slice_new (PermissionRequestData); GSlice is slower than the system allocator nowadays; I always use g_new(). ::: src/ephy-window.c @@ +89,3 @@ { "PagePopupAction", NULL, "" }, { "NotebookPopupAction", NULL, "" }, + { "ForgetPermissions", NULL, N_("_Forget Permissions"), NULL, NULL, Hm, I think we need to think about this more... nobody is going to know what exactly "Forget Permissions" entails, or even that it only affects the current page.... @@ +705,3 @@ +sync_host_settings (EphyHostsManager *hosts_manager, + G_GNUC_UNUSED const char *host, + G_GNUC_UNUSED const char *key, I don't remember needing to use this in Epiphany before; were you hitting -Wunused-parameter?
(In reply to Michael Catanzaro from comment #14) > ::: src/ephy-window.c > @@ +89,3 @@ > { "PagePopupAction", NULL, "" }, > { "NotebookPopupAction", NULL, "" }, > + { "ForgetPermissions", NULL, N_("_Forget Permissions"), NULL, NULL, > > Hm, I think we need to think about this more... nobody is going to know what > exactly "Forget Permissions" entails, or even that it only affects the > current page.... I'd like to land this even without UI forgetting permissions.
Sorry for the delay, I had an extended vacation and just came back. I'll update the patch and upload a new one tonight =)
Created attachment 319879 [details] [review] Save decision to allow or deny notifications The decision is stored using gsettings database. A new menu item was added to the hamburger menu to allow forgeting permissions.
Review of attachment 319879 [details] [review]: I don't like the menu item, but we can improve that in the future. It's more important to land this. accepted-commit-now means accepted-commit-after-waiting-two-days-for-carlos-to-complain-if-he-wants-to. Thank you Gustavo! Hope you had an enjoyable megavacation (1.5 months long? :) ::: embed/ephy-hosts-manager.c @@ +107,3 @@ + return settings; + + key_file = g_strdup_printf ("%s/hosts.ini", ephy_dot_dir ()); "%s" G_DIR_SEPARATOR_S "hosts.ini"
Review of attachment 319879 [details] [review]: ::: data/Makefile.am @@ +15,3 @@ gsettings_ENUM_FILES = $(top_srcdir)/lib/ephy-prefs.h +gsettings_SCHEMAS = org.gnome.epiphany.gschema.xml org.gnome.epiphany.host.gschema.xml Did you forget to git add this new file? ::: embed/ephy-hosts-manager.c @@ +41,3 @@ +{ + manager->hosts_mapping = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); + manager->settings_mapping = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_free); What's the settings mapping for? It seems you add items, but you never lookup items from it. @@ +108,3 @@ + + key_file = g_strdup_printf ("%s/hosts.ini", ephy_dot_dir ()); + backend = g_keyfile_settings_backend_new (key_file, "/", "Hosts"); Do we store any setting in the root "Hosts" group? @@ +114,3 @@ + settings = g_settings_new_with_backend_and_path ("org.gnome.Epiphany.host", backend, host_path); + g_hash_table_insert (manager->hosts_mapping, host, settings); + g_hash_table_insert (manager->settings_mapping, settings, g_strdup (host)); I think there's a problem here. the hosts_mapping hash table doesn't dup/free the key, but we are passing a pointer to a string that will be freed by the autoptr when this function goes out of scope. The settings_mapping table does dup/free, though, so we should dup first, and use the same pointer as the key of both tables (assuming we really need both tables). Or even better, we could manually free the host on previous early return, and pass ownership to the hash table to avoid the unneeded dup/free extra. @@ +122,3 @@ +} + +EphyHostsManager* EphyHostsManager* -> EphyHostsManager * @@ +132,3 @@ + const char *address) +{ + GSettings *settings = ephy_hosts_manager_get_settings_for_address (manager, address); I wonder if we could expose only ephy_hosts_manager_get_settings_for_address and then let the caller use the g_settings api to get/set settings. ::: embed/ephy-hosts-manager.h @@ +26,3 @@ +#define EPHY_TYPE_HOSTS_MANAGER (ephy_hosts_manager_get_type ()) + +G_DECLARE_FINAL_TYPE (EphyHostsManager, ephy_hosts_manager, EPHY, HOSTS_MANAGER, GObject) I love the idea of this generic class to manage per host settings, but I think the name HostsManager sounds too generic, maybe HostsSettingsManager? @@ +34,3 @@ +} EphyHostPermission; + +EphyHostsManager* ephy_hosts_manager_new (void); EphyHostsManager* -> EphyHostsManager *ephy_hosts_manager_new ::: embed/ephy-web-view.c @@ +2685,3 @@ + + scheme = g_uri_parse_scheme (address); + if (!scheme || !(g_str_equal (scheme, "http") || g_str_equal (scheme, "https"))) This is not consistent with the condition to save notifications permissions, that uses ephy_embed_utils_address_has_web_scheme(), we should use the same thing both places. ::: src/ephy-window.c @@ +89,3 @@ { "PagePopupAction", NULL, "" }, { "NotebookPopupAction", NULL, "" }, + { "ForgetPermissions", NULL, N_("_Forget Permissions"), NULL, NULL, I think this is very confusing. It's not obvious what permissions it is talking about, and it's not obvious either that this only affects the host of the current web view URL. @@ +710,3 @@ + EphyWindowPrivate *priv = window->priv; + EphyWebView *view = ephy_embed_get_web_view (priv->active_embed); + GtkActionGroup *action_group = priv->action_group; Do we really need this just to save priv-> ? @@ +716,3 @@ + has_saved_permissions = ephy_web_view_has_saved_permissions (view); + action = gtk_action_group_get_action (action_group, "ForgetPermissions"); + gtk_action_set_sensitive (action, has_saved_permissions); I guess this could just be gtk_action_set_sensitive (action, ephy_web_view_has_saved_permissions (view));
Review of attachment 319879 [details] [review]: Thanks! ::: embed/ephy-hosts-manager.c @@ +41,3 @@ +{ + manager->hosts_mapping = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); + manager->settings_mapping = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_free); Oops. It's to be used in settings_changed, I must have broken it in a last minute refactoring because I did test and I can't see how it would work looking up the settings object on the hosts hash? Fixed now. @@ +108,3 @@ + + key_file = g_strdup_printf ("%s/hosts.ini", ephy_dot_dir ()); + backend = g_keyfile_settings_backend_new (key_file, "/", "Hosts"); No, I used "/" because there's no need to protect the namespace and it's less bytes, can change if you think it's better. @@ +114,3 @@ + settings = g_settings_new_with_backend_and_path ("org.gnome.Epiphany.host", backend, host_path); + g_hash_table_insert (manager->hosts_mapping, host, settings); + g_hash_table_insert (manager->settings_mapping, settings, g_strdup (host)); The host for hosts_mapping is not kept by the hash table though, it's hashed and not used after that, right? I guess I can manually free but I would prefer using autofree where possible. @@ +132,3 @@ + const char *address) +{ + GSettings *settings = ephy_hosts_manager_get_settings_for_address (manager, address); I'd prefer too keep the GSettings an implementation detail if possible, and I think it also makes it simpler to understand at the call sites.
Created attachment 320137 [details] [review] Save decision to allow or deny notifications The decision is stored using gsettings database. A new menu item was added to the hamburger menu to allow forgeting permissions.
(In reply to Gustavo Noronha (kov) from comment #20) > Review of attachment 319879 [details] [review] [review]: > > Thanks! > > ::: embed/ephy-hosts-manager.c > @@ +41,3 @@ > +{ > + manager->hosts_mapping = g_hash_table_new_full (g_str_hash, g_str_equal, > NULL, g_object_unref); > + manager->settings_mapping = g_hash_table_new_full (g_direct_hash, > g_direct_equal, NULL, g_free); > > Oops. It's to be used in settings_changed, I must have broken it in a last > minute refactoring because I did test and I can't see how it would work > looking up the settings object on the hosts hash? Fixed now. > > @@ +108,3 @@ > + > + key_file = g_strdup_printf ("%s/hosts.ini", ephy_dot_dir ()); > + backend = g_keyfile_settings_backend_new (key_file, "/", "Hosts"); > > No, I used "/" because there's no need to protect the namespace and it's > less bytes, can change if you think it's better. My point is that you can use NULL there if we don't have global settings. > @@ +114,3 @@ > + settings = g_settings_new_with_backend_and_path > ("org.gnome.Epiphany.host", backend, host_path); > + g_hash_table_insert (manager->hosts_mapping, host, settings); > + g_hash_table_insert (manager->settings_mapping, settings, g_strdup > (host)); > > The host for hosts_mapping is not kept by the hash table though, it's hashed > and not used after that, right? I guess I can manually free but I would > prefer using autofree where possible. No, GHashTable keeps a pointer to the key. > @@ +132,3 @@ > + const char > *address) > +{ > + GSettings *settings = ephy_hosts_manager_get_settings_for_address > (manager, address); > > I'd prefer too keep the GSettings an implementation detail if possible, and > I think it also makes it simpler to understand at the call sites. OK.
Review of attachment 320137 [details] [review]: ::: embed/ephy-hosts-manager.h @@ +34,3 @@ +} EphyHostPermission; + +EphyHostsManager* ephy_hosts_manager_new (void); EphyHostsManager* -> EphyHostsManager *ephy_hosts_manager_new ::: embed/ephy-web-view.c @@ +1371,3 @@ + if (ephy_embed_utils_address_has_web_scheme (address)) { + EphyHostsManager *hosts_manager = ephy_embed_shell_get_hosts_manager (ephy_embed_shell_get_default ()); + ephy_hosts_manager_set_notifications_permission_for_address ( I've just realized that we are remembering this unconditionally. I think we should add a checkbox to the info bar and only save when marked. Is this decision saved per session automatically? because otherwise I would save it per session by default and permanently when checked by the user. @@ +1380,3 @@ gtk_widget_destroy (info_bar); + g_object_unref (data->request); + g_slice_free (PermissionRequestData, data); This was allocated by g_new, it should be g_free. @@ +2685,3 @@ + + scheme = g_uri_parse_scheme (address); + if (!scheme || !(g_str_equal (scheme, "http") || g_str_equal (scheme, "https"))) Again, why not using ephy_embed_utils_address_has_web_scheme? since that's what we are using to decide whether so save permissions or not. ::: src/ephy-window.c @@ +90,3 @@ { "NotebookPopupAction", NULL, "" }, + { "ForgetPermissions", NULL, N_("_Forget Permissions"), NULL, NULL, + G_CALLBACK (window_cmd_forget_permissions) }, I still think this is very confusing. I would add this to the privacy tab in the preferences dialog somehow.
*** Bug 765321 has been marked as a duplicate of this bug. ***
Created attachment 336424 [details] [review] Save decision to allow or deny notifications As we discussed on IRC, this feature is something we should really, really have. We will need a redesign for the UI, especially now that there have been many changes to the browser design. So we decided to go ahead with saving the decision now so we are not stuck with the UI design.
Review of attachment 336424 [details] [review]: OK, great. Please also commit this to gnome-3-22; it's more code than I like to add to a stable release, but this is important. When you commit, you should request permission from gnome-i18n@gnome.org since the new gsettings schema is technically a string freeze break, but if you mention that the string is only user-visible in dconf-editor I think it will be accepted. ::: embed/ephy-hosts-manager.c @@ +96,3 @@ +{ + char *host = ephy_string_get_host_name (address); + g_autofree char *key_file = NULL; Hm, we don't use autofrees in Epiphany, please free these normally. ::: embed/ephy-web-view.h @@ +76,3 @@ gboolean ephy_web_view_get_is_blank (EphyWebView *view); gboolean ephy_web_view_is_overview (EphyWebView *view); +gboolean ephy_web_view_has_saved_permissions (EphyWebView *view); This is unused in this version of the patch, right? Please get rid of it.
Attachment 336424 [details] pushed as 6b1344e - Save decision to allow or deny notifications
I'll deal with the string freeze break request and push to 3.20 after my hackfest talk =)
Is this fix already pushed to gnome-3-22? Because it doesn't seem to work in my installation (epiphany 3.22.5, Arch).
(In reply to Dirk Möbius from comment #30) > Is this fix already pushed to gnome-3-22? Because it doesn't seem to work in > my installation (epiphany 3.22.5, Arch). Yes, it is fixed in 3.22.5, but there are other related bugs. Please look at https://bugs.webkit.org/show_bug.cgi?id=163366 to see if you might be hitting that bug. That's fixed in WebKit but still broken in Epiphany as it needs Epiphany implementation, and it won't be backported.
(In reply to Michael Catanzaro from comment #31) > Yes, it is fixed in 3.22.5, (Actually 3.22.1)
Thanks for the explanation. Yes, it seems that I experience exactly the problem, that the WebKit fix has not been backported to Epiphany yet. Is there a bug entry for this Epihany problem, which I can track?
I've just made bug #777835.
Thanks! :)