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

Issue 29345667: Issue 4139 - Don't save element hiding filters on disk (Closed)

Created:
June 8, 2016, 8:26 p.m. by Wladimir Palant
Modified:
June 17, 2016, 10:45 p.m.
Reviewers:
Thomas Greiner
Visibility:
Public.

Description

This integrates changes from https://codereview.adblockplus.org/29345663/. Repository: hg.adblockplus.org/adblockplus

Patch Set 1 : #

Patch Set 2 : Consider enabled pref when applying stylesheet #

Patch Set 3 : Rebased and updated dependency #

Unified diffs Side-by-side diffs Delta from patch set Stats (+262 lines, -40 lines) Patch
M dependencies View 1 2 1 chunk +1 line, -1 line 0 comments Download
M lib/child/elemHide.js View 2 chunks +153 lines, -39 lines 0 comments Download
A lib/elemHideStylesheet.js View 1 1 chunk +107 lines, -0 lines 0 comments Download
M lib/main.js View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 5
Wladimir Palant
June 8, 2016, 8:26 p.m. (2016-06-08 20:26:17 UTC) #1
Wladimir Palant
The adblockpluscore commit this relies on landed so I've updated the dependencies file.
June 17, 2016, 12:11 a.m. (2016-06-17 00:11:42 UTC) #2
Thomas Greiner
Looks fine overall and I like the small improvements you made to existing code. But ...
June 17, 2016, 4:25 p.m. (2016-06-17 16:25:06 UTC) #3
Wladimir Palant
On 2016/06/17 16:25:06, Thomas Greiner wrote: > But what about preexisting elemhide.css files? Shouldn't we ...
June 17, 2016, 4:30 p.m. (2016-06-17 16:30:01 UTC) #4
Thomas Greiner
June 17, 2016, 4:43 p.m. (2016-06-17 16:43:12 UTC) #5
On 2016/06/17 16:30:01, Wladimir Palant wrote:
> On 2016/06/17 16:25:06, Thomas Greiner wrote:
> > But what about preexisting elemhide.css files? Shouldn't we delete those
when
> > updating to this version?
> 
> I thought about this and the approach that Firefox usually takes is just
keeping
> files around. It's simpler, doesn't introduce an additional source of errors
and
> having these files really doesn't cost you anything. And after a few years
most
> profiles will be reset one way or another anyway.

Ok, let's leave it then.

LGTM

Powered by Google App Engine
This is Rietveld