Issue 29505565: Issue 5237 - Correct filter lists not applied on other language installation (Closed)
|
Created:
Aug. 4, 2017, 2:27 p.m. by jens Modified:
Aug. 15, 2017, 11:43 a.m. CC:
Felix Dahlke, René Jeschke Visibility:
Public. |
DescriptionIssue 5237 - Correct filter lists not applied on other language installation
Patch Set 1 : Issue 5237 - Correct filter lists not applied on other language installation #
Total comments: 12
Patch Set 2 : Fixed copyright text and access level for getAdsSubscriptions() #
Total comments: 1
Patch Set 3 : Fixed typo in SubscriptionUtils #
Created: Aug. 15, 2017, 7:31 a.m.
(Patch set is too large to download)
MessagesTotal messages: 8
https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java:96: List<DefaultSubscriptionInfo> getAdsSubscriptions() why not public (package now)? https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:71: public static final String EASYLIST_URL = "https://easylist-downloads.adblockplus.org/easylist.txt"; what's the point of changing order? seems to be not related to the task https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:402: engine.wasFirstRun = engine.subscriptions.wasUnitialized(); not related to this task, but anyway. What is `wasUnitialized`, was it meant to be 'wasInitialized?' Please confirm it's saved correctly between app launches (in order NOT to have `wasFirstRun`= true next app launch). https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:3: * Copyright (C) 2006-2016 Eyeo GmbH it should be -2017
https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java:96: List<DefaultSubscriptionInfo> getAdsSubscriptions() On 2017/08/08 05:43:41, anton wrote: > why not public (package now)? I thought it was to keep this method package-private for now, as we currently don't use it outside of the engine package. https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:71: public static final String EASYLIST_URL = "https://easylist-downloads.adblockplus.org/easylist.txt"; On 2017/08/08 05:43:41, anton wrote: > what's the point of changing order? seems to be not related to the task I moved it to group it together with the other public constants, as it was private before. Isn't that okay for the sake of order? https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:402: engine.wasFirstRun = engine.subscriptions.wasUnitialized(); On 2017/08/08 05:43:41, anton wrote: > not related to this task, but anyway. > What is `wasUnitialized`, was it meant to be 'wasInitialized?' > > Please confirm it's saved correctly between app launches (in order NOT to have > `wasFirstRun`= true next app launch). Yeah, that's a typo. We should fix that in the next noissue commit. I double checked it again and wasFirstRun is only true when starting the app for the first time or when you wipe the app date manually (which is the intended behavior, I think). https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:3: * Copyright (C) 2006-2016 Eyeo GmbH On 2017/08/08 05:43:41, anton wrote: > it should be -2017 Acknowledged.
https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java:96: List<DefaultSubscriptionInfo> getAdsSubscriptions() On 2017/08/08 13:53:05, jens wrote: > On 2017/08/08 05:43:41, anton wrote: > > why not public (package now)? > > I thought it was to keep this method package-private for now, as we currently > don't use it outside of the engine package. That looks strange. Please consider making it 'private' or 'public' unless 'package' is common used in other classes. https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:3: * Copyright (C) 2006-2016 Eyeo GmbH On 2017/08/08 13:53:05, jens wrote: > On 2017/08/08 05:43:41, anton wrote: > > it should be -2017 > > Acknowledged. also please check `e` or `E` should used
https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java:96: List<DefaultSubscriptionInfo> getAdsSubscriptions() On 2017/08/11 11:25:07, anton wrote: > On 2017/08/08 13:53:05, jens wrote: > > On 2017/08/08 05:43:41, anton wrote: > > > why not public (package now)? > > > > I thought it was to keep this method package-private for now, as we currently > > don't use it outside of the engine package. > > That looks strange. Please consider making it 'private' or 'public' unless > 'package' is common used in other classes. Acknowledged. https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java (right): https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:3: * Copyright (C) 2006-2016 Eyeo GmbH On 2017/08/11 11:25:07, anton wrote: > On 2017/08/08 13:53:05, jens wrote: > > On 2017/08/08 05:43:41, anton wrote: > > > it should be -2017 > > > > Acknowledged. > > also please check `e` or `E` should used Acknowledged.
On 2017/08/11 13:27:23, jens wrote: > https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... > File > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java > (right): > > https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/DefaultSubscriptions.java:96: > List<DefaultSubscriptionInfo> getAdsSubscriptions() > On 2017/08/11 11:25:07, anton wrote: > > On 2017/08/08 13:53:05, jens wrote: > > > On 2017/08/08 05:43:41, anton wrote: > > > > why not public (package now)? > > > > > > I thought it was to keep this method package-private for now, as we > currently > > > don't use it outside of the engine package. > > > > That looks strange. Please consider making it 'private' or 'public' unless > > 'package' is common used in other classes. > > Acknowledged. > > https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... > File > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java > (right): > > https://codereview.adblockplus.org/29505565/diff/29505567/adblockplussbrowser... > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:3: > * Copyright (C) 2006-2016 Eyeo GmbH > On 2017/08/11 11:25:07, anton wrote: > > On 2017/08/08 13:53:05, jens wrote: > > > On 2017/08/08 05:43:41, anton wrote: > > > > it should be -2017 > > > > > > Acknowledged. > > > > also please check `e` or `E` should used > > Acknowledged. Probably it would be better to fix `E` in all files in separate issue (not in SubscriptionUtils separately). LGTM
https://codereview.adblockplus.org/29505565/diff/29512682/adblockplussbrowser... File adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java (right): https://codereview.adblockplus.org/29505565/diff/29512682/adblockplussbrowser... adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:74: * ("iw", "ji", and "in") but in subscriptions.xnl we use the new codes ("he", "yi", and "id"). Typo in subscriptions.xnl -> .xml
On 2017/08/14 17:55:01, diegocarloslima wrote: > https://codereview.adblockplus.org/29505565/diff/29512682/adblockplussbrowser... > File > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java > (right): > > https://codereview.adblockplus.org/29505565/diff/29512682/adblockplussbrowser... > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:74: > * ("iw", "ji", and "in") but in subscriptions.xnl we use the new codes ("he", > "yi", and "id"). > Typo in subscriptions.xnl -> .xml Ups, good catch.
On 2017/08/15 07:28:50, jens wrote: > On 2017/08/14 17:55:01, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29505565/diff/29512682/adblockplussbrowser... > > File > > > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java > > (right): > > > > > https://codereview.adblockplus.org/29505565/diff/29512682/adblockplussbrowser... > > > adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:74: > > * ("iw", "ji", and "in") but in subscriptions.xnl we use the new codes ("he", > > "yi", and "id"). > > Typo in subscriptions.xnl -> .xml > > Ups, good catch. LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
