Issue 29350065: Issue 2853 - Settings changes are sometimes not saved if the user quits the app (Closed)
|
Created:
Aug. 23, 2016, 12:39 p.m. by diegocarloslima Modified:
Feb. 9, 2017, 9:13 p.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 2853 - Settings changes are sometimes not saved if the user quits the app
Patch Set 1 #
Total comments: 10
Patch Set 2 : Adjusting spacing and also adding code change comment #
Total comments: 8
Patch Set 3 : Instead of pooling, adding observer for save operations performed by the extension #
Total comments: 6
Patch Set 4 : Fixing a bug where uncompleted requests could be sent out of order with the use of filter load pool… #Patch Set 5 : Removing now unused getFiltersLoaded #
Total comments: 2
Patch Set 6 : Renaming 'uncompleted' to 'pending' #
MessagesTotal messages: 13
https://codereview.adblockplus.org/29350065/diff/29350066/adblockplus/Api.jsm File adblockplus/Api.jsm (right): https://codereview.adblockplus.org/29350065/diff/29350066/adblockplus/Api.jsm... adblockplus/Api.jsm:40: let {FilterNotifier} = require("filterNotifier"); Please make sure it does not interfere with https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm: creating and initializing FilterNotifier, etc. https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:118: if(PENDING_SYNC_REQUESTS.isEmpty()) space here? https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:177: if(requestList == null) space here? https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:192: if(jsonString == null) space here? https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:205: catch (JSONException e) Was ignoring of thrown exception done intentionally? Consider throwing `RuntimeException` instead or declaring `throws` and catching exception in invoker (not sure why it's not done in other methods in this class). I can see in that file exceptions are also logged with `Log.e()` at least
https://codereview.adblockplus.org/29350065/diff/29350066/adblockplus/Api.jsm File adblockplus/Api.jsm (right): https://codereview.adblockplus.org/29350065/diff/29350066/adblockplus/Api.jsm... adblockplus/Api.jsm:40: let {FilterNotifier} = require("filterNotifier"); On 2016/09/30 06:51:03, anton wrote: > Please make sure it does not interfere with > https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm: > creating and initializing FilterNotifier, etc. Yes, they don't conflict to each other, altough they share some of the same basic/helper functions https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:118: if(PENDING_SYNC_REQUESTS.isEmpty()) On 2016/09/30 06:51:03, anton wrote: > space here? Acknowledged. https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:177: if(requestList == null) On 2016/09/30 06:51:03, anton wrote: > space here? Acknowledged. https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:192: if(jsonString == null) On 2016/09/30 06:51:03, anton wrote: > space here? Acknowledged. https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:205: catch (JSONException e) On 2016/09/30 06:51:03, anton wrote: > Was ignoring of thrown exception done intentionally? Consider throwing > `RuntimeException` instead or declaring `throws` and catching exception in > invoker (not sure why it's not done in other methods in this class). I can see > in that file exceptions are also logged with `Log.e()` at least Yes it was done intentionally, to handle gracefully if for some reason the pending request list is malformed.
On 2016/10/26 13:48:04, diegocarloslima wrote: > https://codereview.adblockplus.org/29350065/diff/29350066/adblockplus/Api.jsm > File adblockplus/Api.jsm (right): > > https://codereview.adblockplus.org/29350065/diff/29350066/adblockplus/Api.jsm... > adblockplus/Api.jsm:40: let {FilterNotifier} = require("filterNotifier"); > On 2016/09/30 06:51:03, anton wrote: > > Please make sure it does not interfere with > > https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm: > > creating and initializing FilterNotifier, etc. > > Yes, they don't conflict to each other, altough they share some of the same > basic/helper functions > > https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... > File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): > > https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... > mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:118: > if(PENDING_SYNC_REQUESTS.isEmpty()) > On 2016/09/30 06:51:03, anton wrote: > > space here? > > Acknowledged. > > https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... > mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:177: > if(requestList == null) > On 2016/09/30 06:51:03, anton wrote: > > space here? > > Acknowledged. > > https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... > mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:192: > if(jsonString == null) > On 2016/09/30 06:51:03, anton wrote: > > space here? > > Acknowledged. > > https://codereview.adblockplus.org/29350065/diff/29350066/mobile/android/thir... > mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:205: catch > (JSONException e) > On 2016/09/30 06:51:03, anton wrote: > > Was ignoring of thrown exception done intentionally? Consider throwing > > `RuntimeException` instead or declaring `throws` and catching exception in > > invoker (not sure why it's not done in other methods in this class). I can see > > in that file exceptions are also logged with `Log.e()` at least > > Yes it was done intentionally, to handle gracefully if for some reason the > pending request list is malformed. LGTM
Bit puzzled about this one, but I might be missing something. Please look at my first comment in AddOnBridge.java first. https://codereview.adblockplus.org/29350065/diff/29361546/adblockplus/Api.jsm File adblockplus/Api.jsm (right): https://codereview.adblockplus.org/29350065/diff/29361546/adblockplus/Api.jsm... adblockplus/Api.jsm:48: let shouldSaveFiltersPref = "should_save_filters"; I think this name is so generic that it could conflict with other prefs we set. How about `abb_should_save_filters`? I would probably prefer a more technical term even, maybe `abb_filters_dirty` or `abb_filters_need_saving`? https://codereview.adblockplus.org/29350065/diff/29361546/adblockplus/Api.jsm... adblockplus/Api.jsm:50: function initListeners() Nit: Maybe call this `initFilterListeners` or something along those lines, since there are also some other listeners being created here, but not in this function? https://codereview.adblockplus.org/29350065/diff/29361546/adblockplus/Api.jsm... adblockplus/Api.jsm:64: function onSave() Nit: Shouldn't this be called `onFiltersSave` for consistency? Just a thought, wouldn't insist. https://codereview.adblockplus.org/29350065/diff/29361546/adblockplus/Api.jsm... adblockplus/Api.jsm:69: function getBoolPref(name) It's a bit of a shame that we can apparently not use our own Prefs object for this. Duplicating that logic is not great. I guess we can live with that for now, but could you create a follow-up issue for resolving this, and link to it here? My first hunch would be to modify `lib/prefs.json` in the Firefox extension during the build (i.e. in `adblockplus/build.py` in the ABB repository). https://codereview.adblockplus.org/29350065/diff/29361546/adblockplus/Api.jsm... adblockplus/Api.jsm:116: get requestsSaved() Naming: Do we actually "save requests" here? Seems we rather "process requests" or "save filters". So maybe we could call this `filtersSaved`? `requestsProcessed` would be another option, but I feel this could be confusing, since we don't actually check whether there are requests being process right now, we just check whether the filters have been saved. https://codereview.adblockplus.org/29350065/diff/29361546/mobile/android/thir... File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29350065/diff/29361546/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:60: private static final List<ChainedRequest> PENDING_SYNC_REQUESTS = new ArrayList<>(); I'm a bit confused as to what the point of all this is. How I see it, we never actually do anything with PENDING_SYNC_REQUESTS, is that correct? We just keep sending a "requestsSaved" message to the extension effectively. Does that alone solve issue 2853? Because if so, we should be able to simplify this code, how I see it. Also, I'm not sure I understand the naming. What is a "sync request"? Is that a "requestsSaved" message which did not receive a response yet? Maybe we can find a name that describes what it does, i.e. wait for the extension to finish saving filters? https://codereview.adblockplus.org/29350065/diff/29361546/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:78: public static void init(Context context) It makes me a bit nervous to rely on outside code initialising this once, and only once. I think it would be more robust to make this method deal with multiple invocations. How I see it, we could just check if sharedPrefs is set, and if not, proceed. While at it, I would suggest to rename this from `init` to `startSyncRequests` to make it more clear what it does - and combine it with the method that currently has that name. https://codereview.adblockplus.org/29350065/diff/29361546/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:94: public static void startSyncRequests() Does this have to be public? But see my suggestion above, if you implement that, that would make this comment obsolete.
LGTM pretty much, some nits for your consideration. https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm File adblockplus/Api.jsm (right): https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm... adblockplus/Api.jsm:176: initListeners(); Nit: Considering how there's only a single listener and only a single line needed to register it, how about inlining this? https://codereview.adblockplus.org/29350065/diff/29369470/mobile/android/thir... File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29350065/diff/29369470/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:280: final Map<String, Object> parameters, boolean shouldAddUncompletedRequest) Nit: `shouldAddUncompletedRequest` sounds a bit like a leaky abstraction. How about something like `repeatIfAborted`? Not a great name either, but something in that vein maybe...
https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm File adblockplus/Api.jsm (right): https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm... adblockplus/Api.jsm:176: initListeners(); On 2016/12/21 20:23:25, Felix Dahlke wrote: > Nit: Considering how there's only a single listener and only a single line > needed to register it, how about inlining this? Actually there is another listener that is already registered by Issue #3247. So this is required to keep consistency https://codereview.adblockplus.org/29350065/diff/29369470/mobile/android/thir... File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29350065/diff/29369470/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:280: final Map<String, Object> parameters, boolean shouldAddUncompletedRequest) On 2016/12/21 20:23:25, Felix Dahlke wrote: > Nit: `shouldAddUncompletedRequest` sounds a bit like a leaky abstraction. How > about something like `repeatIfAborted`? Not a great name either, but something > in that vein maybe... Acknowledged.
https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm File adblockplus/Api.jsm (right): https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm... adblockplus/Api.jsm:176: initListeners(); On 2016/12/21 20:29:31, diegocarloslima wrote: > On 2016/12/21 20:23:25, Felix Dahlke wrote: > > Nit: Considering how there's only a single listener and only a single line > > needed to register it, how about inlining this? > > Actually there is another listener that is already registered by Issue #3247. So > this is required to keep consistency Oh, so it seems you need to rebase this patch? Think I suggested to call that function `initFilterListeners` then for clarity's sake.
https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm File adblockplus/Api.jsm (right): https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm... adblockplus/Api.jsm:176: initListeners(); On 2016/12/21 20:36:20, Felix Dahlke wrote: > On 2016/12/21 20:29:31, diegocarloslima wrote: > > On 2016/12/21 20:23:25, Felix Dahlke wrote: > > > Nit: Considering how there's only a single listener and only a single line > > > needed to register it, how about inlining this? > > > > Actually there is another listener that is already registered by Issue #3247. > So > > this is required to keep consistency > > Oh, so it seems you need to rebase this patch? Think I suggested to call that > function `initFilterListeners` then for clarity's sake. Acknowledged.
On 2016/12/21 20:42:03, diegocarloslima wrote: > https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm > File adblockplus/Api.jsm (right): > > https://codereview.adblockplus.org/29350065/diff/29369470/adblockplus/Api.jsm... > adblockplus/Api.jsm:176: initListeners(); > On 2016/12/21 20:36:20, Felix Dahlke wrote: > > On 2016/12/21 20:29:31, diegocarloslima wrote: > > > On 2016/12/21 20:23:25, Felix Dahlke wrote: > > > > Nit: Considering how there's only a single listener and only a single line > > > > needed to register it, how about inlining this? > > > > > > Actually there is another listener that is already registered by Issue > #3247. > > So > > > this is required to keep consistency > > > > Oh, so it seems you need to rebase this patch? Think I suggested to call that > > function `initFilterListeners` then for clarity's sake. > > Acknowledged. LGTM 2
https://codereview.adblockplus.org/29350065/diff/29369547/mobile/android/thir... File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29350065/diff/29369547/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:60: private static final List<AddOnRequest> PENDING_REQUESTS = new ArrayList<>(); You've renamed this from "uncomplete" to "pending" (I prefer the latter FWIW), but there are still some references to "uncompleted" requests in the code. Those should be changed as well IMO.
https://codereview.adblockplus.org/29350065/diff/29369547/mobile/android/thir... File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right): https://codereview.adblockplus.org/29350065/diff/29369547/mobile/android/thir... mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:60: private static final List<AddOnRequest> PENDING_REQUESTS = new ArrayList<>(); On 2017/01/30 15:37:53, Felix Dahlke wrote: > You've renamed this from "uncomplete" to "pending" (I prefer the latter FWIW), > but there are still some references to "uncompleted" requests in the code. Those > should be changed as well IMO. Acknowledged.
Nice, LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
