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

Issue 29524668: Issue 3916 - Supporting Adding filter lists via URL (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 5 months ago by jens
Modified:
2 years, 4 months ago
Reviewers:
diegocarloslima, anton
CC:
René Jeschke, Felix Dahlke
Visibility:
Public.

Description

Issue 3916 - Supporting Adding filter lists via URL

Patch Set 1 : Issue 3916 - Supporting Adding filer lists via URL #

Total comments: 23

Patch Set 2 : Renamed URLInputPreference and adjusted layout design #

Total comments: 24

Patch Set 3 : fixes #

Total comments: 9

Patch Set 4 : Use TAG constant for log, fix in MoreBlockingPreferenceCategory #

Unified diffs Side-by-side diffs Delta from patch set Stats (+252 lines, -66 lines) Patch
M adblockplussbrowser/res/values-de/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-el/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-es/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-fr/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-it/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-nl/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-pl/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-pt/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-ru/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values-tr/strings.xml View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M adblockplussbrowser/res/values/strings.xml View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java View 1 2 3 4 chunks +61 lines, -21 lines 0 comments Download
M adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java View 1 2 3 3 chunks +141 lines, -40 lines 0 comments Download
M adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java View 1 2 1 chunk +5 lines, -4 lines 0 comments Download
M adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java View 1 2 3 4 chunks +28 lines, -1 line 0 comments Download
M adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java View 1 2 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 16
jens
2 years, 5 months ago (2017-08-23 14:24:59 UTC) #1
anton
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java#newcode46 adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: private boolean validateTld; sorry, what is `Tld` (did i ...
2 years, 5 months ago (2017-08-25 10:40:40 UTC) #2
jens
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java#newcode46 adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: private boolean validateTld; On 2017/08/25 10:40:40, anton wrote: > ...
2 years, 5 months ago (2017-08-25 11:35:07 UTC) #3
anton
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java#newcode46 adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: private boolean validateTld; On 2017/08/25 11:35:06, jens wrote: > ...
2 years, 5 months ago (2017-08-25 11:43:31 UTC) #4
jens
On 2017/08/25 11:43:31, anton wrote: > https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java > File > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java > (right): > > ...
2 years, 5 months ago (2017-08-25 11:45:51 UTC) #5
diegocarloslima
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/res/layout/add_filter_by_url_pref.xml File adblockplussbrowser/res/layout/add_filter_by_url_pref.xml (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/res/layout/add_filter_by_url_pref.xml#newcode1 adblockplussbrowser/res/layout/add_filter_by_url_pref.xml:1: <?xml version="1.0" encoding="utf-8"?> As we already discussed through IRC, ...
2 years, 5 months ago (2017-09-08 15:06:08 UTC) #6
jens
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/res/layout/add_filter_by_url_pref.xml File adblockplussbrowser/res/layout/add_filter_by_url_pref.xml (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser/res/layout/add_filter_by_url_pref.xml#newcode1 adblockplussbrowser/res/layout/add_filter_by_url_pref.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2017/09/08 15:06:07, diegocarloslima wrote: > ...
2 years, 5 months ago (2017-09-13 08:50:30 UTC) #7
diegocarloslima
https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser/res/values/strings.xml File adblockplussbrowser/res/values/strings.xml (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser/res/values/strings.xml#newcode52 adblockplussbrowser/res/values/strings.xml:52: <string name="add_other_list">Add other filter list</string> We recently changed this ...
2 years, 4 months ago (2017-09-22 17:43:43 UTC) #8
jens
https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser/res/values/strings.xml File adblockplussbrowser/res/values/strings.xml (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser/res/values/strings.xml#newcode52 adblockplussbrowser/res/values/strings.xml:52: <string name="add_other_list">Add other filter list</string> On 2017/09/22 17:43:41, diegocarloslima ...
2 years, 4 months ago (2017-09-26 10:25:02 UTC) #9
anton
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java#newcode62 adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:62: public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new HashMap<>(); ...
2 years, 4 months ago (2017-10-10 06:28:07 UTC) #10
jens
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java#newcode62 adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:62: public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new HashMap<>(); ...
2 years, 4 months ago (2017-10-10 08:09:11 UTC) #11
anton
On 2017/08/23 14:24:59, jens wrote: LGTM (though some lines could be splitted in order to ...
2 years, 4 months ago (2017-10-10 08:14:58 UTC) #12
anton
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java#newcode62 adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:62: public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new HashMap<>(); ...
2 years, 4 months ago (2017-10-10 08:15:06 UTC) #13
diegocarloslima
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java#newcode164 adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:164: Log.i(getClass().getSimpleName(), "ValidationType was not defined and is therefore set ...
2 years, 4 months ago (2017-10-10 13:08:29 UTC) #14
jens
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java#newcode164 adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:164: Log.i(getClass().getSimpleName(), "ValidationType was not defined and is therefore set ...
2 years, 4 months ago (2017-10-10 13:56:41 UTC) #15
diegocarloslima
2 years, 4 months ago (2017-10-12 09:17:48 UTC) #16
On 2017/10/10 13:56:41, jens wrote:
>
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser...
> File
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java
> (right):
> 
>
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:164:
> Log.i(getClass().getSimpleName(), "ValidationType was not defined and is
> therefore set to" +
> On 2017/10/10 13:08:28, diegocarloslima wrote:
> > I know that the log tag is currently only used here, but it is a good
practice
> > to define it globally (private static final String TAG = ...) so if we ever
> use
> > it in multiple places, we would have a consistent TAG field that can be
> changed
> > when needed (e.g. to a hardcoded string to avoid obfuscation or to avoid the
> tag
> > limit of 23 chars)
> 
> I was not sure if it's mandatory, as there are also classes that don't use a
log
> tag. But I'll add it here.
> 
>
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser...
> File
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java
> (right):
> 
>
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:62:
> public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new
> HashMap<>();
> On 2017/10/10 13:08:28, diegocarloslima wrote:
> > On 2017/10/10 08:15:06, anton wrote:
> > > On 2017/10/10 08:09:10, jens wrote:
> > > > On 2017/10/10 06:28:06, anton wrote:
> > > > > according to "programming to interface" i'd prefer it to be `Map .. =
> new
> > > > > HashMap()`. Or are you using any HashMap-specific methods that are
> missing
> > > in
> > > > > Map? Are there any other reasons?
> > > > 
> > > > I did not introduce URL_TO_RES_ID_MAP but only changed its visibility.
But
> > you
> > > > are right, as we only use put and get, it can also be used as Map.
> Shouldn't
> > > we
> > > > do that in a separate ticket?
> > > 
> > > okay, then it's fine for now
> > 
> > URL_TO_RES_ID_MAP is now only used here, isn't it? If so, we can keep it as
> > private
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:62:
> public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new
> HashMap<>();
> On 2017/10/10 08:15:06, anton wrote:
> > On 2017/10/10 08:09:10, jens wrote:
> > > On 2017/10/10 06:28:06, anton wrote:
> > > > according to "programming to interface" i'd prefer it to be `Map .. =
new
> > > > HashMap()`. Or are you using any HashMap-specific methods that are
missing
> > in
> > > > Map? Are there any other reasons?
> > > 
> > > I did not introduce URL_TO_RES_ID_MAP but only changed its visibility. But
> you
> > > are right, as we only use put and get, it can also be used as Map.
Shouldn't
> > we
> > > do that in a separate ticket?
> > 
> > okay, then it's fine for now
> 
> As I have to make changes anyways, I'll change that too now.

LGTM
Sign in to reply to this message.

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