Issue 29524668: Issue 3916 - Supporting Adding filter lists via URL (Closed)
|
Created:
Aug. 23, 2017, 12:56 p.m. by jens Modified:
Oct. 12, 2017, 2:41 p.m. CC:
René Jeschke, Felix Dahlke Visibility:
Public. |
DescriptionIssue 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 #MessagesTotal messages: 16
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: private boolean validateTld; sorry, what is `Tld` (did i miss anything)? Can you change it to anything more readable? https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:148: private boolean validate() improvement suggestion: `validate` makes me think it's just an action (without result) and `isValid()` sounds better for me. Also you already have 'isValidUrl' and 'isValidDomain' methods. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:633: for(SubscriptionInfo sub : getListedSubscriptions()) it seems that space is required after `for` https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:225: if (this.subscriptions.containsKey(id)) the whole method could be replaced with `return this.subscriptions.remove(id) != null;`, is it? BTW is return value needed somewhere?
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: private boolean validateTld; On 2017/08/25 10:40:40, anton wrote: > sorry, what is `Tld` (did i miss anything)? Can you change it to anything more > readable? It stands for top-level domain. I thought it is a common abbreviation. The reason I used this name is, that we introduced a domain validation to ensure that only existing top-level domains can be entered when whitelisting a website. I will change the name to 'validateTopLevelDomain' https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:148: private boolean validate() On 2017/08/25 10:40:40, anton wrote: > improvement suggestion: `validate` makes me think it's just an action (without > result) > and `isValid()` sounds better for me. Also you already have 'isValidUrl' and > 'isValidDomain' methods. Acknowledged. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:633: for(SubscriptionInfo sub : getListedSubscriptions()) On 2017/08/25 10:40:40, anton wrote: > it seems that space is required after `for` Acknowledged. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:225: if (this.subscriptions.containsKey(id)) On 2017/08/25 10:40:40, anton wrote: > the whole method could be replaced with `return this.subscriptions.remove(id) != > null;`, is it? > BTW is return value needed somewhere? Acknowledged. The return values is not needed yet but I thought it makes sense to confirm if the subscription was removed or not.
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: private boolean validateTld; On 2017/08/25 11:35:06, jens wrote: > On 2017/08/25 10:40:40, anton wrote: > > sorry, what is `Tld` (did i miss anything)? Can you change it to anything more > > readable? > > It stands for top-level domain. I thought it is a common abbreviation. The > reason I used this name is, that we introduced a domain validation to ensure > that only existing top-level domains can be entered when whitelisting a > website. I will change the name to 'validateTopLevelDomain' Okay, why not just call it 'validateDomain' for simplicity and add a comment that it validates only TLD? We can ask Diego what he thinks about it. "There are only two hard things in Computer Science: cache invalidation and naming things" ;)
On 2017/08/25 11:43:31, anton wrote: > https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... > File > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java > (right): > > https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: > private boolean validateTld; > On 2017/08/25 11:35:06, jens wrote: > > On 2017/08/25 10:40:40, anton wrote: > > > sorry, what is `Tld` (did i miss anything)? Can you change it to anything > more > > > readable? > > > > It stands for top-level domain. I thought it is a common abbreviation. The > > reason I used this name is, that we introduced a domain validation to ensure > > that only existing top-level domains can be entered when whitelisting a > > website. I will change the name to 'validateTopLevelDomain' > > Okay, why not just call it 'validateDomain' for simplicity and add a comment > that it validates only TLD? > We can ask Diego what he thinks about it. > > "There are only two hard things in Computer Science: cache invalidation and > naming things" ;) Fair enough :)
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/res/layout/add_filter_by_url_pref.xml (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/res/layout/add_filter_by_url_pref.xml:1: <?xml version="1.0" encoding="utf-8"?> As we already discussed through IRC, the add filter list look and feel, should be consistent with the whitelisting functionality (I added new screenshots to the ticket in Trac). So it seems that there's no need for this file. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/res/values/strings.xml (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/res/values/strings.xml:53: <string name="add_other_list_url">https://www.example.com/filterlist.txt</string> I would suggest to rename it to `add_other_list_url_hint`, just to be consistent with `whitelist_website_dialog_hint` https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:60: public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new HashMap<>(); Since this field is now used by Engine, we should move it there. All the general purpose fields currently should be declared there. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (left): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:39: public class UrlInputOpenerPreference extends EditTextPreference implements TextWatcher, I would suggest to rename the class to something like InputOpenerPreference or InputValidatorDialogPreference, since now it became a more general purpose class that can validate URLs and domain (and can validate other stuff in the future). https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:153: public interface OnUrlReadyListener I would also suggest to rename the listener to OnInputReadyListener and the method to onInputReady(String input) https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: private boolean validateTld; On 2017/08/25 11:43:30, anton wrote: > On 2017/08/25 11:35:06, jens wrote: > > On 2017/08/25 10:40:40, anton wrote: > > > sorry, what is `Tld` (did i miss anything)? Can you change it to anything > more > > > readable? > > > > It stands for top-level domain. I thought it is a common abbreviation. The > > reason I used this name is, that we introduced a domain validation to ensure > > that only existing top-level domains can be entered when whitelisting a > > website. I will change the name to 'validateTopLevelDomain' > > Okay, why not just call it 'validateDomain' for simplicity and add a comment > that it validates only TLD? > We can ask Diego what he thinks about it. > > "There are only two hard things in Computer Science: cache invalidation and > naming things" ;) I would suggest to create an enum for that, so we can extend this preference to validate other types of inputs in the future. I would call the enum ValidationType {DOMAIN, URL} https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:148: private boolean validate() On 2017/08/25 11:35:06, jens wrote: > On 2017/08/25 10:40:40, anton wrote: > > improvement suggestion: `validate` makes me think it's just an action (without > > result) > > and `isValid()` sounds better for me. Also you already have 'isValidUrl' and > > 'isValidDomain' methods. > > Acknowledged. I would further suggest to rename it to isValidInput()
https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/res/layout/add_filter_by_url_pref.xml (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... 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: > As we already discussed through IRC, the add filter list look and feel, should > be consistent with the whitelisting functionality (I added new screenshots to > the ticket in Trac). So it seems that there's no need for this file. Acknowledged. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/res/values/strings.xml (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/res/values/strings.xml:53: <string name="add_other_list_url">https://www.example.com/filterlist.txt</string> On 2017/09/08 15:06:07, diegocarloslima wrote: > I would suggest to rename it to `add_other_list_url_hint`, just to be consistent > with `whitelist_website_dialog_hint` Acknowledged. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:60: public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new HashMap<>(); On 2017/09/08 15:06:07, diegocarloslima wrote: > Since this field is now used by Engine, we should move it there. All the general > purpose fields currently should be declared there. Acknowledged. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (left): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:39: public class UrlInputOpenerPreference extends EditTextPreference implements TextWatcher, On 2017/09/08 15:06:07, diegocarloslima wrote: > I would suggest to rename the class to something like InputOpenerPreference or > InputValidatorDialogPreference, since now it became a more general purpose class > that can validate URLs and domain (and can validate other stuff in the future). Acknowledged. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:153: public interface OnUrlReadyListener On 2017/09/08 15:06:07, diegocarloslima wrote: > I would also suggest to rename the listener to OnInputReadyListener and the > method to onInputReady(String input) Acknowledged. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:46: private boolean validateTld; On 2017/09/08 15:06:07, diegocarloslima wrote: > On 2017/08/25 11:43:30, anton wrote: > > On 2017/08/25 11:35:06, jens wrote: > > > On 2017/08/25 10:40:40, anton wrote: > > > > sorry, what is `Tld` (did i miss anything)? Can you change it to anything > > more > > > > readable? > > > > > > It stands for top-level domain. I thought it is a common abbreviation. The > > > reason I used this name is, that we introduced a domain validation to ensure > > > that only existing top-level domains can be entered when whitelisting a > > > website. I will change the name to 'validateTopLevelDomain' > > > > Okay, why not just call it 'validateDomain' for simplicity and add a comment > > that it validates only TLD? > > We can ask Diego what he thinks about it. > > > > "There are only two hard things in Computer Science: cache invalidation and > > naming things" ;) > > I would suggest to create an enum for that, so we can extend this preference to > validate other types of inputs in the future. > I would call the enum ValidationType {DOMAIN, URL} Acknowledged. https://codereview.adblockplus.org/29524668/diff/29524885/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java:148: private boolean validate() On 2017/09/08 15:06:07, diegocarloslima wrote: > On 2017/08/25 11:35:06, jens wrote: > > On 2017/08/25 10:40:40, anton wrote: > > > improvement suggestion: `validate` makes me think it's just an action > (without > > > result) > > > and `isValid()` sounds better for me. Also you already have 'isValidUrl' and > > > 'isValidDomain' methods. > > > > Acknowledged. > > I would further suggest to rename it to isValidInput() Acknowledged.
https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/res/values/strings.xml (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/res/values/strings.xml:52: <string name="add_other_list">Add other filter list</string> We recently changed this string to 'Add another filter list' in ABB, so you should change here too. For the translations, you can keep as it is for now https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java (left): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:116: Toast.makeText(getContext(), R.string.whitelist_website_invalid_url_error, Toast.LENGTH_SHORT).show(); We might consider making possible to change the text of the error message externally, by defining a setErrorMessage() method https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:140: private String getUrl() This might be renamed to getInput() https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:51: private AlertDialog mAlertDialog; This variable passed through the last code review, but it should be renamed to just 'alertDialog'. Since we're refactoring this class in this issue, I'm fine with either changing it here or in a follow up issue. What do you think? https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:60: final ValidationType validationType) This type of constructor will fail if the class is placed into a xml file, because of the ValidationType argument https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:181: void onInputReady(String url); I would suggest changing from `String url` to `String input` https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:211: initialize(); Maybe we should change the name of this method to something like `refreshEntries()`, since initialize() gives the idea that the method will be only called during the class initialization and not multiple times (e.g. when a subscription is added) https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:106: public void onInputReady(String url) I would suggest changing from `String url` to `String input` https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:77: public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new HashMap<>(); I know that I asked you in the last code review to move this Hash here, but giving a second look at the code review, I think that it would be better if we move this back to MoreBlockingPreferenceCategory, along with the getMoreBlockingPreferencesSubscriptions() method. I feel that the Engine class is getting way too big and that logic is currently only relevant for the MoreBlockingPreferenceCategory. What do you think? https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:631: public List<SubscriptionInfo> getMoreBlockingPreferenceSubscriptions() Please read my comment above regarding moving this to MoreBlockingPreferenceCategory https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:660: private boolean isAcceptableAdsUrl(final SubscriptionInfo subscriptionInfo) If we move the method above, we should make this public https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:207: w.write('\n'); We shouldn't revert this temporary fix in this ticket. Also, we need first to align with Samsung if almost no users are still using an old version of the browser that doesn't contain the edge case crash fix.
https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/res/values/strings.xml (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/res/values/strings.xml:52: <string name="add_other_list">Add other filter list</string> On 2017/09/22 17:43:41, diegocarloslima wrote: > We recently changed this string to 'Add another filter list' in ABB, so you > should change here too. For the translations, you can keep as it is for now Acknowledged. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java (left): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:116: Toast.makeText(getContext(), R.string.whitelist_website_invalid_url_error, Toast.LENGTH_SHORT).show(); On 2017/09/22 17:43:42, diegocarloslima wrote: > We might consider making possible to change the text of the error message > externally, by defining a setErrorMessage() method o have different error messages for different ValidationTypes? That's a good idea but maybe we should do it in a separate ticket as we will need new strings ans translations too. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:140: private String getUrl() On 2017/09/22 17:43:42, diegocarloslima wrote: > This might be renamed to getInput() Acknowledged. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:51: private AlertDialog mAlertDialog; On 2017/09/22 17:43:42, diegocarloslima wrote: > This variable passed through the last code review, but it should be renamed to > just 'alertDialog'. Since we're refactoring this class in this issue, I'm fine > with either changing it here or in a follow up issue. What do you think? For me it's totally fine to change the name of the variable within this ticket. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:60: final ValidationType validationType) On 2017/09/22 17:43:42, diegocarloslima wrote: > This type of constructor will fail if the class is placed into a xml file, > because of the ValidationType argument Oh, good catch. Should we create a separate ticket to make it possible to set ValidationType from XML? For now I would suggest to move setting the defaults to the three arguments default constructor and remove validationType from this one. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/InputValidatorDialogPreference.java:181: void onInputReady(String url); On 2017/09/22 17:43:41, diegocarloslima wrote: > I would suggest changing from `String url` to `String input` Acknowledged. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MoreBlockingPreferenceCategory.java:211: initialize(); On 2017/09/22 17:43:42, diegocarloslima wrote: > Maybe we should change the name of this method to something like > `refreshEntries()`, since initialize() gives the idea that the method will be > only called during the class initialization and not multiple times (e.g. when a > subscription is added) Acknowledged. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/WhitelistedWebsitesPreferenceCategory.java:106: public void onInputReady(String url) On 2017/09/22 17:43:42, diegocarloslima wrote: > I would suggest changing from `String url` to `String input` Acknowledged. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:77: public static final HashMap<String, Integer> URL_TO_RES_ID_MAP = new HashMap<>(); On 2017/09/22 17:43:42, diegocarloslima wrote: > I know that I asked you in the last code review to move this Hash here, but > giving a second look at the code review, I think that it would be better if we > move this back to MoreBlockingPreferenceCategory, along with the > getMoreBlockingPreferencesSubscriptions() method. I feel that the Engine class > is getting way too big and that logic is currently only relevant for the > MoreBlockingPreferenceCategory. What do you think? Acknowledged. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:631: public List<SubscriptionInfo> getMoreBlockingPreferenceSubscriptions() On 2017/09/22 17:43:43, diegocarloslima wrote: > Please read my comment above regarding moving this to > MoreBlockingPreferenceCategory Acknowledged. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:660: private boolean isAcceptableAdsUrl(final SubscriptionInfo subscriptionInfo) On 2017/09/22 17:43:43, diegocarloslima wrote: > If we move the method above, we should make this public Acknowledged. https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java (right): https://codereview.adblockplus.org/29524668/diff/29545624/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Subscriptions.java:207: w.write('\n'); On 2017/09/22 17:43:43, diegocarloslima wrote: > We shouldn't revert this temporary fix in this ticket. Also, we need first to > align with Samsung if almost no users are still using an old version of the > browser that doesn't contain the edge case crash fix. Sorry, I don't know how these two lines slipped into the patch set. That was definitely not on purpose!
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<>(); 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?
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 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?
On 2017/08/23 14:24:59, jens wrote: LGTM (though some lines could be splitted in order to fit into 100 symbols)
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 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 https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29524668/diff/29564682/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:259: public void createAndAddSubscriptionFromUrl(final String url, final SubscriptionAddedCallback callback) throws IOException the line seems to be too long (>100)
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" + 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) 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 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
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.
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 
|
