Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(81)

Issue 5338025085108224: Support Acceptable Ads (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by Oleksandr
Modified:
5 years, 3 months ago
Reviewers:
Eric, Felix Dahlke
Visibility:
Public.

Description

Support Acceptable Ads

Patch Set 1 #

Total comments: 15

Patch Set 2 : Cosmetic code fixes. Addressing comments. #

Total comments: 3

Patch Set 3 : Missing negation fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+111 lines, -11 lines) Patch
M locales/en.ini View 1 1 chunk +2 lines, -1 line 0 comments Download
M locales/uk.ini View 1 chunk +1 line, -0 lines 0 comments Download
M src/engine/Main.cpp View 1 2 1 chunk +30 lines, -1 line 0 comments Download
M src/plugin/AdblockPlus.rc View 1 1 chunk +5 lines, -4 lines 0 comments Download
M src/plugin/AdblockPlusClient.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M src/plugin/AdblockPlusClient.cpp View 1 2 chunks +30 lines, -0 lines 0 comments Download
M src/plugin/PluginClass.cpp View 1 2 chunks +27 lines, -0 lines 0 comments Download
M src/plugin/PluginSettings.cpp View 1 1 chunk +10 lines, -4 lines 0 comments Download
M src/plugin/Resource.h View 1 chunk +1 line, -1 line 0 comments Download
M src/shared/Communication.h View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 9
Oleksandr
5 years, 7 months ago (2014-04-14 09:22:09 UTC) #1
Eric
LGTM The comments I have involve concerns larger than the feature in this review itself. ...
5 years, 4 months ago (2014-06-25 14:34:15 UTC) #2
Felix Dahlke
http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/locales/en.ini File locales/en.ini (right): http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/locales/en.ini#newcode51 locales/en.ini:51: menu-acceptable-ads=Enable some non-intrusive ads "Allow some non-intrusive advertising" is ...
5 years, 4 months ago (2014-06-30 17:24:04 UTC) #3
Eric
http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/locales/en.ini File locales/en.ini (right): http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/locales/en.ini#newcode51 locales/en.ini:51: menu-acceptable-ads=Enable some non-intrusive ads Do we have a style ...
5 years, 4 months ago (2014-07-18 17:02:11 UTC) #4
Oleksandr
http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5338025085108224/diff/5629499534213120/src/engine/Main.cpp#newcode125 src/engine/Main.cpp:125: // Remove all subscriptions, besides the Acceptable Ads https://issues.adblockplus.org/ticket/1084 ...
5 years, 4 months ago (2014-07-18 19:06:41 UTC) #5
Eric
http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/engine/Main.cpp#newcode119 src/engine/Main.cpp:119: if (valuePtr->IsNull()) Missing a ! negation?
5 years, 4 months ago (2014-07-18 19:26:48 UTC) #6
Oleksandr
http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/engine/Main.cpp#newcode119 src/engine/Main.cpp:119: if (valuePtr->IsNull()) Wow! That's awkward now :D On 2014/07/18 ...
5 years, 4 months ago (2014-07-18 19:44:00 UTC) #7
Eric
http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5338025085108224/diff/5743114304094208/src/engine/Main.cpp#newcode119 src/engine/Main.cpp:119: if (valuePtr->IsNull()) There's always the infrequently used !!! operator, ...
5 years, 4 months ago (2014-07-18 19:52:10 UTC) #8
Felix Dahlke
5 years, 3 months ago (2014-07-23 05:31:26 UTC) #9
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5