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

Issue 29600561: Issue 6001 - Pass IV8IsolateProvider to Helper (Closed)

Created:
Nov. 7, 2017, 7:42 a.m. by anton
Modified:
Nov. 7, 2017, 11:17 a.m.
CC:
René Jeschke, Felix Dahlke
Visibility:
Public.

Description

Issue 6001 - Pass IV8IsolateProvider to Helper

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -39 lines) Patch
M libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java View 3 chunks +6 lines, -5 lines 2 comments Download
M libadblockplus-android/jni/JniPlatform.cpp View 2 chunks +4 lines, -26 lines 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java View 2 chunks +3 lines, -3 lines 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java View 3 chunks +5 lines, -5 lines 0 comments Download

Messages

Total messages: 4
anton
Nov. 7, 2017, 7:44 a.m. (2017-11-07 07:44:31 UTC) #1
sergei
LGTM https://codereview.adblockplus.org/29600561/diff/29600562/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29600561/diff/29600562/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode138 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:138: return this; Changing of the return value is ...
Nov. 7, 2017, 9:58 a.m. (2017-11-07 09:58:46 UTC) #2
anton
https://codereview.adblockplus.org/29600561/diff/29600562/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29600561/diff/29600562/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode138 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:138: return this; On 2017/11/07 09:58:45, sergei wrote: > Changing ...
Nov. 7, 2017, 10:17 a.m. (2017-11-07 10:17:06 UTC) #3
jens
Nov. 7, 2017, 11 a.m. (2017-11-07 11:00:11 UTC) #4 
On 2017/11/07 10:17:06, anton wrote:
>
https://codereview.adblockplus.org/29600561/diff/29600562/libadblockplus-andr...
> File
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
> (right):
> 
>
https://codereview.adblockplus.org/29600561/diff/29600562/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:138:
> return this;
> On 2017/11/07 09:58:45, sergei wrote:
> > Changing of the return value is an unrelated change but it's fine for me.
> 
> Agree it's unrelated in general.
> Just want to keep it more close to `Builder` pattern as we change method
> signature anyway so now it's suitable time.

LGTM

Powered by Google App Engine
This is Rietveld