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

Issue 29572644: Noissue - Lint adjustments

Created:
Oct. 10, 2017, 12:03 p.m. by diegocarloslima
Modified:
Oct. 12, 2017, 9:14 a.m.
Reviewers:
anton, jens
CC:
Felix Dahlke, René Jeschke
Visibility:
Public.

Description

Noissue - Lint adjustments

Patch Set 1 #

Total comments: 9

Patch Set 2 : Removing shared prefs editor commit() to apply() change #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -63 lines) Patch
M mobile/android/base/resources/layout/abb_start_pane.xml View 2 chunks +2 lines, -2 lines 0 comments Download
M mobile/android/base/resources/xml/preferences_abb_abp.xml View 1 chunk +1 line, -2 lines 0 comments Download
M mobile/android/base/resources/xml/preferences_abb_abp_acceptable_ads.xml View 1 chunk +0 lines, -1 line 0 comments Download
M mobile/android/base/resources/xml/preferences_abb_adblocking.xml View 1 chunk +0 lines, -1 line 0 comments Download
M mobile/android/base/resources/xml/preferences_abb_more_blocking.xml View 1 chunk +0 lines, -1 line 0 comments Download
M mobile/android/thirdparty/org/adblockplus/browser/AdblockPlusApiCallback.java View 1 chunk +2 lines, -2 lines 0 comments Download
M mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java View 1 4 chunks +12 lines, -11 lines 0 comments Download
M mobile/android/thirdparty/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java View 6 chunks +8 lines, -27 lines 0 comments Download
M mobile/android/thirdparty/org/adblockplus/browser/StartPane.java View 5 chunks +6 lines, -6 lines 0 comments Download
M mobile/android/thirdparty/org/adblockplus/browser/SubscriptionContainer.java View 4 chunks +5 lines, -9 lines 0 comments Download
M mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 4
diegocarloslima
Oct. 10, 2017, 12:09 p.m. (2017-10-10 12:09:57 UTC) #1
anton
https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java#newcode207 mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:207: editor.apply(); this seems to be not lint change: apply ...
Oct. 10, 2017, 12:19 p.m. (2017-10-10 12:19:43 UTC) #2
diegocarloslima
https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java#newcode207 mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:207: editor.apply(); On 2017/10/10 12:19:43, anton wrote: > this seems ...
Oct. 10, 2017, 2:54 p.m. (2017-10-10 14:54:19 UTC) #3
anton
Oct. 12, 2017, 5:50 a.m. (2017-10-12 05:50:18 UTC) #4 
https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right):

https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:207:
editor.apply();
On 2017/10/10 14:54:18, diegocarloslima wrote:
> On 2017/10/10 12:19:43, anton wrote:
> > this seems to be not lint change: apply returns immediately but commit
blocks
> > the thread until it's actually written.
> > I'd suggest it to be done in separate code review as it changes app logics
> (not
> > just code style for lint).
> 
> Actually this is a lint warning, but I agree with you, since this is not just
a
> coding style or best practice and is more prone to bugs, it would make more
> sense to treat it separately

okay, waiting for the next patch set to LGTM it

https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:453: private
static class AddOnEventListener implements BundleEventListener
On 2017/10/10 14:54:18, diegocarloslima wrote:
> On 2017/10/10 12:19:42, anton wrote:
> > is it lint change too? seems to be not.
> 
> Yeah, actually this was a deprecation warning in
> 
>
EventDispatcher.getInstance().registerGeckoThreadListener(ADD_ON_EVENT_LISTENER,
> ON_FILTERS_LOAD_EVENT, ON_FILTERS_SAVE_EVENT);
> 
> which uses this deprecated method
>
https://hg.adblockplus.org/adblockbrowser/file/7018960f20ac/mobile/android/ge...

Acknowledged.

https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thir...
File
mobile/android/thirdparty/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java
(left):

https://codereview.adblockplus.org/29572644/diff/29572645/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java:115:
public static Preference createCheckBoxOrSwitch(final Context context)
On 2017/10/10 14:54:18, diegocarloslima wrote:
> On 2017/10/10 12:19:43, anton wrote:
> > this seems to be not for lint change, is it?
> 
> Yeah it was a lint warning for unused method. This one won't be ever used
again,
> since our minSdk is now 15, so we can use SwitchPreference directly when we
need
> to.

Acknowledged.

Powered by Google App Engine
This is Rietveld