Issue 29860578: Issue 6775 - Work around filter data stored in IndexedDB getting lost on Microsoft Edge (Closed)
DescriptionThis patch contains:
1.new module that handles creating backups using browser.storage.local
2.updates to ioIndexedDB, (restore logic in case of missing data and logic for reestablishing a connection to indexedDB if the user clears the data while the extension is running)
3.tests for the new code
Patch Set 1 #
Total comments: 31
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 1
MessagesTotal messages: 10
Hi Geo, thank a lot for working on this! https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup.js File lib/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:41: MIN_WRITE_INTERVAL = backupInterval || 60 * 1000; Just out of curiosity? Could you experience any performance difference with vs without this delay (in terms of measurable CPU usage, and/or irresponsive UI)? https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:42: saveToStorage = saveFunction || I'd rather mock browser.storage.local.set() in the tests than passing in this function. https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:103: buffer.push(""); Do we have to add an empty line here? https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:112: buffer.push( Can't we just use Subscription.serialize() (but without calling Subscription.serializeFilters()) here? https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:152: scheduleBackup, Can't we just emit one of the above events in the tests rather than calling sheduleBackup directly? https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:171: }, 0); Is this timeout necessary? https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:176: return Promise.reject(err); Nit: You can just throw the error in here. This will have the same effect as returning a rejected promise. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:191: return IndexedDBBackup.getBackupData() I wonder whether we also have to trigger the subscriptions to be downloaded again, since the backup doesn't included any filter data for downloadable subscriptions? https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:221: .then(() => Promise.resolve()); Nit: This seems redundant. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:222: return Promise.reject(error); Nit: See above. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:263: .then(() => Promise.resolve()); Nit: This seems redundant. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:266: return Promise.reject(error); Nit: See above. https://codereview.adblockplus.org/29860578/diff/29860579/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29860578/diff/29860579/metadata.edge#newco... metadata.edge:21: lib/adblockplus.js += lib/indexedDBBackup.js Is this necessary? As far as I understand, webpack should discover the depdency automatically as it is requested in ioIndexedDB.js https://codereview.adblockplus.org/29860578/diff/29860579/qunit/tests/indexed... File qunit/tests/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29860579/qunit/tests/indexed... qunit/tests/indexedDBBackup.js:62: deepEqual(actualFormat, expectedFormat, "formates the data correctly"); Maybe we should test the serialization as part of the actual write operation. Then we test the functionality end-to-end (which seems preferable), but also the tests can be agnostic of the internal serialize() function.
https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup.js File lib/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:41: MIN_WRITE_INTERVAL = backupInterval || 60 * 1000; On 2018/08/21 20:26:24, Sebastian Noack wrote: > Just out of curiosity? Could you experience any performance difference with vs > without this delay (in terms of measurable CPU usage, and/or irresponsive UI)? I've tried to measure CPU usage without too much luck, but when it comes to UI, it really depends on what the user does. I don't notice any difference between toggling subscriptions or adding more. Custom filters are a different story. I've tested with about 433KB. Saving custom filters is slow at this point, without performing any backup so having a small delay here is beneficial. Maybe it would be better to handle different updates separately. I'm not sure. https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:42: saveToStorage = saveFunction || On 2018/08/21 20:26:24, Sebastian Noack wrote: > I'd rather mock browser.storage.local.set() in the tests than passing in this > function. Done. https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:103: buffer.push(""); On 2018/08/21 20:26:24, Sebastian Noack wrote: > Do we have to add an empty line here? Done. https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:112: buffer.push( On 2018/08/21 20:26:24, Sebastian Noack wrote: > Can't we just use Subscription.serialize() (but without calling > Subscription.serializeFilters()) here? As discussed on IRC, I'm leaving the current implementation as it is, since Subscription.serialize() add some extra fields that we don't need. https://codereview.adblockplus.org/29860578/diff/29860579/lib/indexedDBBackup... lib/indexedDBBackup.js:152: scheduleBackup, On 2018/08/21 20:26:24, Sebastian Noack wrote: > Can't we just emit one of the above events in the tests rather than calling > sheduleBackup directly? Done. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:171: }, 0); On 2018/08/21 20:26:25, Sebastian Noack wrote: > Is this timeout necessary? This part here is mostly from experimentation. If I try to call `indexedDB.openDB` before calling `close` I'm getting errors. I couldn't find an event for when the close operation completes, hence the timeout. However it seems that it works on the second try without the timeout. Done. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:176: return Promise.reject(err); On 2018/08/21 20:26:25, Sebastian Noack wrote: > Nit: You can just throw the error in here. This will have the same effect as > returning a rejected promise. Done. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:191: return IndexedDBBackup.getBackupData() On 2018/08/21 20:26:25, Sebastian Noack wrote: > I wonder whether we also have to trigger the subscriptions to be downloaded > again, since the backup doesn't included any filter data for downloadable > subscriptions? No need with the information we are currently saving. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:221: .then(() => Promise.resolve()); On 2018/08/21 20:26:25, Sebastian Noack wrote: > Nit: This seems redundant. I want this to resolve with `undefined` so that `getFile` will check in `local.storage` next. I can remove it, but I have to change the `getFile` logic. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:222: return Promise.reject(error); On 2018/08/21 20:26:25, Sebastian Noack wrote: > Nit: See above. Done. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:263: .then(() => Promise.resolve()); On 2018/08/21 20:26:25, Sebastian Noack wrote: > Nit: This seems redundant. Done. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:266: return Promise.reject(error); On 2018/08/21 20:26:25, Sebastian Noack wrote: > Nit: See above. Done. https://codereview.adblockplus.org/29860578/diff/29860579/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29860578/diff/29860579/metadata.edge#newco... metadata.edge:21: lib/adblockplus.js += lib/indexedDBBackup.js On 2018/08/21 20:26:25, Sebastian Noack wrote: > Is this necessary? As far as I understand, webpack should discover the depdency > automatically as it is requested in ioIndexedDB.js It does, however the module is not initialized at startup. In ioIndexedDB I'm requiring it only if a data restore is necessary. https://codereview.adblockplus.org/29860578/diff/29860579/qunit/tests/indexed... File qunit/tests/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29860579/qunit/tests/indexed... qunit/tests/indexedDBBackup.js:62: deepEqual(actualFormat, expectedFormat, "formates the data correctly"); On 2018/08/21 20:26:25, Sebastian Noack wrote: > Maybe we should test the serialization as part of the actual write operation. > Then we test the functionality end-to-end (which seems preferable), but also the > tests can be agnostic of the internal serialize() function. Done.
https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:171: }, 0); On 2018/08/31 15:49:25, geo wrote: > On 2018/08/21 20:26:25, Sebastian Noack wrote: > > Is this timeout necessary? > > This part here is mostly from experimentation. If I try to call > `indexedDB.openDB` before calling `close` I'm getting errors. I couldn't find an > event for when the close operation completes, hence the timeout. However it > seems that it works on the second try without the timeout. > Done. Ok, let's stick with the delay then. https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:221: .then(() => Promise.resolve()); On 2018/08/31 15:49:25, geo wrote: > On 2018/08/21 20:26:25, Sebastian Noack wrote: > > Nit: This seems redundant. > > I want this to resolve with `undefined` so that `getFile` will check in > `local.storage` next. I can remove it, but I have to change the `getFile` logic. I see, then this should be: .then(() => undefined) The behavior will essentially be the same, but it's less code, and more obvious that you specifically want to return undefined here. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup.js File lib/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:26: const BACKUP_NAME = "file:indexedDB-backup"; Nit: IMO it reads better with a blank line separating the constants from the variables. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:27: let MIN_WRITE_INTERVAL; Nit: Since this isn't really a constant I would spell it in lowercase. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:40: } Could we have a setBackupInterval() function instead of an init() function, which won't cancel any pending backup, but merely changes the interval for future backups? If that works out, this might be less code, and seems a bit more robust since the current implementation could potentially cause a backup to be skipped if init() is called (again) during a pending backup. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:81: } Correct me if I'm wrong, but to me it seems the logic here could be simplified: let lastRun = -MIN_WRITE_INTERVAL; ... function scheduleBackup() { if (!lastBackup) { lastBackup = setTimeout( () => { saveToStorage(); lastBackup = null; }, lastRun + MIN_WRITE_INTERVAL - performance.now() ); } } https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:115: `disabled=${disabled}`); Nit: We wrap argument lists always with either of the following flavors: buffer.push("[Subscription]", `homepage=${homepage}`, `title=${title}`, `url=${url}`, `disabled=${disabled}`); buffer.push( "[Subscription]", `homepage=${homepage}`, `title=${title}`, `url=${url}`, `disabled=${disabled}` ); https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:163: retries = retries || 1; You can use default arguments. Also I would rather count down to zero than counting up to n. That way you have the maximum number of retries configured in a more structured way (rather than being hidden in the code), and as a side-effect you can easily override it to test something during development. function reestablishConnection(dbInstance, retries=10) { ... } https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:182: const {IndexedDBBackup} = require("./indexedDBBackup"); Nit: Can this import be moved to the top of the module, or do we have a circular dependency then? https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:194: .then(() => backupData)); Nit: The indentation is a little off here: .then(backupData => saveFile( ... ) .then(...); https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:234: saveFile(data, newDbInstance, storeName)); Nit: For consistent with other code, can you indent this like that: return reestablishConnection(dbInstance).then(newDbInstance => saveFile(data, newDbInstance, storeName) );
https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29860578/diff/29860579/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:221: .then(() => Promise.resolve()); On 2018/08/31 18:07:04, Sebastian Noack wrote: > On 2018/08/31 15:49:25, geo wrote: > > On 2018/08/21 20:26:25, Sebastian Noack wrote: > > > Nit: This seems redundant. > > > > I want this to resolve with `undefined` so that `getFile` will check in > > `local.storage` next. I can remove it, but I have to change the `getFile` > logic. > > I see, then this should be: > > .then(() => undefined) > > The behavior will essentially be the same, but it's less code, and more obvious > that you specifically want to return undefined here. Done. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup.js File lib/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:26: const BACKUP_NAME = "file:indexedDB-backup"; On 2018/08/31 18:07:05, Sebastian Noack wrote: > Nit: IMO it reads better with a blank line separating the constants from the > variables. Done. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:27: let MIN_WRITE_INTERVAL; On 2018/08/31 18:07:05, Sebastian Noack wrote: > Nit: Since this isn't really a constant I would spell it in lowercase. Done. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:40: } On 2018/08/31 18:07:04, Sebastian Noack wrote: > Could we have a setBackupInterval() function instead of an init() function, > which won't cancel any pending backup, but merely changes the interval for > future backups? If that works out, this might be less code, and seems a bit more > robust since the current implementation could potentially cause a backup to be > skipped if init() is called (again) during a pending backup. Done. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:81: } On 2018/08/31 18:07:04, Sebastian Noack wrote: > Correct me if I'm wrong, but to me it seems the logic here could be simplified: > > let lastRun = -MIN_WRITE_INTERVAL; > > ... > > function scheduleBackup() > { > if (!lastBackup) > { > lastBackup = setTimeout( > () => > { > saveToStorage(); > lastBackup = null; > }, > lastRun + MIN_WRITE_INTERVAL - performance.now() > ); > } > } Done. https://codereview.adblockplus.org/29860578/diff/29870559/lib/indexedDBBackup... lib/indexedDBBackup.js:115: `disabled=${disabled}`); On 2018/08/31 18:07:05, Sebastian Noack wrote: > Nit: We wrap argument lists always with either of the following flavors: > > buffer.push("[Subscription]", > `homepage=${homepage}`, > `title=${title}`, > `url=${url}`, > `disabled=${disabled}`); > > buffer.push( > "[Subscription]", > `homepage=${homepage}`, > `title=${title}`, > `url=${url}`, > `disabled=${disabled}` > ); Done. https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:163: retries = retries || 1; On 2018/08/31 18:07:05, Sebastian Noack wrote: > You can use default arguments. Also I would rather count down to zero than > counting up to n. That way you have the maximum number of retries configured in > a more structured way (rather than being hidden in the code), and as a > side-effect you can easily override it to test something during development. > > function reestablishConnection(dbInstance, retries=10) > { > ... > } Done. https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:182: const {IndexedDBBackup} = require("./indexedDBBackup"); On 2018/08/31 18:07:05, Sebastian Noack wrote: > Nit: Can this import be moved to the top of the module, or do we have a circular > dependency then? Yes, we end up with a circular dependency, as indexedDBBackup requires filterStorage and filterStorage requires ioIndexedDB. https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:194: .then(() => backupData)); On 2018/08/31 18:07:05, Sebastian Noack wrote: > Nit: The indentation is a little off here: > > .then(backupData => > saveFile( > ... > ) > .then(...); I've changed a bit the indentation, hopefully it's more clear now. https://codereview.adblockplus.org/29860578/diff/29870559/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:234: saveFile(data, newDbInstance, storeName)); On 2018/08/31 18:07:05, Sebastian Noack wrote: > Nit: For consistent with other code, can you indent this like that: > > return reestablishConnection(dbInstance).then(newDbInstance => > saveFile(data, newDbInstance, storeName) > ); Done.
https://codereview.adblockplus.org/29860578/diff/29874601/lib/indexedDBBackup.js File lib/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29874601/lib/indexedDBBackup... lib/indexedDBBackup.js:24: require("../adblockpluscore/lib/subscriptionClasses"); Nit: No reason to wrap twice here (also more consistent with existing code): const {DownloadableSubscription, SpecialSubscription} = require("../adblockpluscore/lib/subscriptionClasses"); https://codereview.adblockplus.org/29860578/diff/29874601/lib/indexedDBBackup... lib/indexedDBBackup.js:43: lastBackup = setTimeout( It seems now the only reason to have the lastBackup variable is to now if there is a pending backup (we never clear it anymore). So a boolean (e.g. called pendingPackup) seems more appropriate. https://codereview.adblockplus.org/29860578/diff/29874601/lib/indexedDBBackup... lib/indexedDBBackup.js:50: lastRun + minWriteInterval - performance.now() Just an idea, I'm wondering if a simpler throttling strategy would make more sense. What if we always defer the backup by a fixed delay, ignoring the time elapsed since the last backup. With the current strategy, the first write will trigger an immediate backup, and when one write occurs, another one is often going to follow, so that we might end up writing another backup exactly 60 seconds later. However, if we already defer the backup for the first write by 60 seconds, any further write that might occur within in the next 60 seconds, wouldn't trigger another backup. Also the implementation might be somewhat simpler. What do you think? https://codereview.adblockplus.org/29860578/diff/29874601/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29860578/diff/29874601/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:213: .then(() => undefined); Nit: Wrapping here seems unnecessary. It appears to fit on one line. https://codereview.adblockplus.org/29860578/diff/29874601/qunit/tests/indexed... File qunit/tests/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29874601/qunit/tests/indexed... qunit/tests/indexedDBBackup.js:44: {value: this._storageLocalSet, enumerable: true}); Nit: See previous comment(s) about wrapping argument lists, and where we prefer to put the closing parenthesis. :) Also check the code below, I see at least 2 more cases of inconsistently wrapped argument lists.
https://codereview.adblockplus.org/29860578/diff/29874601/lib/indexedDBBackup.js File lib/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29874601/lib/indexedDBBackup... lib/indexedDBBackup.js:24: require("../adblockpluscore/lib/subscriptionClasses"); On 2018/09/04 21:00:01, Sebastian Noack wrote: > Nit: No reason to wrap twice here (also more consistent with existing code): > > const {DownloadableSubscription, SpecialSubscription} = > require("../adblockpluscore/lib/subscriptionClasses"); Done. https://codereview.adblockplus.org/29860578/diff/29874601/lib/indexedDBBackup... lib/indexedDBBackup.js:43: lastBackup = setTimeout( On 2018/09/04 21:00:01, Sebastian Noack wrote: > It seems now the only reason to have the lastBackup variable is to now if there > is a pending backup (we never clear it anymore). So a boolean (e.g. called > pendingPackup) seems more appropriate. Done. https://codereview.adblockplus.org/29860578/diff/29874601/lib/indexedDBBackup... lib/indexedDBBackup.js:50: lastRun + minWriteInterval - performance.now() On 2018/09/04 21:00:01, Sebastian Noack wrote: > Just an idea, I'm wondering if a simpler throttling strategy would make more > sense. What if we always defer the backup by a fixed delay, ignoring the time > elapsed since the last backup. > > With the current strategy, the first write will trigger an immediate backup, and > when one write occurs, another one is often going to follow, so that we might > end up writing another backup exactly 60 seconds later. > > However, if we already defer the backup for the first write by 60 seconds, any > further write that might occur within in the next 60 seconds, wouldn't trigger > another backup. Also the implementation might be somewhat simpler. > > What do you think? Yup, it does make things simpler. And you are probably right about the updates. The first one is triggered by the `load` event, and we might as well defer it, plus I think that is pretty likely that if a user plays with the settings, he/she is going to change more than one setting at a time. So maybe we'll capture them in one write instead of two. I've also updated the tests, to reflect this change. https://codereview.adblockplus.org/29860578/diff/29874601/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29860578/diff/29874601/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:213: .then(() => undefined); On 2018/09/04 21:00:01, Sebastian Noack wrote: > Nit: Wrapping here seems unnecessary. It appears to fit on one line. Done. https://codereview.adblockplus.org/29860578/diff/29874601/qunit/tests/indexed... File qunit/tests/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29874601/qunit/tests/indexed... qunit/tests/indexedDBBackup.js:44: {value: this._storageLocalSet, enumerable: true}); On 2018/09/04 21:00:01, Sebastian Noack wrote: > Nit: See previous comment(s) about wrapping argument lists, and where we prefer > to put the closing parenthesis. :) > > Also check the code below, I see at least 2 more cases of inconsistently wrapped > argument lists. Done.
https://codereview.adblockplus.org/29860578/diff/29875579/lib/indexedDBBackup.js File lib/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29875579/lib/indexedDBBackup... lib/indexedDBBackup.js:27: let minWriteInterval; Nit: This is no longer a "min" but a fixed value. Also "backupDelay" seems more accurately describe it (than write interval).
https://codereview.adblockplus.org/29860578/diff/29875579/lib/indexedDBBackup.js File lib/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29875579/lib/indexedDBBackup... lib/indexedDBBackup.js:27: let minWriteInterval; On 2018/09/05 14:34:49, Sebastian Noack wrote: > Nit: This is no longer a "min" but a fixed value. Also "backupDelay" seems more > accurately describe it (than write interval). Done. I've also changed the name of `writeInterval` variable in tests/indexedDBBackup as this change made sense there as well. https://codereview.adblockplus.org/29860578/diff/29875629/qunit/tests/indexed... File qunit/tests/indexedDBBackup.js (right): https://codereview.adblockplus.org/29860578/diff/29875629/qunit/tests/indexed... qunit/tests/indexedDBBackup.js:111: setTimeout(() => I've reintroduced this timeout because without it, this test seems to get stuck after running the first 2 assertions.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
