Issue 29796555: Issue 6621 (Closed)
|
Created:
June 1, 2018, 1:58 p.m. by piscoi.georgiana Modified:
July 5, 2018, 9:37 a.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 6621 - Use IndexDB for filter storage on Edge
Patch Set 1 #
Total comments: 40
Patch Set 2 : Most of the changes from patch 1 #Patch Set 3 : Comment improvements and a rename #
Total comments: 19
Patch Set 4 : Most of the changes from P3 #
Total comments: 6
Patch Set 5 : Remaining change from p3 and changes from p4 #Patch Set 6 : Fixed tests #
Total comments: 1
MessagesTotal messages: 17
Thanks Geo, I'll take a look through this next week! In the mean time please could you give this a more descriptive title? For example something like "Issue 6621 - Use IndexDB for filter storage on Edge" would be better.
Hi Geo, thanks for the patch! The overall implementation looks great, but I have some comments, below. https://codereview.adblockplus.org/29796555/diff/29796556/dependencies File dependencies (right): https://codereview.adblockplus.org/29796555/diff/29796556/dependencies#newcode3 dependencies:3: buildtools = buildtools hg:168dac24ad9e git:f679caf I updated the buildtools dependency separately: https://issues.adblockplus.org/ticket/6721 Please rebase on top of the "next" branch. There should be no changes here (and in ensure_depdencies.py) afterwards. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:29: dbName: "adbp", This acronym seems somewhat bogus. If we abbreviate "Adblock Plus" we use "ABP", but genrally it's a good idea to avoid abbreviations as much as possible. So I'd suggest to use "adblockplus" as database name. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:35: let dbInstances = {}; I wonder whether it would be more appropriate to just have two variables (e.g. "db" and "dbLocalForage") rather than an object (used as mapping) here. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:54: migrationStatus[fileName] = new Promise((resolve, reject) => Perhaps, rather than migrating each file on demand, we can get all filenames and convert them all together upfront. Something like that: let migrationDone = getAllFiles().then( filenames => Promise.all(filenames.map(migrateFile)) ); This has following advantages: * We make sure all files get migrated. So that if at some point we remove the migration code, it cannot happen that some files which haven't been accessed in a while are still stored in the old format. * We start the migration as soon as possible, minimizing any delay when the IO API is used the first time. * It seems to simplify the logic, as we don't need to keep track and check for files that have already been migrated. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:61: fileData = file; Rather than making fileData a non-local variable, I would do it like that: .then(file => { openDB(dbConfig).then(db => { saveFile(file, db, dbConfig.storeName).then(() => { ... }); }); }); https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:102: const {dbName, storeName, version} = config; Sorry, this is matter of taste, but at least for the matter of consistency with the rest of our code, we don't use const wherever possible, but merely as an assertion in cases where reassigning a variable would inherently break things. If you wonder why, you might want to read this article: https://madhatted.com/2016/1/25/let-it-be https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioStorage.js File lib/ioStorage.js (left): https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioStorage.js#ol... lib/ioStorage.js:124: }, Well spotted! I confirmed that copyFile() and removeFile() are no longer used (by other code). So removing them here (and not implementing them in ioIndexedDB in the first place) is the right thing to do. However, removeFile() is internally called by renameFile(). So you have to adapt the code there to call browser.storage.local.remove() directly. https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge#newcode1 metadata.edge:1: [default] Thanks for fixing the newlines here (this file used CRLF while all other files use UNIX newlines). However, this makes it difficult to review the relevant changes in here. So I went ahead, and fixed the newlines in a separate commit: https://hg.adblockplus.org/adblockpluschrome/rev/b177288cd22a Please rebase on top of the "next" branch. Afterwards the only changes in here should be the added [module_alias] section.
Good work Geo, my comments are mostly "nits" (small stylistic things). https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:21: // from DefaultConfig https://github.com/localForage/localForage/blob/master/src/localforage.js Please could you improve this comment a bit? For example // Values taken from the default localforage IndexDB configuration // https://github.com/localForage/localForage/blob/2cdbd74/src/localforage.js#L4... (I added line numbers to the link and then replaced "master" with "2cdbd74" in case it changes the linked file changes in the future.) https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:22: const localForageDefs = { Nit: Would you mind naming this one more consistently with the other? For example perhaps `localforageDbConfig`? https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:35: let dbInstances = {}; On 2018/06/01 19:15:46, Sebastian Noack wrote: > I wonder whether it would be more appropriate to just have two variables (e.g. > "db" and "dbLocalForage") rather than an object (used as mapping) here. Yea, I agree. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:44: * Handles migrating a file, if found, from localforage db to the new one Would you mind specifying what "the new one" is here? https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:53: if (!migrationStatus[fileName]) Nit: When the body of an if statement spans multiple lines we add the { } braces. Did your changes pass ESLint? (If you've not tried ESLint yet have a look at the README, there are some instructions there.) https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:98: function openDB(config) I guess we could destructure `config` here? function openDb({dbName, storeName, version}) https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:145: req.onsuccess = (event) => Nit: While we don't have a rule about adding / omitting the parenthesis for arrow function arguments, please could you use them consistently? Here you added them but elsewhere you omitted them. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:151: reject({type: "NoSuchFile"}); Nit: This line isn't indented correctly. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:193: Nit: Please could you remove this newline? https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:272: Nit: Please could you remove this newline? https://codereview.adblockplus.org/29796555/diff/29796556/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.chrome#new... metadata.chrome:166: io$ = ioStorage Instead of renaming lib/io.js to lib/ioStorage.js and adding this alias why don't we just leave the file alone? https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge#newco... metadata.edge:17: qunit/tests.js += qunit/tests/ioIndexedDB.js Really cool you took the extra time to write some tests.
https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge#newco... metadata.edge:17: qunit/tests.js += qunit/tests/ioIndexedDB.js On 2018/06/04 11:06:07, kzar wrote: > Really cool you took the extra time to write some tests. Looks like the fact that the qunit/tests.js bundle is listed in the metadata file at all was a bug. So once the change for #6720 lands your new tests should be bundled and included automatically.
Thank you for reviewing my initial code. I've replied to most of the comments. The code for migrateFile has changed a bit, so some comments didn't apply any more. And sorry about the double patches. https://codereview.adblockplus.org/29796555/diff/29796556/dependencies File dependencies (right): https://codereview.adblockplus.org/29796555/diff/29796556/dependencies#newcode3 dependencies:3: buildtools = buildtools hg:168dac24ad9e git:f679caf On 2018/06/01 19:15:46, Sebastian Noack wrote: > I updated the buildtools dependency separately: > https://issues.adblockplus.org/ticket/6721 > > Please rebase on top of the "next" branch. There should be no changes here (and > in ensure_depdencies.py) afterwards. Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:21: // from DefaultConfig https://github.com/localForage/localForage/blob/master/src/localforage.js On 2018/06/04 11:06:07, kzar wrote: > Please could you improve this comment a bit? For example > > // Values taken from the default localforage IndexDB configuration > // > https://github.com/localForage/localForage/blob/2cdbd74/src/localforage.js#L4... > > (I added line numbers to the link and then replaced "master" with "2cdbd74" in > case it changes the linked file changes in the future.) I didn't knew you can link to line numbers. Thanks! I've updated as per your suggestion https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:22: const localForageDefs = { On 2018/06/04 11:06:07, kzar wrote: > Nit: Would you mind naming this one more consistently with the other? For > example perhaps `localforageDbConfig`? Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:29: dbName: "adbp", On 2018/06/01 19:15:46, Sebastian Noack wrote: > This acronym seems somewhat bogus. If we abbreviate "Adblock Plus" we use "ABP", > but genrally it's a good idea to avoid abbreviations as much as possible. So I'd > suggest to use "adblockplus" as database name. Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:35: let dbInstances = {}; On 2018/06/01 19:15:46, Sebastian Noack wrote: > I wonder whether it would be more appropriate to just have two variables (e.g. > "db" and "dbLocalForage") rather than an object (used as mapping) here. Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:35: let dbInstances = {}; On 2018/06/04 11:06:07, kzar wrote: > On 2018/06/01 19:15:46, Sebastian Noack wrote: > > I wonder whether it would be more appropriate to just have two variables (e.g. > > "db" and "dbLocalForage") rather than an object (used as mapping) here. > > Yea, I agree. Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:44: * Handles migrating a file, if found, from localforage db to the new one On 2018/06/04 11:06:07, kzar wrote: > Would you mind specifying what "the new one" is here? I've changed it, but I'm not 100% sure https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:53: if (!migrationStatus[fileName]) On 2018/06/04 11:06:07, kzar wrote: > Nit: When the body of an if statement spans multiple lines we add the { } > braces. Did your changes pass ESLint? (If you've not tried ESLint yet have a > look at the README, there are some instructions there.) It did pass. My editor shows me lint errors, but I've also ran the command, just in case. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:54: migrationStatus[fileName] = new Promise((resolve, reject) => On 2018/06/01 19:15:46, Sebastian Noack wrote: > Perhaps, rather than migrating each file on demand, we can get all filenames and > convert them all together upfront. Something like that: > > let migrationDone = getAllFiles().then( > filenames => Promise.all(filenames.map(migrateFile)) > ); > > This has following advantages: > > * We make sure all files get migrated. So that if at some point we remove > the migration code, it cannot happen that some files which haven't been > accessed in a while are still stored in the old format. > * We start the migration as soon as possible, minimizing any delay when > the IO API is used the first time. > * It seems to simplify the logic, as we don't need to keep track and check > for files that have already been migrated. Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:98: function openDB(config) On 2018/06/04 11:06:07, kzar wrote: > I guess we could destructure `config` here? > > function openDb({dbName, storeName, version}) Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:102: const {dbName, storeName, version} = config; On 2018/06/01 19:15:46, Sebastian Noack wrote: > Sorry, this is matter of taste, but at least for the matter of consistency with > the rest of our code, we don't use const wherever possible, but merely as an > assertion in cases where reassigning a variable would inherently break things. > > If you wonder why, you might want to read this article: > https://madhatted.com/2016/1/25/let-it-be For me, I guess it's mostly reflex from working on projects that were "constantly const", I didn't even notice. Thank you! I've updated the rest of the code as well. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:145: req.onsuccess = (event) => On 2018/06/04 11:06:07, kzar wrote: > Nit: While we don't have a rule about adding / omitting the parenthesis for > arrow function arguments, please could you use them consistently? Here you added > them but elsewhere you omitted them. Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:151: reject({type: "NoSuchFile"}); On 2018/06/04 11:06:07, kzar wrote: > Nit: This line isn't indented correctly. Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:193: On 2018/06/04 11:06:07, kzar wrote: > Nit: Please could you remove this newline? Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:272: On 2018/06/04 11:06:07, kzar wrote: > Nit: Please could you remove this newline? Done. https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioStorage.js File lib/ioStorage.js (left): https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioStorage.js#ol... lib/ioStorage.js:124: }, On 2018/06/01 19:15:46, Sebastian Noack wrote: > Well spotted! I confirmed that copyFile() and removeFile() are no longer used > (by other code). So removing them here (and not implementing them in ioIndexedDB > in the first place) is the right thing to do. > > However, removeFile() is internally called by renameFile(). So you have to adapt > the code there to call browser.storage.local.remove() directly. Done. https://codereview.adblockplus.org/29796555/diff/29796556/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.chrome#new... metadata.chrome:166: io$ = ioStorage On 2018/06/04 11:06:07, kzar wrote: > Instead of renaming lib/io.js to lib/ioStorage.js and adding this alias why > don't we just leave the file alone? I suppose it can work either way. My only concern would be if someone adds new methods to the api, will they notice that there are 2 files that have to be updated? https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge#newco... metadata.edge:17: qunit/tests.js += qunit/tests/ioIndexedDB.js On 2018/06/04 18:06:56, kzar wrote: > On 2018/06/04 11:06:07, kzar wrote: > > Really cool you took the extra time to write some tests. > > Looks like the fact that the qunit/tests.js bundle is listed in the metadata > file at all was a bug. So once the change for #6720 lands your new tests should > be bundled and included automatically. I'll keep an eye out for the change and update accordingly
https://codereview.adblockplus.org/29796555/diff/29799557/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29796555/diff/29799557/lib/io.js#oldcode85 lib/io.js:85: * Copies a file. Nice, always good to remove old code where possible. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:35: let db; I'd rather we stored the connection promise here, the APIs we export are all asynchronous anyway. Something like this: let db = openDB({ dbName: "adblockplus", ... ); https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:36: let localForageDb; We don't use the localforage database connection outside of the migration function, so I figure we can get rid of this variable. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:50: return openDB(dbConfig) Nit: I think we should move the openDB function's definition above, since we're calling it here. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:53: db = dbInstance; (I think assigning something from a promise which we later use synchronously is worth avoiding if possible, which is why I suggested to just store the promise in the comment above.) https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:63: Promise.all(saveFile(file, db, dbConfig.storeName)))) Supposing a file exists in both databases wouldn't this logic overwrite the one stored in the new database? I think we should probably skip files which already exist in the new database. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:65: clearObjectStore(localForageDb, localForageDbConfig.storeName)); I wonder what will happen the next time we try to open this database after we've cleared it? Will it throw?
https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge#newco... metadata.edge:17: qunit/tests.js += qunit/tests/ioIndexedDB.js On 2018/06/04 18:06:56, kzar wrote: > On 2018/06/04 11:06:07, kzar wrote: > > Really cool you took the extra time to write some tests. > > Looks like the fact that the qunit/tests.js bundle is listed in the metadata > file at all was a bug. So once the change for #6720 lands your new tests should > be bundled and included automatically. Done. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:35: let db; On 2018/06/05 12:38:53, kzar wrote: > I'd rather we stored the connection promise here, the APIs we export are all > asynchronous anyway. Something like this: > > let db = openDB({ > dbName: "adblockplus", > ... > ); Done. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:36: let localForageDb; On 2018/06/05 12:38:53, kzar wrote: > We don't use the localforage database connection outside of the migration > function, so I figure we can get rid of this variable. Done. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:50: return openDB(dbConfig) On 2018/06/05 12:38:53, kzar wrote: > Nit: I think we should move the openDB function's definition above, since we're > calling it here. Is there any specific rule/preference regarding reading direction? Normally, I like to put my exports at the top, and everything else, in a top-bottom order, with functions that are called from multiple places last. I'm only asking because, I think I should probably move more than just this one function. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:53: db = dbInstance; On 2018/06/05 12:38:53, kzar wrote: > (I think assigning something from a promise which we later use synchronously is > worth avoiding if possible, which is why I suggested to just store the promise > in the comment above.) Done. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:63: Promise.all(saveFile(file, db, dbConfig.storeName)))) On 2018/06/05 12:38:53, kzar wrote: > Supposing a file exists in both databases wouldn't this logic overwrite the one > stored in the new database? I think we should probably skip files which already > exist in the new database. Yes, it will override any file that is present in both databases, however that scenario is possible, only if the user, somehow used the new version, without localForage, then used the version with localForage, and finally back to the new version. I think this scenario is unlikely, so I didn't treat it, but if you think that it might happen, I'll change the implementation. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:65: clearObjectStore(localForageDb, localForageDbConfig.storeName)); On 2018/06/05 12:38:53, kzar wrote: > I wonder what will happen the next time we try to open this database after we've > cleared it? Will it throw? No, it won't throw. The cursor value will be null, so the getAllFiles will resolve with an empty array.
https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:50: return openDB(dbConfig) On 2018/06/11 07:02:45, geo wrote: > On 2018/06/05 12:38:53, kzar wrote: > > Nit: I think we should move the openDB function's definition above, since > we're > > calling it here. > > Is there any specific rule/preference regarding reading direction? Normally, I > like to put my exports at the top, and everything else, in a top-bottom order, > with functions that are called from multiple places last. > I'm only asking because, I think I should probably move more than just this one > function. No rule that I know of, just something I try to do since personally I find it kind of confusing to use something above where it's defined. I don't insist anyway, if you prefer it this way around that's fair enough. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:63: Promise.all(saveFile(file, db, dbConfig.storeName)))) On 2018/06/11 07:02:45, geo wrote: > On 2018/06/05 12:38:53, kzar wrote: > > Supposing a file exists in both databases wouldn't this logic overwrite the > one > > stored in the new database? I think we should probably skip files which > already > > exist in the new database. > > Yes, it will override any file that is present in both databases, however that > scenario is possible, only if the user, somehow used the new version, without > localForage, then used the version with localForage, and finally back to the new > version. I think this scenario is unlikely, so I didn't treat it, but if you > think that it might happen, I'll change the implementation. What do you think Sebastian? I think I'd prefer that the file is only copied from the old database if it doesn't exist in the new database yet. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:65: clearObjectStore(localForageDb, localForageDbConfig.storeName)); On 2018/06/11 07:02:44, geo wrote: > On 2018/06/05 12:38:53, kzar wrote: > > I wonder what will happen the next time we try to open this database after > we've > > cleared it? Will it throw? > > No, it won't throw. The cursor value will be null, so the getAllFiles will > resolve with an empty array. Acknowledged. https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:53: return getAllFiles(localForageDb, localForageDbConfig.storeName) Nit: I guess we can omit the curly braces and the `return` here, like you've done elsewhere. https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:256: .then(() => db) Couldn't we do `db.then(dbInstance => ...` instead? (Same elsewhere.)
https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:50: return openDB(dbConfig) On 2018/06/12 10:46:16, kzar wrote: > On 2018/06/11 07:02:45, geo wrote: > > On 2018/06/05 12:38:53, kzar wrote: > > > Nit: I think we should move the openDB function's definition above, since > > we're > > > calling it here. > > > > Is there any specific rule/preference regarding reading direction? Normally, I > > like to put my exports at the top, and everything else, in a top-bottom order, > > with functions that are called from multiple places last. > > I'm only asking because, I think I should probably move more than just this > one > > function. > > No rule that I know of, just something I try to do since personally I find it > kind of confusing to use something above where it's defined. I don't insist > anyway, if you prefer it this way around that's fair enough. The idea is to increase code locality, i.e. so that when someone studies the code, following the path of execution across functions defined in here, you have to scroll as less as possible. There is no strict rule though. https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:63: Promise.all(saveFile(file, db, dbConfig.storeName)))) On 2018/06/12 10:46:16, kzar wrote: > On 2018/06/11 07:02:45, geo wrote: > > On 2018/06/05 12:38:53, kzar wrote: > > > Supposing a file exists in both databases wouldn't this logic overwrite the > > one > > > stored in the new database? I think we should probably skip files which > > already > > > exist in the new database. > > > > Yes, it will override any file that is present in both databases, however that > > scenario is possible, only if the user, somehow used the new version, without > > localForage, then used the version with localForage, and finally back to the > new > > version. I think this scenario is unlikely, so I didn't treat it, but if you > > think that it might happen, I'll change the implementation. > > What do you think Sebastian? I think I'd prefer that the file is only copied > from the old database if it doesn't exist in the new database yet. I think it's fine to copy the file over from localForage without checking if it already exists in the new database (if that keeps the logic simpler). There is no practical scenario in which users would/could downgrade to an older version, and we never bothered about that scenario when we did similar migrations before. https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:56: Promise.all(saveFile(file, dbInstance, dbConfig.storeName))))) Please apologize if I'm wrong, but to me this seems like a bug: saveFile() returns a promise (not an array of promises). So why do we call Promise.all() here? Did you mean to pass the result of files.map(...) to Promise.all()?
https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:50: return openDB(dbConfig) On 2018/06/13 00:55:11, Sebastian Noack wrote: > On 2018/06/12 10:46:16, kzar wrote: > > On 2018/06/11 07:02:45, geo wrote: > > > On 2018/06/05 12:38:53, kzar wrote: > > > > Nit: I think we should move the openDB function's definition above, since > > > we're > > > > calling it here. > > > > > > Is there any specific rule/preference regarding reading direction? Normally, > I > > > like to put my exports at the top, and everything else, in a top-bottom > order, > > > with functions that are called from multiple places last. > > > I'm only asking because, I think I should probably move more than just this > > one > > > function. > > > > No rule that I know of, just something I try to do since personally I find it > > kind of confusing to use something above where it's defined. I don't insist > > anyway, if you prefer it this way around that's fair enough. > > The idea is to increase code locality, i.e. so that when someone studies the > code, following the path of execution across functions defined in here, you have > to scroll as less as possible. There is no strict rule though. Done. https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:53: return getAllFiles(localForageDb, localForageDbConfig.storeName) On 2018/06/12 10:46:17, kzar wrote: > Nit: I guess we can omit the curly braces and the `return` here, like you've > done elsewhere. Done. https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:56: Promise.all(saveFile(file, dbInstance, dbConfig.storeName))))) On 2018/06/13 00:55:12, Sebastian Noack wrote: > Please apologize if I'm wrong, but to me this seems like a bug: saveFile() > returns a promise (not an array of promises). So why do we call Promise.all() > here? Did you mean to pass the result of files.map(...) to Promise.all()? Spot on, thank you! I've fixed it. https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... lib/ioIndexedDB.js:256: .then(() => db) On 2018/06/12 10:46:17, kzar wrote: > Couldn't we do `db.then(dbInstance => ...` instead? (Same elsewhere.) Yes, it looks better your way. Thank you!
On 2018/06/14 13:41:27, geo wrote: > https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js > File lib/ioIndexedDB.js (right): > > https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#... > lib/ioIndexedDB.js:50: return openDB(dbConfig) > On 2018/06/13 00:55:11, Sebastian Noack wrote: > > On 2018/06/12 10:46:16, kzar wrote: > > > On 2018/06/11 07:02:45, geo wrote: > > > > On 2018/06/05 12:38:53, kzar wrote: > > > > > Nit: I think we should move the openDB function's definition above, > since > > > > we're > > > > > calling it here. > > > > > > > > Is there any specific rule/preference regarding reading direction? > Normally, > > I > > > > like to put my exports at the top, and everything else, in a top-bottom > > order, > > > > with functions that are called from multiple places last. > > > > I'm only asking because, I think I should probably move more than just > this > > > one > > > > function. > > > > > > No rule that I know of, just something I try to do since personally I find > it > > > kind of confusing to use something above where it's defined. I don't insist > > > anyway, if you prefer it this way around that's fair enough. > > > > The idea is to increase code locality, i.e. so that when someone studies the > > code, following the path of execution across functions defined in here, you > have > > to scroll as less as possible. There is no strict rule though. > > Done. > > https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js > File lib/ioIndexedDB.js (right): > > https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... > lib/ioIndexedDB.js:53: return getAllFiles(localForageDb, > localForageDbConfig.storeName) > On 2018/06/12 10:46:17, kzar wrote: > > Nit: I guess we can omit the curly braces and the `return` here, like you've > > done elsewhere. > > Done. > > https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... > lib/ioIndexedDB.js:56: Promise.all(saveFile(file, dbInstance, > dbConfig.storeName))))) > On 2018/06/13 00:55:12, Sebastian Noack wrote: > > Please apologize if I'm wrong, but to me this seems like a bug: saveFile() > > returns a promise (not an array of promises). So why do we call Promise.all() > > here? Did you mean to pass the result of files.map(...) to Promise.all()? > > Spot on, thank you! I've fixed it. > > https://codereview.adblockplus.org/29796555/diff/29804555/lib/ioIndexedDB.js#... > lib/ioIndexedDB.js:256: .then(() => db) > On 2018/06/12 10:46:17, kzar wrote: > > Couldn't we do `db.then(dbInstance => ...` instead? (Same elsewhere.) > > Yes, it looks better your way. Thank you! FWIW, I have tried this patch and everything seems to work fine in devenv. LGTM. Nice work!
Also LGTM from my side.
LGTM, nice work. Please email a (squashed) patch to Sebastian or myself when you get a chance and we'll push it for you.
I went ahead, downloading the patch from Rietveld and pushing it: https://hg.adblockplus.org/adblockpluschrome/rev/d97edcab2c18 Thanks for your contribution! Feel free to close this review now.
Please could you open a new review for the test fix? This one should be closed now since the change already landed. Anyway I have one idea for an alternative approach, see the inline comment below. https://codereview.adblockplus.org/29796555/diff/29822560/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29822560/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:21: const isEdge = info.platform == "edgehtml"; How about something like this? let testEdge = require("info").platform == "edgehtml" ? QUnit.test : QUnit.skip; Then we could replace the `test` calls with `testEdge`.
On 2018/07/04 15:45:59, kzar wrote: > Please could you open a new review for the test fix? This one should be closed > now since the change already landed. > > Anyway I have one idea for an alternative approach, see the inline comment > below. > > https://codereview.adblockplus.org/29796555/diff/29822560/qunit/tests/ioIndex... > File qunit/tests/ioIndexedDB.js (right): > > https://codereview.adblockplus.org/29796555/diff/29822560/qunit/tests/ioIndex... > qunit/tests/ioIndexedDB.js:21: const isEdge = info.platform == "edgehtml"; > How about something like this? > > let testEdge = require("info").platform == "edgehtml" ? QUnit.test : > QUnit.skip; > > Then we could replace the `test` calls with `testEdge`. Yes, here is the new review https://codereview.adblockplus.org/29823569/ 
|
