Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29331669: Issue 3237 - Whitelisted AFS ad is not shown on netzwelt.de (Closed)

Created:
Dec. 1, 2015, 2:15 a.m. by Oleksandr
Modified:
Dec. 16, 2015, 1:10 p.m.
Reviewers:
sergei, Eric
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue 3237 - Whitelisted AFS ad is not shown on netzwelt.de

Patch Set 1 #

Total comments: 9

Patch Set 2 : Move subdocument detection to the top. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -13 lines) Patch
M src/plugin/AdblockPlusDomTraverser.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/plugin/PluginDomTraverserBase.h View 1 6 chunks +7 lines, -7 lines 0 comments Download
M src/plugin/PluginWbPassThrough.cpp View 1 2 chunks +5 lines, -4 lines 2 comments Download

Messages

Total messages: 6
Oleksandr
This fixes the issue in 2 steps. 1. The whitelisted ad was blocked in the ...
Dec. 1, 2015, 2:23 a.m. (2015-12-01 02:23:21 UTC) #1
sergei
On 2015/12/01 02:23:21, Oleksandr wrote: > This fixes the issue in 2 steps. > 1. ...
Dec. 1, 2015, 10:53 a.m. (2015-12-01 10:53:59 UTC) #2
Eric
I'm good with fix 2 in the present change set; Sergei also had no comment ...
Dec. 1, 2015, 8:32 p.m. (2015-12-01 20:32:57 UTC) #3
Oleksandr
Patchset 2 is a rebase and addressing comments. https://codereview.adblockplus.org/29331669/diff/29331670/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): https://codereview.adblockplus.org/29331669/diff/29331670/src/plugin/PluginWbPassThrough.cpp#newcode145 src/plugin/PluginWbPassThrough.cpp:145: ContentType ...
Dec. 15, 2015, 2:10 a.m. (2015-12-15 02:10:16 UTC) #4
Eric
LGTM. I wouldn't mind a new patch set with the suggestions in my comment. But ...
Dec. 15, 2015, 3:18 a.m. (2015-12-15 03:18:18 UTC) #5
sergei
Dec. 15, 2015, 3:29 p.m. (2015-12-15 15:29:32 UTC) #6
LGTM

https://codereview.adblockplus.org/29331669/diff/29332669/src/plugin/PluginWb...
File src/plugin/PluginWbPassThrough.cpp (right):

https://codereview.adblockplus.org/29331669/diff/29332669/src/plugin/PluginWb...
src/plugin/PluginWbPassThrough.cpp:145: ContentType
WBPassthruSink::GetContentTypeFromMimeType(const std::wstring& mimeType)
On 2015/12/15 03:18:18, Eric wrote:
> While not strictly necessary, there are a few things that would improve the
> code, as long as we're touching it here. They're listed in the order I care.
> - Add a comment here listing the media type strings found in testing. That's
> some useful information that doesn't deserve to get buried in a comment in a
> review.
> - "MimeType" should be renamed "MediaTypeList". The present name is somewhat
> confusing unless you track down where it's called from.
> - I'd use "InferContentType" rather than "Get...". This is a heuristic, not
just
> a parse-and-fetch operation like its name seems to indicate.
> - This function could be in an anonymous namespace. It doesn't need to be a
> static class member.
> 
> I'm not going to hold the review up if these aren't present, but I would like
to
> see them, particularly the first, since it directly related to the defect
being
> fixed in the present change set.

The same from my side.

Powered by Google App Engine
This is Rietveld