GNOME Bugzilla – Bug 660154
Renew adblock extension based on Midori's one
Last modified: 2011-12-03 20:06:09 UTC
Created attachment 197498 [details] [review] Patch implementing this idea As discussed in the ephiphany-list mailing list some time ago [1], the idea of evolving current Ephy's adblock extension into something more similar to Midori's one has been "blowing in the wind" for some time now, so it would be good to move on that direction. Unfortunately, even though my original idea was to have it ready for GNOME 3.2, that could not be due to different problems (mainly time constraints due to other commitments). Good news is that I finally manage to put some time together to finish a first version of the new adblocker, which I'm now attaching the patch for. Basically, I implemented what I said in [1], that is, a version of Midori's adblocker for Ephy that also allows to add and remove adblock Plus subscriptions as Midori does. Still, there's no possibility yet to manually add/remove individual rules as in the former version of the extension, but I guess that could be added later, if there's enough interest for that. Eager to get some feedback, Mario [1] http://mail.gnome.org/archives/epiphany-list/2011-June/msg00002.html
Review of attachment 197498 [details] [review]: So, I tested this for a bit. Seems to work pretty well, so great work! A few comments: - I think it would have been better to have two patches, one removing the old stuff and another adding the new code. Easier to review? - You are adding extra noise by changing from tabs to spaces in some places. Since you are not doing it thoroughly (I think), it's not OK, either change the file completely (but then please do the two patch thing) or keep the current style. - config.h always needs to be the first header included. You are changing this in a few places. Also, read the HACKING file in epiphany for our preferred header include style. Some other minor comments: ::: extensions/adblock/adblock-ui.c @@ +77,1 @@ } I think we can move all this stuff to G_DEFINE_TYPE (especially the new UriTester class, no point in adding new GObject boilerplate to the repo). @@ +122,2 @@ + gtk_tree_model_get (model, iter, COL_FILTER_URI, &filter, -1); + *filters = g_slist_append (*filters, filter); Micro-optimization, but unless this is relevant somehow you can probably keep the prepend like in the old code. @@ +168,3 @@ + const char *new_filter = gtk_entry_get_text (dialog->priv->new_filter); + + if (new_filter && ADBLOCK_FILTER_VALID (new_filter)) the macro already checks for filter != NULL @@ +190,3 @@ + _("Invalid filter")); + gtk_dialog_run (GTK_DIALOG (error_dialog)); + gtk_widget_destroy (error_dialog); Dialogs inside dialogs! I guess this can go as-is, but ideally I'd rather have the adblock UI validate the URL in place and warn if it's wrong. @@ +210,1 @@ } new_filter_cb and add_cb could be the same callback, I guess. @@ +277,1 @@ { I think you can just use the 'constructed' vmethod instead of chaining up to to the constructor here. Easier. @@ +389,1 @@ } Seems we don't need finalize. ::: extensions/adblock/ephy-adblock-extension.c @@ +30,1 @@ +#include <epiphany/epiphany.h> If you are including epiphany.h you can get rid of the other ephy headers I suppose. @@ +131,3 @@ + dirname = g_build_filename (ephy_dot_dir (), "extensions", "data", "adblock", NULL); + g_mkdir_with_parents (dirname, 0775); + g_free (dirname); I think there's code for this in URITester, it should not be duplicated. @@ +195,1 @@ + return !uri_tester_test_uri (self->priv->tester, url, address, type); Seems we are getting rid of the code to temporarily disable ad blocking? (ad_blocker_should_block). You don't think it's useful? ::: extensions/adblock/extension.c @@ +20,1 @@ * $Id$ You can remove this CVS leftovers while you are at it. ::: extensions/adblock/uri-tester.c @@ +29,3 @@ +#include "config.h" +#include "ephy-debug.h" +#include "ephy-file-helpers.h" the .h includes epiphany.h, so redundant. @@ +36,3 @@ +#include <string.h> +#include <webkit/webkit.h> +#include <webkit/webkitdownload.h> redundant because you have webkit.h @@ +118,3 @@ + + return folder; +} This also creates the dir, so the name is a bit misguiding. I think the getter should only return the directory, and the creating should happen as part of the object creation process? @@ +152,3 @@ + return; + + const char *dest; Move this up, likely generates a warning. @@ +351,3 @@ + * @tester: an UriTester + * + * Reload patterns from zero s/zero/scratch/ ?
Created attachment 201883 [details] [review] 1. Remove the old 'Ad Blocker' extension and its documentation. This is the previous step to provide the patch with the new version of the extension, to be named 'Ad Blocker v2'.
Created attachment 201884 [details] [review] 2. Add the new 'Ad Blocker' extension, namely 'Ad Blocker v2'. Based on Midori's adblocker, this new version of the extension has been written to be as simple as possible and compatible with the EasyList subscriptions for 'AdBlock Plus' (http://easylist.adblockplus.org).
Hi again, First of all, thanks a lot for the review and sorry not to have been able to work on this before, but I've been damm busy lately (we all are, I guess) and could not find the right moment for it. Fortunately I did find that moment today, so here you have the new patch. Still, see my comments to your comments below... (In reply to comment #1) > [...] > So, I tested this for a bit. Seems to work pretty well, so great work! Thank you! > A few comments: > > - I think it would have been better to have two patches, one removing the old > stuff and another adding the new code. Easier to review? Done, as you can see in the two patches I added now. > - You are adding extra noise by changing from tabs to spaces in some places. > Since you are not doing it thoroughly (I think), it's not OK, either change the > file completely (but then please do the two patch thing) or keep the current > style. That was the result of the previous patch being more something to get feedback from than an actual patch, so I put not much attention on some details yet. And the fact that some parts of the patch came from the original adblocker, some others from Midori and some others form myself didn't help either to "consistency". So, my bad, sorry about that. Fortunately I tried to fix those issues as much as possible now, so hope the new patch is better :-) > - config.h always needs to be the first header included. You are changing this > in a few places. Also, read the HACKING file in epiphany for our preferred > header include style. Fixed (both the config.h thing and other details as specified in HACKING) > Some other minor comments: > > ::: extensions/adblock/adblock-ui.c > @@ +77,1 @@ > } > > I think we can move all this stuff to G_DEFINE_TYPE (especially the new > UriTester class, no point in adding new GObject boilerplate to the repo). Done. I removed all the boilerplate in .c files and replaced them with G_DEFINE_DYNAMIC_TYPE (G_DEFINE_TYPE would not work since extensions can be loaded, unloaded and re-loaded at any time). It definitely looks cleaner now! > @@ +122,2 @@ > + gtk_tree_model_get (model, iter, COL_FILTER_URI, &filter, -1); > + *filters = g_slist_append (*filters, filter); > > Micro-optimization, but unless this is relevant somehow you can probably keep > the prepend like in the old code. Done. Also, I changed and algorithm in other place (uri-tester.c) using g_slist_append all the time to create a big list, so it now uses g_slist_prepend all the time and g_slist_reverse when it's done. > @@ +168,3 @@ > + const char *new_filter = gtk_entry_get_text > (dialog->priv->new_filter); > + > + if (new_filter && ADBLOCK_FILTER_VALID (new_filter)) > > the macro already checks for filter != NULL Done. > @@ +190,3 @@ > + _("Invalid filter")); > + gtk_dialog_run (GTK_DIALOG (error_dialog)); > + gtk_widget_destroy (error_dialog); > > Dialogs inside dialogs! I guess this can go as-is, but ideally I'd rather have > the adblock UI validate the URL in place and warn if it's wrong. Agree it's not the best UX design in the world, but also agree we can live with this for now and try to improve it later on, so keep it that way for the moment. > @@ +210,1 @@ > } > > new_filter_cb and add_cb could be the same callback, I guess. Sure! Fixed. > @@ +277,1 @@ > { > > I think you can just use the 'constructed' vmethod instead of chaining up to to > the constructor here. Easier. Argh! I think I forgot about this part in the currently attached patch. Sorry. Please feel free to point it out in the next review if you still feel it makes sense to change it. Otherwise, I will probably do it anyway... :-) > @@ +389,1 @@ > } > > Seems we don't need finalize. You're right. Removed. > ::: extensions/adblock/ephy-adblock-extension.c > @@ +30,1 @@ > +#include <epiphany/epiphany.h> > > If you are including epiphany.h you can get rid of the other ephy headers I > suppose. Yep. Removed! > @@ +131,3 @@ > + dirname = g_build_filename (ephy_dot_dir (), "extensions", "data", > "adblock", NULL); > + g_mkdir_with_parents (dirname, 0775); > + g_free (dirname); > > I think there's code for this in URITester, it should not be duplicated. Actually, I don't know why in hell I have this code here, as it only really matters in UriTester. Hence, removed this snippet from here. > @@ +195,1 @@ > + return !uri_tester_test_uri (self->priv->tester, url, address, type); > > Seems we are getting rid of the code to temporarily disable ad blocking? > (ad_blocker_should_block). You don't think it's useful? It's not that I think it's not useful, but that I think it was not needed at this point of this implementation, so I kept it out of the patch in order to get a cleaner one. But as with the "dialog inside dialogs!" thing, this could probably be something that could be improved in the future. What do you think? > ::: extensions/adblock/extension.c > @@ +20,1 @@ > * $Id$ > > You can remove this CVS leftovers while you are at it. Yep. Removed. > ::: extensions/adblock/uri-tester.c > @@ +29,3 @@ > +#include "config.h" > +#include "ephy-debug.h" > +#include "ephy-file-helpers.h" > > the .h includes epiphany.h, so redundant. It's actually *not* redundant, since neither ephy-debug.h nor ephy-file-helpers.h get included along with epiphany.h, so I need to explicitly include those anyway. So, kept as it is. > @@ +36,3 @@ > +#include <string.h> > +#include <webkit/webkit.h> > +#include <webkit/webkitdownload.h> > > redundant because you have webkit.h Sure. Removed. > @@ +118,3 @@ > + > + return folder; > +} > > This also creates the dir, so the name is a bit misguiding. I think the getter > should only return the directory, and the creating should happen as part of the > object creation process? Argh! Another thing I forgot... Added to the TODO list for the next version of the patch. Sorry about it as well. > @@ +152,3 @@ > + return; > + > + const char *dest; > > Move this up, likely generates a warning. And yet another thing I forgot about... added to my TODO list too (I should probably get some sleep) > @@ +351,3 @@ > + * @tester: an UriTester > + * > + * Reload patterns from zero > > s/zero/scratch/ ? Sure! So that's all for now. If possible, I will try to write a new version of the patch with the things I forgot about asap, or perhaps will wait for your review to incorporate those to the changes that I will likely have to perform anyway :-). Again, thanks for the review and patience
Review of attachment 201883 [details] [review]: Boom.
Review of attachment 201884 [details] [review]: I'm fine with having this basically as-is upstream. It's much better than what we have now. Just a bunch of nitpicks follow, and I haven't really looked closely at the UriTester logic. ::: extensions/adblock/adblock-ui.c @@ +34,3 @@ + (G_TYPE_INSTANCE_GET_PRIVATE((object), \ + TYPE_ADBLOCK_UI, \ + AdblockUIPrivate)) Nitpick mode: maximum level I think this can be just one line. @@ +116,3 @@ + + /* Ask manager to emit a signal that rules have changed. */ + manager = EPHY_ADBLOCK_MANAGER ( No extra line? @@ +292,3 @@ + guint n_construct_properties, + GObjectConstructParam *construct_params) +{ As suggested earlier, it's easier to just use 'constructed' here. @@ +363,3 @@ + GObjectClass *object_class = G_OBJECT_CLASS (klass); + + adblock_ui_parent_class = g_type_class_peek_parent (klass); This is not needed. ::: extensions/adblock/ephy-adblock-extension.c @@ +32,3 @@ +#include <gtk/gtk.h> + +#define EPHY_ADBLOCK_EXTENSION_GET_PRIVATE(object) \ Also just one line? @@ +98,3 @@ + g_object_unref (extension->priv->ui); + extension->priv->ui = NULL; + } Something I learned today: we can use g_clear_object here. @@ +104,3 @@ + g_object_unref (extension->priv->tester); + extension->priv->tester = NULL; + } And here. ::: extensions/adblock/uri-tester.c @@ +39,3 @@ +#define UPDATE_FREQUENCY 24 * 60 * 60 /* In seconds */ + +#define URI_TESTER_GET_PRIVATE(object) \ Same as usual.
(In reply to comment #5) > Review of attachment 201883 [details] [review]: > > Boom. Bang! (In reply to comment #6) > Review of attachment 201884 [details] [review]: > > I'm fine with having this basically as-is upstream. It's much better than what > we have now. Just a bunch of nitpicks follow, and I haven't really looked > closely at the UriTester logic. Ok, I'll address those issues then before pushing. > ::: extensions/adblock/adblock-ui.c > @@ +34,3 @@ > + (G_TYPE_INSTANCE_GET_PRIVATE((object), \ > + TYPE_ADBLOCK_UI, \ > + AdblockUIPrivate)) > > Nitpick mode: maximum level > > I think this can be just one line. Fixed. > @@ +116,3 @@ > + > + /* Ask manager to emit a signal that rules have changed. */ > + manager = EPHY_ADBLOCK_MANAGER ( > > No extra line? Fixed. > @@ +292,3 @@ > + guint n_construct_properties, > + GObjectConstructParam *construct_params) > +{ > > As suggested earlier, it's easier to just use 'constructed' here. Fixed. > @@ +363,3 @@ > + GObjectClass *object_class = G_OBJECT_CLASS (klass); > + > + adblock_ui_parent_class = g_type_class_peek_parent (klass); > > This is not needed. Removed. > ::: extensions/adblock/ephy-adblock-extension.c > @@ +32,3 @@ > +#include <gtk/gtk.h> > + > +#define EPHY_ADBLOCK_EXTENSION_GET_PRIVATE(object) \ > > Also just one line? Fixed. > @@ +98,3 @@ > + g_object_unref (extension->priv->ui); > + extension->priv->ui = NULL; > + } > > Something I learned today: we can use g_clear_object here. Very convenient. Change done. > @@ +104,3 @@ > + g_object_unref (extension->priv->tester); > + extension->priv->tester = NULL; > + } > > And here. Fixed. > ::: extensions/adblock/uri-tester.c > @@ +39,3 @@ > +#define UPDATE_FREQUENCY 24 * 60 * 60 /* In seconds */ > + > +#define URI_TESTER_GET_PRIVATE(object) \ > > Same as usual. Fixed.
This problem has been fixed in the development version. The fix will be available in the next major software release. See the commit in git.gnome.org: http://git.gnome.org/browse/epiphany-extensions/commit/?id=f9fb2ae8787193eefa6d96fbe5769551cf99041b