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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by Wladimir Palant
Modified:
3 years, 9 months ago
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
3 years, 10 months ago (2016-06-08 20:26:17 UTC) #1
Wladimir Palant
The adblockpluscore commit this relies on landed so I've updated the dependencies file.
3 years, 9 months ago (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 ...
3 years, 9 months ago (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 ...
3 years, 9 months ago (2016-06-17 16:30:01 UTC) #4
Thomas Greiner
3 years, 9 months ago (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
Sign in to reply to this message.

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