Support Acceptable Ads

Support Acceptable Ads

[Eric, 2014-06-25 14:34:15]

LGTM

The comments I have involve concerns larger than the feature in this review
itself.

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/engine/Main.cpp:122: }
Why the partial type enforcement? Is there any reason not just to call
AsString() directly?

Personally, I'm a believer in strict type enforcement. (That's a policy choice:
Don't mask errors in code you have complete control over.) That would mean
throwing an exception here, but there's not even try-catch block around the body
of HandleRequest(), much less a way of delivering an exception as a result
rather than an ordinary return value.

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
File src/plugin/PluginSettings.cpp (left):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/plugin/PluginSettings.cpp:319: CString CPluginSettings::GetSubscription()
FYI. The return type of this function has changed to std::wstring as part of the
ATL rewrite.

[Felix Dahlke, 2014-06-30 17:24:04]

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/loca...
File locales/en.ini (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/loca...
locales/en.ini:51: menu-acceptable-ads=Enable some non-intrusive ads
"Allow some non-intrusive advertising" is our usual way to put this, would vote
for that.

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/engine/Main.cpp:122: }
On 2014/06/25 14:34:15, Eric wrote:
> Why the partial type enforcement? Is there any reason not just to call
> AsString() directly?
> 
> Personally, I'm a believer in strict type enforcement. (That's a policy
choice:
> Don't mask errors in code you have complete control over.) That would mean
> throwing an exception here, but there's not even try-catch block around the
body
> of HandleRequest(), much less a way of delivering an exception as a result
> rather than an ordinary return value.

I guess this is because the pref could be missing, but we still want to switch
language subscriptions properly? Wouldn't JsValue::IsNull() be a better check
for that?

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/engine/Main.cpp:125: // Remove all subscriptions, besides the Acceptable Ads
Wow, this is hacky. So we currently only support exactly one subscription?
This'll do the trick for now, but we should really file a follow-up issue for
implementing language switching properly.

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/engine/Main.cpp:128: if
(subscriptions[i].get()->GetProperty("url")->AsString() != aaUrl)
Are you sure the .get() is required here? That should be
std::tr1::shared_ptr::get - that'd be redundant.

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/engine/Main.cpp:134: // Add new subscription
Kinda obvious IMHO :P

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
File src/plugin/AdblockPlus.rc (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/plugin/AdblockPlus.rc:42: VALUE "LegalCopyright", "Copyright 2014"
Unrelated, can we put this in a separate commit? Doesn't really need a review.

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:347: bool
CAdblockPlusClient::AcceptableAdsStatus()
How about IsAcceptableAdsEnabled()? That'd make the comment redundant.

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/plugin/AdblockPlusClient.cpp:351: for
(std::vector<SubscriptionDescription>::iterator subscription =
subscriptions.begin(); subscription < subscriptions.end(); subscription ++)
!= for iterators, not <

Also no whitespace before "++"

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:22: 
I liked it better with just one line of whitespace here :) Unrelated anyway.

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:1453: // Are Acceptable Ads enabled?
This comment can also go if the function is called "IsAcceptableAdsEnabled()",
eh?

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
File src/plugin/PluginSettings.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/plugin/PluginSettings.cpp:324: for
(std::vector<SubscriptionDescription>::iterator subscription =
subscriptions.begin(); subscription < subscriptions.end(); subscription ++)
"<" -> "!=" and "subscription ++" -> "subscription++" again.

[Eric, 2014-07-18 17:02:11]

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/loca...
File locales/en.ini (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/loca...
locales/en.ini:51: menu-acceptable-ads=Enable some non-intrusive ads
Do we have a style guide somewhere with recommended phrases etc.? It could act
as a specification for auditing UI things like this.

[Oleksandr, 2014-07-18 19:06:41]

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/...
src/engine/Main.cpp:125: // Remove all subscriptions, besides the Acceptable Ads
https://issues.adblockplus.org/ticket/1084
On 2014/06/30 17:24:04, Felix H. Dahlke wrote:
> Wow, this is hacky. So we currently only support exactly one subscription?
> This'll do the trick for now, but we should really file a follow-up issue for
> implementing language switching properly.

[Eric, 2014-07-18 19:26:48]

http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/...
src/engine/Main.cpp:119: if (valuePtr->IsNull())
Missing a ! negation?

[Oleksandr, 2014-07-18 19:44:00]

http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/...
src/engine/Main.cpp:119: if (valuePtr->IsNull())
Wow! That's awkward now :D
On 2014/07/18 19:26:48, Eric wrote:
> Missing a ! negation?

[Eric, 2014-07-18 19:52:10]

http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/...
src/engine/Main.cpp:119: if (valuePtr->IsNull())
There's always the infrequently used !!! operator, for when you really, really
want that value negated.

[Felix Dahlke, 2014-07-23 05:31:26]

LGTM