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

Issue 29358502: Issue 4547 - ABP edge 0.9.8 re-adds easylist (Closed)

Created:
Oct. 21, 2016, 10:38 p.m. by Oleksandr
Modified:
Oct. 22, 2016, 4:05 p.m.
Reviewers:
Sebastian Noack, kzar
Visibility:
Public.

Description

This is slightly bizzare - I have tried to figure out why this was commented out, but I did not find any reasons. It was commented out in the original code Microsoft sent us too, so it seems to be a long living bug already.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -12 lines) Patch
M lib/adblockplus.js View 1 chunk +12 lines, -12 lines 0 comments Download

Messages

Total messages: 4
Oleksandr
Oct. 21, 2016, 10:40 p.m. (2016-10-21 22:40:40 UTC) #1
Sebastian Noack
LGTM. How did that code got commented out? I don't recall this being a change ...
Oct. 21, 2016, 10:57 p.m. (2016-10-21 22:57:56 UTC) #2
Oleksandr
On 2016/10/21 22:57:56, Sebastian Noack wrote: > LGTM. How did that code got commented out? ...
Oct. 21, 2016, 11:17 p.m. (2016-10-21 23:17:31 UTC) #3
Sebastian Noack
Oct. 22, 2016, 12:52 a.m. (2016-10-22 00:52:10 UTC) #4
On 2016/10/21 23:17:31, Oleksandr wrote:
> On 2016/10/21 22:57:56, Sebastian Noack wrote:
> > LGTM. How did that code got commented out? I don't recall this being a
change
> we
> > did in between 0.9.6 and 0.9.7.
> > 
> > (Please prepare a devbuild, I will submit it tomorrow, I'm currently on the
> > phone)
> 
> This was commented out all the way in 0.9.4. That is the result of an upgrade
to
> chrome version 1.12 that was done when you guys went to Microsoft, I think.
> Could have been just an accidental "temporary" commenting out, I guess.

I think you are right, this logic has been disabled ever since (even before
0.9.4) since initially EasyList was downloaded on every startup (by separate
code) and other filter lists weren't supported. I'm still surprised that it
didn't caused a bug that was noticed before the last release.

Powered by Google App Engine
This is Rietveld