Issue 527 - Only process notification requests sent by our own extensions

Issue 527 - Only process notification requests sent by our own extensions

[Felix Dahlke, 2014-05-22 07:35:21]

http://codereview.adblockplus.org/5438786527821824/diff/5629499534213120/site...
File sitescripts/stats/bin/logprocessor.py (right):

http://codereview.adblockplus.org/5438786527821824/diff/5629499534213120/site...
sitescripts/stats/bin/logprocessor.py:401: if filename == "notification.json"
and info["addonName"] not in ("adblockplus", "adblockpluschrome",
"adblockplusopera", "adblockplussafari"):
I think we should filter this for all downloads really, objections Wladimir?

Also, not a fan of hard coding the addon names. How about anything that starts
with "adblockplus"?

[Wladimir Palant, 2014-05-22 07:44:05]

http://codereview.adblockplus.org/5438786527821824/diff/5629499534213120/site...
File sitescripts/stats/bin/logprocessor.py (right):

http://codereview.adblockplus.org/5438786527821824/diff/5629499534213120/site...
sitescripts/stats/bin/logprocessor.py:401: if filename == "notification.json"
and info["addonName"] not in ("adblockplus", "adblockpluschrome",
"adblockplusopera", "adblockplussafari"):
a) We really need to do this for all downloads, otherwise we will have a
distorted picture.
b) We don't want to filter unknown extensions in general. We want to know how
many users are on outdated Adblock Plus versions (not sending addonName
parameter) and the forks. We merely shouldn't count them for the
previousDownload field.
c) fhd is right, this approach won't scale. What about adblockplusandroid or
adblockplusie? .startswith("adblockplus") feels hacky but it seems to be the
better approach here.

[Wladimir Palant, 2014-05-22 08:23:33]

LGTM

[Felix Dahlke, 2014-05-22 08:30:37]

LGTM, with the whitespace change addressed.

http://codereview.adblockplus.org/5438786527821824/diff/5639274879778816/site...
File sitescripts/stats/bin/logprocessor.py (right):

http://codereview.adblockplus.org/5438786527821824/diff/5639274879778816/site...
sitescripts/stats/bin/logprocessor.py:431: 
Unrelated whitespace change?

[Felix Dahlke, 2014-05-22 08:30:39]

LGTM, with the whitespace change addressed.

[Sebastian Noack, 2014-05-22 08:40:39]

http://codereview.adblockplus.org/5438786527821824/diff/5629499534213120/site...
File sitescripts/stats/bin/logprocessor.py (right):

http://codereview.adblockplus.org/5438786527821824/diff/5629499534213120/site...
sitescripts/stats/bin/logprocessor.py:401: if filename == "notification.json"
and info["addonName"] not in ("adblockplus", "adblockpluschrome",
"adblockplusopera", "adblockplussafari"):
On 2014/05/22 07:44:05, Wladimir Palant wrote:
> a) We really need to do this for all downloads, otherwise we will have a
> distorted picture.
> b) We don't want to filter unknown extensions in general. We want to know how
> many users are on outdated Adblock Plus versions (not sending addonName
> parameter) and the forks. We merely shouldn't count them for the
> previousDownload field.
> c) fhd is right, this approach won't scale. What about adblockplusandroid or
> adblockplusie? .startswith("adblockplus") feels hacky but it seems to be the
> better approach here.

Done.

http://codereview.adblockplus.org/5438786527821824/diff/5639274879778816/site...
File sitescripts/stats/bin/logprocessor.py (right):

http://codereview.adblockplus.org/5438786527821824/diff/5639274879778816/site...
sitescripts/stats/bin/logprocessor.py:431: 
On 2014/05/22 08:30:38, Felix H. Dahlke wrote:
> Unrelated whitespace change?

Done.

[Felix Dahlke, 2014-05-22 08:41:31]

LGTM!

[Wladimir Palant, 2014-05-22 08:42:43]

Even more LGTM.