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

Issue 29757571: Issue 6591 - Bundle ABP for Samsung Internet China with Easylist for China (Closed)

Created:
April 20, 2018, 1:27 p.m. by diegocarloslima
Modified:
May 9, 2018, 11:49 a.m.
Reviewers:
anton, jens
CC:
René Jeschke
Visibility:
Public.

Description

Issue 6591 - Bundle ABP for Samsung Internet China with Easylist for China

Patch Set 1 #

Total comments: 10

Patch Set 2 : Adjustments regarding Anton's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -10 lines) Patch
M adblockplussbrowser/build.gradle View 4 chunks +8 lines, -4 lines 0 comments Download
M adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java View 1 chunk +0 lines, -1 line 0 comments Download
M adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java View 1 4 chunks +9 lines, -1 line 0 comments Download
M adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt View 3 chunks +3 lines, -3 lines 0 comments Download
M build.gradle View 1 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 6
diegocarloslima
https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/build.gradle File adblockplussbrowser/build.gradle (right): https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/build.gradle#newcode21 adblockplussbrowser/build.gradle:21: buildConfigField "String", "FLAVOR_REGION_CHINA", '"china"' Since the current flavor is ...
April 20, 2018, 1:33 p.m. (2018-04-20 13:33:02 UTC) #1
jens
On 2018/04/20 13:33:02, diegocarloslima wrote: > https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/build.gradle > File adblockplussbrowser/build.gradle (right): > > https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/build.gradle#newcode21 > ...
April 20, 2018, 1:54 p.m. (2018-04-20 13:54:08 UTC) #2
anton
https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java (right): https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java#newcode33 adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:33: private static final String EASYLIST_CHINA_URL ="https://easylist-downloads.adblockplus.org/easylistchina+easylist.txt"; why can't it ...
April 20, 2018, 2:03 p.m. (2018-04-20 14:03:20 UTC) #3
diegocarloslima
https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java (right): https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java#newcode33 adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:33: private static final String EASYLIST_CHINA_URL ="https://easylist-downloads.adblockplus.org/easylistchina+easylist.txt"; On 2018/04/20 14:03:20, ...
April 20, 2018, 2:48 p.m. (2018-04-20 14:48:53 UTC) #4
anton
https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java (right): https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java#newcode33 adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:33: private static final String EASYLIST_CHINA_URL ="https://easylist-downloads.adblockplus.org/easylistchina+easylist.txt"; On 2018/04/20 14:48:52, ...
April 20, 2018, 2:50 p.m. (2018-04-20 14:50:28 UTC) #5
anton
April 20, 2018, 5:24 p.m. (2018-04-20 17:24:20 UTC) #6 
On 2018/04/20 14:50:28, anton wrote:
>
https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser...
> File
>
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java
> (right):
> 
>
https://codereview.adblockplus.org/29757571/diff/29757572/adblockplussbrowser...
>
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SubscriptionUtils.java:33:
> private static final String EASYLIST_CHINA_URL
> ="https://easylist-downloads.adblockplus.org/easylistchina+easylist.txt";
> On 2018/04/20 14:48:52, diegocarloslima wrote:
> > On 2018/04/20 14:03:20, anton wrote:
> > > why can't it be public?
> > 
> > It actually can, but since it was't being used anywhere else I left as
> private,
> > but if you think is valid to keep it public, I'm fine with that
> 
> ok, i'm not strong about it. Just wanted to make sure it was intentional and
> consistent with other code.

patch set #2 LGTM

Powered by Google App Engine
This is Rietveld