> On Feb. 19, 2011, 7:56 p.m., Benjamin Poulain wrote: > > 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?
In general, I agree your points. >From the implementation POV, I see some problems/questions to debate. To decide if a string is a null rule (i.e. not implemented one, yet) we have to parse it BEFORE. This basically means IMHO removing the AdBlockRuleTextMatchImpl::isTextMatchFilter() and implement a more general check, being able to decide if the string is a text rule, a fallback one or has some options not yet implemented (i.e. it is not valid or it is a null one). >From the memory side, do not add an invalid rule at all should be better, >while on the "speed" side, this means also removing/changing the checks on the >AdBlockRuleFallbackImpl ctor, to not double parsing every (fallback) rule. What do you think about? - Andrea ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100683/#review1519 ----------------------------------------------------------- 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
