Add support for regional Ad Block lists #119
Conversation
|
@bbondy I'll switch from using ad_block_regional_updater_list.h and just retrieve the public key and component id from ad block's regions.h instead (just let me know if my approach in the current pull request I have open against ad-block is acceptable!) Figured I'd get this review in front of you now, though. |
emerick
force-pushed
the
regional-ad-block
branch
from
d26ded4
to
db0d033
Compare
emerick
changed the title
|
@bbondy This one's ready for review again! |
| [locale](const FilterList& filter_list) { | ||
| return std::find_if(filter_list.langs.begin(), | ||
| filter_list.langs.end(), | ||
| [locale](const std::string& lang) { |
There was a problem hiding this comment.
I think you could capture locale by reference for the lambda here with [&locale]
|
|
||
| void AdBlockRegionalService::Cleanup() { | ||
| AdBlockBaseService::Cleanup(); | ||
| } |
There was a problem hiding this comment.
You can just remove the override completely here
| } | ||
|
|
||
| AdBlockService::~AdBlockService() { | ||
| Cleanup(); | ||
| } | ||
|
|
||
| void AdBlockService::Cleanup() { | ||
| ad_block_client_.reset(); | ||
| AdBlockBaseService::Cleanup(); | ||
| } |
There was a problem hiding this comment.
Ditto here, you can remove this Cleanup completely.
| } | ||
|
|
||
| AdBlockRegionalService::~AdBlockRegionalService() { | ||
| Cleanup(); |
There was a problem hiding this comment.
I think the base class will already pick this up, so no need to call Cleanup here.
| g_ad_block_component_base64_public_key_), | ||
| ad_block_client_(new AdBlockClient()), | ||
| weak_factory_(this) { | ||
| AdBlockService::AdBlockService() { | ||
| } | ||
|
|
||
| AdBlockService::~AdBlockService() { | ||
| Cleanup(); |
There was a problem hiding this comment.
I think the base class will already pick this up, so no need to call Cleanup here.
| </script> | ||
|
|
||
| Ad banner: | ||
| <img id='adImage' src='/ad_fr.png'> |
There was a problem hiding this comment.
Just making sure the normal adblock will not pick this up as well. Pls verify if you didn't already.
There was a problem hiding this comment.
Yep, I verified that.
emerick
force-pushed
the
regional-ad-block
branch
from
db0d033
to
42fba3f
Compare
|
@bbondy Updated with code review feedback. |
Fixes #118, order_by ordering
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist: