After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748339 - Notifications permissions not remembered
Notifications permissions not remembered
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Web Applications
3.18.x (obsolete)
Other Linux
: Normal major
: ---
Assigned To: Gustavo Noronha (kov)
Epiphany Maintainers
: 765321 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-04-22 23:17 UTC by javiermon
Modified: 2017-01-27 15:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Save decision to allow or deny notifications (59.54 KB, patch)
2015-12-09 16:14 UTC, Gustavo Noronha (kov)
none Details | Review
Save decision to allow or deny notifications (21.90 KB, patch)
2015-12-22 20:10 UTC, Gustavo Noronha (kov)
none Details | Review
Save decision to allow or deny notifications (21.82 KB, patch)
2016-01-27 20:36 UTC, Gustavo Noronha (kov)
none Details | Review
Save decision to allow or deny notifications (22.68 KB, patch)
2016-01-31 17:04 UTC, Gustavo Noronha (kov)
none Details | Review
Save decision to allow or deny notifications (17.84 KB, patch)
2016-09-28 10:29 UTC, Gustavo Noronha (kov)
committed Details | Review

Description javiermon 2015-04-22 23:17:01 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,
Comment 1 Carlos Garcia Campos 2015-04-23 07:13:27 UTC
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.
Comment 2 Claudio Saavedra 2015-04-23 14:45:53 UTC
Summoning Allan.
Comment 3 kapouer 2015-07-06 21:59:55 UTC
Before designing an UI for this, maybe a gsetting could be used to store the info and restore it ?
Comment 4 Jean-François Fortin Tam 2015-07-21 18:34:59 UTC
FWIW, calendar.google.com, with its "Gentle Notifications" (in the "Labs" settings) is a good testcase for this.
Comment 5 scorpy_sk 2015-08-11 08:27:32 UTC
I would suggest that web applications (epiphany running with --application-mode) have notifications enabled by default.
Comment 6 Andrei Cristian Petcu 2015-08-12 20:51:33 UTC
(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.
Comment 7 Michael Catanzaro 2015-08-14 16:50:07 UTC
I also agree with that suggestion, but we still need UI for notifications permissions regardless.
Comment 8 Andrei Cristian Petcu 2015-08-18 07:21:52 UTC
(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
Comment 9 Gustavo Noronha (kov) 2015-12-09 16:14:42 UTC
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.
Comment 10 Michael Catanzaro 2015-12-09 16:33:25 UTC
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
Comment 11 Gustavo Noronha (kov) 2015-12-19 16:06:41 UTC
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?
Comment 12 Michael Catanzaro 2015-12-19 16:18:52 UTC
(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!
Comment 13 Gustavo Noronha (kov) 2015-12-22 20:10:33 UTC
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.
Comment 14 Michael Catanzaro 2015-12-23 01:37:23 UTC
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?
Comment 15 Michael Catanzaro 2016-01-08 01:00:09 UTC
(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.
Comment 16 Gustavo Noronha (kov) 2016-01-26 15:41:55 UTC
Sorry for the delay, I had an extended vacation and just came back. I'll update the patch and upload a new one tonight =)
Comment 17 Gustavo Noronha (kov) 2016-01-27 20:36:24 UTC
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.
Comment 18 Michael Catanzaro 2016-01-28 00:04:08 UTC
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"
Comment 19 Carlos Garcia Campos 2016-01-28 08:04:04 UTC
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));
Comment 20 Gustavo Noronha (kov) 2016-01-31 17:04:19 UTC
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.
Comment 21 Gustavo Noronha (kov) 2016-01-31 17:04:43 UTC
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.
Comment 22 Carlos Garcia Campos 2016-02-08 07:36:24 UTC
(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.
Comment 23 Carlos Garcia Campos 2016-02-08 07:47:29 UTC
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.
Comment 24 Michael Catanzaro 2016-04-20 15:06:18 UTC
*** Bug 765321 has been marked as a duplicate of this bug. ***
Comment 25 Michael Catanzaro 2016-04-29 16:28:26 UTC
*** Bug 765321 has been marked as a duplicate of this bug. ***
Comment 26 Gustavo Noronha (kov) 2016-09-28 10:29:58 UTC
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.
Comment 27 Michael Catanzaro 2016-09-28 11:30:28 UTC
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.
Comment 28 Gustavo Noronha (kov) 2016-09-28 12:40:28 UTC
Attachment 336424 [details] pushed as 6b1344e - Save decision to allow or deny notifications
Comment 29 Gustavo Noronha (kov) 2016-09-28 12:41:21 UTC
I'll deal with the string freeze break request and push to 3.20 after my hackfest talk =)
Comment 30 Dirk Möbius 2017-01-26 17:49:50 UTC
Is this fix already pushed to gnome-3-22? Because it doesn't seem to work in my installation (epiphany 3.22.5, Arch).
Comment 31 Michael Catanzaro 2017-01-26 18:26:59 UTC
(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.
Comment 32 Michael Catanzaro 2017-01-26 18:27:26 UTC
(In reply to Michael Catanzaro from comment #31) 
> Yes, it is fixed in 3.22.5, 

(Actually 3.22.1)
Comment 33 Dirk Möbius 2017-01-27 12:37:14 UTC
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?
Comment 34 Michael Catanzaro 2017-01-27 15:02:24 UTC
I've just made bug #777835.
Comment 35 Dirk Möbius 2017-01-27 15:59:53 UTC
Thanks! :)