Issue 29348698: NoIssue - Use localStorage for storing filters in current Windows Store version of ABP for Edge (Closed)
|
Created:
July 27, 2016, 7:29 a.m. by Oleksandr Modified:
July 29, 2016, 4:19 p.m. CC:
Wladimir Palant, Felix Dahlke Visibility:
Public. |
DescriptionNoIssue - Use localStorage for storing filters in current Windows Store version of ABP for Edge
Patch Set 1 #
Total comments: 3
Patch Set 2 : Display an alert when trying to save to full localStorage #
Total comments: 4
Patch Set 3 : Address the nits #Patch Set 4 : Fallback to chrome.storage.local to read initialization data #
Total comments: 16
Patch Set 5 : Address the comments #MessagesTotal messages: 18
https://codereview.adblockplus.org/29348698/diff/29348699/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348699/lib/adblockplus.js#... lib/adblockplus.js:364: if (dataInJson != null) Perhaps just check if dataInJson is truthy like the previous code did? (The `!= null` comparison makes me nervous since the data could apparently be either null or a string at this point.) Nit: I liked the name entry better than dataInJson. https://codereview.adblockplus.org/29348698/diff/29348699/lib/adblockplus.js#... lib/adblockplus.js:374: localStorage.setItem(fileToKey(file), JSON.stringify({ Nit: The indentation here looks kind of weird. How about this? localStorage.setItem(fileToKey(file), JSON.stringify({ content: data, lastModified: Date.now() })); https://codereview.adblockplus.org/29348698/diff/29348699/lib/adblockplus.js#... lib/adblockplus.js:382: // QuotaExceededError can happen. Ignore silently Maybe we should display an alert that asks the user to remove some subscriptions?
All valid points. Addressed.
https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js#... lib/adblockplus.js:363: entry = localStorage.getItem(key); You missed the `var`. https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js#... lib/adblockplus.js:382: alert("Subscription storage is full. Please remove some subscriptions and try again."); Nit: These lines are too long. https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js#... lib/adblockplus.js:383: callback(new Error("Subscription storage is full. Please remove some subscriptions and try again.")); Mind putting the message in a variable instead of duplicating it? Also what about localisation? (I'm not sure if that's out of scope for now or not.)
https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js#... lib/adblockplus.js:383: callback(new Error("Subscription storage is full. Please remove some subscriptions and try again.")); On 2016/07/27 10:08:14, kzar wrote: > Mind putting the message in a variable instead of duplicating it? > > Also what about localisation? (I'm not sure if that's out of scope for now or > not.) If we are going to push this we should do it ASAP. Unfortunately I don't think there's time to a translation round anymore.
LGTM What do you think Sebastian, it worth localising the message that's displayed when storage is full?
That should be good enough, for the temporary version we currently have on the store. LGTM.
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:363: var fileDoesNotExistMessage = "File doesn't exist"; Any reason to put this text into a variable rather than passing it inline? It seems its only used once. https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:364: entry = localStorage.getItem(key); Did you forget the var statement here? https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:369: // Also check the chrome.storage.local Just to clarify, so this is for when migrating from a version that had some data previously stored in chrome.storage.local, or what's the scenario? https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:375: { Nit: The inner braces can be omitted.
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:363: var fileDoesNotExistMessage = "File doesn't exist"; Nit: Don't see the purpose of this variable. https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:364: entry = localStorage.getItem(key); You've missed the `var` again. https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:366: successCallback(JSON.parse(entry)); Nit: Mind adding the { } braces for the if clause since you use them for the else? https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:375: { Nit: Mind removing the braces for the if + else here?
Done. Man you guys are in sync :) https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:369: // Also check the chrome.storage.local On 2016/07/27 16:18:16, Sebastian Noack wrote: > Just to clarify, so this is for when migrating from a version that had some data > previously stored in chrome.storage.local, or what's the scenario? No, this is for the first run when there is no patterns.ini at all.
LGTM
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:369: // Also check the chrome.storage.local On 2016/07/27 16:33:30, Oleksandr wrote: > On 2016/07/27 16:18:16, Sebastian Noack wrote: > > Just to clarify, so this is for when migrating from a version that had some > data > > previously stored in chrome.storage.local, or what's the scenario? > > No, this is for the first run when there is no patterns.ini at all. Then I don't understand what this workaround does. If the file just doesn't exist you get an error either way. So what's the difference?
https://codereview.adblockplus.org/29348698/diff/29348738/manifest.json File manifest.json (right): https://codereview.adblockplus.org/29348698/diff/29348738/manifest.json#newco... manifest.json:74: "version": "0.9.6.0", Don't you have to update the AppXManifest as well, or isn't it in the repo?
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:369: // Also check the chrome.storage.local On 2016/07/27 16:45:43, Sebastian Noack wrote: > On 2016/07/27 16:33:30, Oleksandr wrote: > > On 2016/07/27 16:18:16, Sebastian Noack wrote: > > > Just to clarify, so this is for when migrating from a version that had some > > data > > > previously stored in chrome.storage.local, or what's the scenario? > > > > No, this is for the first run when there is no patterns.ini at all. > > Then I don't understand what this workaround does. If the file just doesn't > exist you get an error either way. So what's the difference? I misdiagnosed it, you are right. It is only needed to make IO.readFile to respond asynchronously, so setTimeout() would in fact work too. However this does fix the issue as well, and it tries to not deviate from the original code too much, so I think this is fine as well. https://codereview.adblockplus.org/29348698/diff/29348738/manifest.json File manifest.json (right): https://codereview.adblockplus.org/29348698/diff/29348738/manifest.json#newco... manifest.json:74: "version": "0.9.6.0", On 2016/07/27 17:08:51, Sebastian Noack wrote: > Don't you have to update the AppXManifest as well, or isn't it in the repo? It isn't in the repo.
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:369: // Also check the chrome.storage.local On 2016/07/27 18:23:40, Oleksandr wrote: > On 2016/07/27 16:45:43, Sebastian Noack wrote: > > On 2016/07/27 16:33:30, Oleksandr wrote: > > > On 2016/07/27 16:18:16, Sebastian Noack wrote: > > > > Just to clarify, so this is for when migrating from a version that had > some > > > data > > > > previously stored in chrome.storage.local, or what's the scenario? > > > > > > No, this is for the first run when there is no patterns.ini at all. > > > > Then I don't understand what this workaround does. If the file just doesn't > > exist you get an error either way. So what's the difference? > > I misdiagnosed it, you are right. It is only needed to make IO.readFile to > respond asynchronously, so setTimeout() would in fact work too. However this > does fix the issue as well, and it tries to not deviate from the original code > too much, so I think this is fine as well. Well, using setTimeout() would be much simpler. Also note that currently, you only respond asynchronously if the data aren't found in localStorage which still might cause similar issues under slightly different circumstances, I suppose. https://codereview.adblockplus.org/29348698/diff/29348738/manifest.json File manifest.json (right): https://codereview.adblockplus.org/29348698/diff/29348738/manifest.json#newco... manifest.json:74: "version": "0.9.6.0", On 2016/07/27 18:23:40, Oleksandr wrote: > On 2016/07/27 17:08:51, Sebastian Noack wrote: > > Don't you have to update the AppXManifest as well, or isn't it in the repo? > > It isn't in the repo. How do you generate the AppXManifest? Unless it's automatic generated, I think it should be in the repo.
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#... lib/adblockplus.js:369: // Also check the chrome.storage.local On 2016/07/27 18:29:43, Sebastian Noack wrote: > On 2016/07/27 18:23:40, Oleksandr wrote: > > On 2016/07/27 16:45:43, Sebastian Noack wrote: > > > On 2016/07/27 16:33:30, Oleksandr wrote: > > > > On 2016/07/27 16:18:16, Sebastian Noack wrote: > > > > > Just to clarify, so this is for when migrating from a version that had > > some > > > > data > > > > > previously stored in chrome.storage.local, or what's the scenario? > > > > > > > > No, this is for the first run when there is no patterns.ini at all. > > > > > > Then I don't understand what this workaround does. If the file just doesn't > > > exist you get an error either way. So what's the difference? > > > > I misdiagnosed it, you are right. It is only needed to make IO.readFile to > > respond asynchronously, so setTimeout() would in fact work too. However this > > does fix the issue as well, and it tries to not deviate from the original code > > too much, so I think this is fine as well. > > Well, using setTimeout() would be much simpler. Also note that currently, you > only respond asynchronously if the data aren't found in localStorage which still > might cause similar issues under slightly different circumstances, I suppose. (Not quite) LGTM but since the code as-is already passed QA, I don't want to have yet another QA round for this, since we also run out of time. 
|
