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

Issue 6307944991817728: Issue 984 - port referrer mapping from Android (Closed)

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

Description

This port is pretty close to what we have on Android: https://hg.adblockplus.org/adblockplusandroid/file/4fd8c169eb6a/src/org/adblockplus/android/ReferrerMapping.java The main difference is that Java has the perfect data structure for our needs; LinkedHashMap, while C++'s standard library doesn't have anything comparable.

Patch Set 1 : #

Total comments: 2

Patch Set 2 : #

Total comments: 5

Patch Set 3 : #

Total comments: 2

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+191 lines, -0 lines) Patch
M include/AdblockPlus.h View 1 chunk +1 line, -0 lines 0 comments Download
A include/AdblockPlus/ReferrerMapping.h View 1 chunk +42 lines, -0 lines 0 comments Download
M libadblockplus.gyp View 2 chunks +2 lines, -0 lines 0 comments Download
A src/ReferrerMapping.cpp View 1 2 3 1 chunk +60 lines, -0 lines 0 comments Download
A test/ReferrerMapping.cpp View 1 chunk +86 lines, -0 lines 0 comments Download

Messages

Total messages: 13
Felix Dahlke
5 years, 3 months ago (2014-07-15 22:55:04 UTC) #1
René Jeschke
Just a single'n'small issue. http://codereview.adblockplus.org/6307944991817728/diff/5161210659995648/src/ReferrerMapping.cpp File src/ReferrerMapping.cpp (right): http://codereview.adblockplus.org/6307944991817728/diff/5161210659995648/src/ReferrerMapping.cpp#newcode53 src/ReferrerMapping.cpp:53: const std::string url = currentEntry->second; ...
5 years, 3 months ago (2014-07-16 08:57:06 UTC) #2
Felix Dahlke
New patch set with the issue addressed. http://codereview.adblockplus.org/6307944991817728/diff/5161210659995648/src/ReferrerMapping.cpp File src/ReferrerMapping.cpp (right): http://codereview.adblockplus.org/6307944991817728/diff/5161210659995648/src/ReferrerMapping.cpp#newcode53 src/ReferrerMapping.cpp:53: const std::string ...
5 years, 3 months ago (2014-07-16 09:05:21 UTC) #3
René Jeschke
LGTM
5 years, 3 months ago (2014-07-16 09:11:33 UTC) #4
Wladimir Palant
LGTM given that the issue below should only affect Linux once we switch to libc++. ...
5 years, 3 months ago (2014-07-16 09:51:11 UTC) #5
Felix Dahlke
On 2014/07/16 09:51:11, Wladimir Palant wrote: > LGTM given that the issue below should only ...
5 years, 3 months ago (2014-07-16 10:14:33 UTC) #6
Felix Dahlke
http://codereview.adblockplus.org/6307944991817728/diff/5750085036015616/src/ReferrerMapping.cpp File src/ReferrerMapping.cpp (right): http://codereview.adblockplus.org/6307944991817728/diff/5750085036015616/src/ReferrerMapping.cpp#newcode33 src/ReferrerMapping.cpp:33: while (cachedUrls.size() > maxCachedUrls) On 2014/07/16 09:51:11, Wladimir Palant ...
5 years, 3 months ago (2014-07-16 10:15:48 UTC) #7
Felix Dahlke
http://codereview.adblockplus.org/6307944991817728/diff/5750085036015616/src/ReferrerMapping.cpp File src/ReferrerMapping.cpp (right): http://codereview.adblockplus.org/6307944991817728/diff/5750085036015616/src/ReferrerMapping.cpp#newcode33 src/ReferrerMapping.cpp:33: while (cachedUrls.size() > maxCachedUrls) On 2014/07/16 10:15:48, Felix H. ...
5 years, 3 months ago (2014-07-16 10:18:38 UTC) #8
Wladimir Palant
LGTM http://codereview.adblockplus.org/6307944991817728/diff/5634387206995968/src/ReferrerMapping.cpp File src/ReferrerMapping.cpp (right): http://codereview.adblockplus.org/6307944991817728/diff/5634387206995968/src/ReferrerMapping.cpp#newcode33 src/ReferrerMapping.cpp:33: const int urlsToPop = cachedUrls.size() - maxCachedUrls; Nit: ...
5 years, 3 months ago (2014-07-16 10:23:24 UTC) #9
Wladimir Palant
http://codereview.adblockplus.org/6307944991817728/diff/5750085036015616/src/ReferrerMapping.cpp File src/ReferrerMapping.cpp (right): http://codereview.adblockplus.org/6307944991817728/diff/5750085036015616/src/ReferrerMapping.cpp#newcode33 src/ReferrerMapping.cpp:33: while (cachedUrls.size() > maxCachedUrls) On 2014/07/16 10:18:39, Felix H. ...
5 years, 3 months ago (2014-07-16 10:26:01 UTC) #10
Felix Dahlke
New patch set is up. So once we've switched to C++11 (and compliant implementations), we ...
5 years, 3 months ago (2014-07-16 10:34:23 UTC) #11
Wladimir Palant
LGTM, nice
5 years, 3 months ago (2014-07-16 10:39:35 UTC) #12
René Jeschke
5 years, 3 months ago (2014-07-16 10:49:51 UTC) #13
LGTM
Sign in to reply to this message.

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