----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100683/#review1519 -----------------------------------------------------------
I am still not a fan of the patch. I think its complexity is high for what it adds, we can probably do better. I also don't like the idea of adding a branch because there is a rule, but it is invalid. It does not look very clean IMHO. Alternative ideas: -add a AdBlockRuleNullImpl that always return false for ::match(). And create that in AdBlockRule::AdBlockRule() if there are any unsupported options. -or add AdBlockRule::isValid(). And do not set the m_implementation if there are any unsupported option. The AdBlockManager would not add a rule if it is not valid, so they are never evaluated. I am also not a fan of comparing lots of string for each rules. What about keeping a QSet of unsupported options? - Benjamin On Feb. 19, 2011, 6:46 p.m., Andrea Diamantini wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/100683/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2011, 6:46 p.m.) > > > Review request for rekonq and Benjamin Poulain. > > > Summary > ------- > > AdBlock clean up. > With this patch we explicitely allow any option that has not been (yet) fully > implemented. > > > This addresses bugs 248045, 253329 and 265909. > /show_bug.cgi?id=248045 > /show_bug.cgi?id=253329 > /show_bug.cgi?id=265909 > > > Diffs > ----- > > src/adblock/adblockrulefallbackimpl.h ec10ee5 > src/adblock/adblockrulefallbackimpl.cpp ae0e14d > > Diff: http://git.reviewboard.kde.org/r/100683/diff > > > Testing > ------- > > > Thanks, > > Andrea > >
_______________________________________________ rekonq mailing list [email protected] https://mail.kde.org/mailman/listinfo/rekonq
