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

Issue 29401555: Issue 5088 - Remove code duplicate for refresh subscriptions

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 3 days ago by anton
Modified:
2 weeks ago
CC:
René Jeschke
Visibility:
Public.

Description

Issue 5088 - Remove code duplicate for refresh subscriptions

Patch Set 1 #

Total comments: 4

Patch Set 2 : using "update" instead of "refresh" terminology #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -15 lines) Patch
M libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java View 1 3 chunks +2 lines, -15 lines 0 comments Download

Messages

Total messages: 6
anton
https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java#oldcode46 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:46: import android.content.pm.PackageManager.NameNotFoundException; not related by the task, but desired ...
3 weeks, 3 days ago (2017-04-03 06:17:54 UTC) #1
diegocarloslima
https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java#oldcode254 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:254: private static final int UPDATE_DELAY_MS = 1 * 1000; ...
2 weeks, 6 days ago (2017-04-07 09:36:54 UTC) #2
anton
On 2017/04/07 09:36:54, diegocarloslima wrote: > https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (left): > > ...
2 weeks, 6 days ago (2017-04-07 09:37:57 UTC) #3
diegocarloslima
On 2017/04/07 09:37:57, anton wrote: > On 2017/04/07 09:36:54, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java ...
2 weeks, 6 days ago (2017-04-07 12:47:25 UTC) #4
Felix Dahlke
Looks good, but I guess it'll need some changing after my comment. https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java ...
2 weeks, 3 days ago (2017-04-10 11:59:47 UTC) #5
anton
2 weeks ago (2017-04-13 09:37:21 UTC) #6
On 2017/04/10 11:59:47, Felix Dahlke wrote:
> Looks good, but I guess it'll need some changing after my comment.
> 
>
https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr...
> File
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
> (left):
> 
>
https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:254:
> private static final int UPDATE_DELAY_MS = 1 * 1000;
> On 2017/04/07 09:36:54, diegocarloslima wrote:
> > On 2017/04/03 06:17:54, anton wrote:
> > > I'm not sure that 'refresh' is better than 'update' for subscriptions
> updating
> > > but we had 'refreshSubscriptions()' method in original AdblockEngine
class.
> > Also
> > > if someone already uses libadblockplus-android API it's preferred not to
> > rename
> > > existing methods. 
> > > 
> > > So i use ''refresh' everywhere below in terms of 'update' subscriptions.
> > 
> > Well, I think that update is a better term in this case than refresh. For
> > someone already used to the API, maybe refresh makes more sense, but for
> someone
> > new to it, update would be easier to understand. I wouldn't insist, but I
> would
> > prefer update to be used.
> 
> I'd also go for "update", since that's the terminology we use in
Libadblockplus
> as well (there is no function for updating all subscriptions, but there is
> Subscription::UpdateFilters).
> 
> Since libadblockplus-android is relatively new (the only user we're aware of
is
> ZTE), we can probably still safely break the API.

Uploaded new patch set
Sign in to reply to this message.

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