GNOME Bugzilla – Bug 755193
Add adblock configuration dialog
Last modified: 2018-08-03 20:31:51 UTC
Created attachment 311586 [details] Minimal UI mockup. Now that (Ad) Blocker is integrated and turned on by default, should be a good moment to create a (minimal) UI to control important aspects of it. Goals of the UI mockup in the Attachment: - Easily turn the (Ad) Blocker ON/OFF per domain. - Easily show if the (Ad) Blocker is turned ON/OFF for the active tab's domain. - Permit an easy "fix" for possibly "broken" websites (by turning OFF the Blocker for that domain). Possible future UI goals (TODO): - Inherit the global (Ad) Blocker setting? - Show number of blocked requests? - Choose used list/filters? - ?
I would say we need UI to customize adblock filters and to whitelist domains in Preferences, at least. I'm not sure if we need to be able to disable it easily via a new header bar button. EasyList is popular enough that it's probably safe to treat any site that doesn't work with EasyList as simply broken. The only issue I'm aware of is bug #754954.
Just a few things, Michael: > I would say we need UI to customize adblock filters [...] I think that "to customize filters" is a complexity that do not fits in the simplicity of GNOME Web. > [...] and to whitelist domains in Preferences, at least. > > I'm not sure if we need to be able to disable it easily via a new header bar > button. The idea isn't a "global blocker switch", but a "per-domain blocker status/switch - that easily creates/manages a whitelist/blacklist". Would work like this: - When Blocker is GLOBALLY ON: - All domains are shown with a "PRESSED (header bar) button" = [status]. - Except domains previously added to the WHITELIST, that are shown with a "non-pressed (header bar) button" = [status]. - Domains are added to the whitelist by un-pressing the (header bar) button = [behavior]. - Domains are removed from the whitelist by pressing the (header bar) button = [behavior]. - When Blocker is GLOBALLY OFF: - All domains are shown with a "NON-PRESSED (header bar) button" = [status]. - Except domains previously added to the BLACKLIST, that are shown with a "pressed (header bar) button" = [status]. - Domains are added to the blacklist by pressing the (header bar) button = [behavior]. - Domains are removed from the blacklist by un-pressing the (header bar) button = [behavior]. This way, user would have instant-fine-control no matter how is the global setting; AND there would be no need of a UI for listing/managing the whitelist/blacklist. OBS: If behaving exactly (and only) this way, the button could be replaced by a "Real Switch", of course. > EasyList is popular enough that it's probably safe to treat any site > that doesn't work with EasyList as simply broken. Yes. BUT, (just reinforcing) this is not the only use-case: user also may want to easily support/whitelist a friendly (or punish/blacklist an annoying) website/domain. ---------- Concluding: It is just a first idea. It surely needs of input from Allan & Company, of course.
(In reply to Diogo Campos from comment #2) > Just a few things, Michael: > > > I would say we need UI to customize adblock filters [...] > > I think that "to customize filters" is a complexity that do not fits in the > simplicity of GNOME Web. Unfortunately, it's required or the adblocker simply won't work for many users. Say you live in Isreal, then you must add Isreali adblock filters, because the default EasyList is not going to carry them. We can't put all the filters of all the world in the same list, since that would harm performance, and we can't guess the filters from your configured locale and language preferences, since people travel and it's common to use non-local languages (notably English). > This way, user would have instant-fine-control no matter how is the global > setting; AND there would be no need of a UI for listing/managing the > whitelist/blacklist. Right, I understand. My hesitation is simply that I would prefer not to afford real estate in the headerbar to the adblocker. Back and forward and reload and new tab are way more important. Burying the adblock settings in the preferences, where it's usually out of sight and out of mind, seems better to me (although it makes it harder to change on each site). But this is just my preference.
> Unfortunately, it's required or the adblocker simply won't work for many > users. Say you live in Isreal, then you must add Isreali adblock filters, > because the default EasyList is not going to carry them. We can't put all > the filters of all the world in the same list, since that would harm > performance, and we can't guess the filters from your configured locale and > language preferences, since people travel and it's common to use non-local > languages (notably English). Ok, agreed. Two questions, then: 1- Should be possible to add arbitrary lists or just pre-selected/pre-tested lists? 2- About development, would be better/easier a GTK UI or an HTML5 UI (like "about:applications")? > Right, I understand. My hesitation is simply that I would prefer not to > afford real estate in the headerbar to the adblocker. Back and forward and > reload and new tab are way more important. Burying the adblock settings in > the preferences, where it's usually out of sight and out of mind, seems > better to me (although it makes it harder to change on each site). But this > is just my preference. No problem, could be something like a "Enable/Disable AdBlocker in this website" action item in the Hamburguer Menu. Or an ON/OFF Widget in a future Hamburguer Popover.
(In reply to Diogo Campos from comment #4) > 1- Should be possible to add arbitrary lists or just pre-selected/pre-tested > lists? This is a tough design question. I we think it's possible to have a list of the most interesting adblock subscriptions, then I think a pre-selected list would be better since it would be much easier to use. The disadvantage is that advanced users would have to edit filters.list manually to add a subscription if we missed one. > 2- About development, would be better/easier a GTK UI or an HTML5 UI (like > "about:applications")? I think it would be better to have a GTK+ UI, integrated with the preferences dialog. > No problem, could be something like a "Enable/Disable AdBlocker in this > website" action item in the Hamburguer Menu. Or an ON/OFF Widget in a future > Hamburguer Popover. OK, that would be fine.
*** Bug 685379 has been marked as a duplicate of this bug. ***
*** Bug 771897 has been marked as a duplicate of this bug. ***
*** Bug 776066 has been marked as a duplicate of this bug. ***
Created attachment 343722 [details] [review] Move the adblock filter list to gsettings
Created attachment 343723 [details] [review] Bring back the support for multiple adblock filter files
Review of attachment 343722 [details] [review]: ::: data/org.gnome.epiphany.gschema.xml @@ -187,1 +187,5 @@ </key> + <key type="as" name="adblock-filters"> + <default>['https://easylist-downloads.adblockplus.org/easylist.txt']</default> + <summary>List of Adblock filters</summary> + <description>List of URLs with filter rules to be used by the Adblock.</description> I prefer to use lowercase a rather that uppercase in "adblock". I know you are just matching the key above. Might as well change that now, too. ::: src/profile-migrator/ephy-profile-migrator.c @@ -608,0 +609,20 @@ +migrate_adblock_filters (void) +{ + char *adblock_dir; ... 17 more ... Cleaning up after me, I see... thanks. @@ +637,3 @@ + g_free (adblock_dir); + + if (!g_file_test (filters_filename, G_FILE_TEST_EXISTS)) { I think it would be simpler if you just remove this test. Just let g_file_get_contents() fail down below. I almost suggested you remove the G_FILE_TEST_IS_DIR up above, too, since it's not really needed (none of the subsequent code will fail if the directory doesn't exist), but I know there is some value in bailing at the very top if the adblock directory doesn't exist. But here, I don't see the value. @@ +644,3 @@ + if (!g_file_get_contents (filters_filename, &contents, &content_size, &error)) { + g_free (filters_filename); + g_error_free (error); This error parameter is not checked and not used for anything. You could print it, but the file could be legitimately nonexistent, so I would just pass NULL for the error. That means you can delete the local variable declaration as well, since it's only used here. @@ +653,3 @@ + guint i; + + filter_list = g_strsplit (contents, ";", -1); You forgot to free it with g_strfreev. @@ +712,3 @@ migrate_app_desktop_file_categories, migrate_bookmarks, + migrate_adblock_filters, Not even required, since this feature was never exposed in the UI, but you did it anyway. Excellent, thanks.
Review of attachment 343723 [details] [review]: +1 1906 (keep track) Now I realize we have one remaining problem: if you delete any filter after loading the browser, the embed shell will never download it again, and web extension initialization will hang forever. This is actually a problem with current master as well; we missed that. I wouldn't block on this, because it's not wholly unexpected that users messing with config files while the browser is running could break it, but this is not great. I'm not sure what all we can do about it, though. ::: embed/ephy-embed-shell.c @@ +802,2 @@ + filters = g_settings_get_strv (EPHY_SETTINGS_WEB, EPHY_PREFS_WEB_ADBLOCK_FILTERS); + for (i = 0; filters[i]; i++) { Declare i here, like we do in WebKit. You must have forgot it's 2017 now! ::: embed/web-extension/ephy-uri-tester.c @@ +513,3 @@ ephy_uri_tester_adblock_loaded (EphyUriTester *tester) { + if (g_atomic_int_dec_and_test(&tester->adblock_filters_to_load)) { Missing space before opening parentheses @@ +661,3 @@ + char **filters; + guint i; + GList* monitors = NULL; GList *monitors @@ +676,3 @@ + filters = g_settings_get_strv (EPHY_SETTINGS_WEB, EPHY_PREFS_WEB_ADBLOCK_FILTERS); + tester->adblock_filters_to_load = g_strv_length (filters); + for (i = 0; filters[i]; i++) { guint i = 0
Actually we have another problem I forgot about, bug #776286. What do we do if adblock is disabled. We should not download the filters at all nor block the web process. Lastly, a reminder not to close this bug, since this is not a configuration dialog.
(In reply to Michael Catanzaro from comment #11) > Review of attachment 343722 [details] [review] [review]: > > ::: data/org.gnome.epiphany.gschema.xml > @@ -187,1 +187,5 @@ > </key> > + <key type="as" name="adblock-filters"> > + > <default>['https://easylist-downloads.adblockplus.org/easylist.txt']</ > default> > + <summary>List of Adblock filters</summary> > + <description>List of URLs with filter rules to be > used by the Adblock.</description> > > I prefer to use lowercase a rather that uppercase in "adblock". I know you > are just matching the key above. Might as well change that now, too. Ok, I don't have any preference, I just followed what we had. > ::: src/profile-migrator/ephy-profile-migrator.c > @@ -608,0 +609,20 @@ > +migrate_adblock_filters (void) > +{ > + char *adblock_dir; > ... 17 more ... > > Cleaning up after me, I see... thanks. Yes, we wanted to save space but leaked the filters file in all web apps :-) > @@ +637,3 @@ > + g_free (adblock_dir); > + > + if (!g_file_test (filters_filename, G_FILE_TEST_EXISTS)) { > > I think it would be simpler if you just remove this test. Just let > g_file_get_contents() fail down below. I think that's what I did first and then I changed it, but I don't remember why. > I almost suggested you remove the G_FILE_TEST_IS_DIR up above, too, since > it's not really needed (none of the subsequent code will fail if the > directory doesn't exist), but I know there is some value in bailing at the > very top if the adblock directory doesn't exist. But here, I don't see the > value. > > @@ +644,3 @@ > + if (!g_file_get_contents (filters_filename, &contents, &content_size, > &error)) { > + g_free (filters_filename); > + g_error_free (error); > > This error parameter is not checked and not used for anything. You could > print it, but the file could be legitimately nonexistent, so I would just > pass NULL for the error. That means you can delete the local variable > declaration as well, since it's only used here. My intention was to add a g_warning, but I forgot. > @@ +653,3 @@ > + guint i; > + > + filter_list = g_strsplit (contents, ";", -1); > > You forgot to free it with g_strfreev. Right! > @@ +712,3 @@ > migrate_app_desktop_file_categories, > migrate_bookmarks, > + migrate_adblock_filters, > > Not even required, since this feature was never exposed in the UI, but you > did it anyway. Excellent, thanks. Makes my life easier and hopefully will make easier to add UI for this too.
(In reply to Michael Catanzaro from comment #12) > Review of attachment 343723 [details] [review] [review]: > > +1 1906 (keep track) > > Now I realize we have one remaining problem: if you delete any filter after > loading the browser, the embed shell will never download it again, and web > extension initialization will hang forever. This is actually a problem with > current master as well; we missed that. I wouldn't block on this, because > it's not wholly unexpected that users messing with config files while the > browser is running could break it, but this is not great. I'm not sure what > all we can do about it, though. I'm not sure we should care about that. After the previous patch user no longer need to deal with the adblock dir at all. We can't expect users to delete internal files, what happens if after ephy starts the user deletes the history database, for example, or even when the whole profile dir? In the worst case, the filer will be loaded when ephy is launched again, not a big deal. What we could do, though, is monitor the gsetting now and downloads filters when the setting changes, because users are expected to change the settings, but not the profile dir. > ::: embed/ephy-embed-shell.c > @@ +802,2 @@ > + filters = g_settings_get_strv (EPHY_SETTINGS_WEB, > EPHY_PREFS_WEB_ADBLOCK_FILTERS); > + for (i = 0; filters[i]; i++) { > > Declare i here, like we do in WebKit. You must have forgot it's 2017 now! I never do that in C. WebKit is C++. > ::: embed/web-extension/ephy-uri-tester.c > @@ +513,3 @@ > ephy_uri_tester_adblock_loaded (EphyUriTester *tester) > { > + if (g_atomic_int_dec_and_test(&tester->adblock_filters_to_load)) { > > Missing space before opening parentheses Good catch. > @@ +661,3 @@ > + char **filters; > + guint i; > + GList* monitors = NULL; > > GList *monitors I used to be better at switching coding styles :-( > @@ +676,3 @@ > + filters = g_settings_get_strv (EPHY_SETTINGS_WEB, > EPHY_PREFS_WEB_ADBLOCK_FILTERS); > + tester->adblock_filters_to_load = g_strv_length (filters); > + for (i = 0; filters[i]; i++) { > > guint i = 0
(In reply to Carlos Garcia Campos from comment #15) > > Declare i here, like we do in WebKit. You must have forgot it's 2017 now! > > I never do that in C. WebKit is C++. I do. It's been allowed since C99, GCC allows it even with C89, and it nowadays defaults to C11, so I think nobody is testing anything else. Why not?
(In reply to Carlos Garcia Campos from comment #15) > What we could do, though, is monitor the gsetting now and > downloads filters when the setting changes, because users are expected to > change the settings, but not the profile dir. Yes, we have to do that in any case, to fix bug #776286.
*** Bug 792321 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/epiphany/issues/268.