Issue 29367480: Issue 4721 - Use IndexedDB for storage in Edge (Closed)
|
Created:
Dec. 14, 2016, 1:48 a.m. by Oleksandr Modified:
Jan. 12, 2017, 10:06 a.m. Visibility:
Public. |
DescriptionIssue 4721 - Use IndexedDB for storage in Edge
Patch Set 1 #
Total comments: 11
Patch Set 2 : Don't introduce unnecessary changes from old code #
Total comments: 3
Patch Set 3 : Cleanup unnecessary changes #
Total comments: 8
Patch Set 4 : Address some nits #
Total comments: 2
MessagesTotal messages: 11
https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#newcode59 lib/io.js:59: content: Array.from(data) Note: While the rest of this code is exactly the same as in https://hg.adblockplus.org/adblockplusedgems/rev/5a8b109447d3, this line was adapted to a recent change in adblockpluschrome.
As long, as this never gets merged upstream, I guess that should be fine. Alternatively, we could just add another file which replaces lib/io.js via metadata.edge, as I initially suggested. kzar, what do you think?
> As long, as this never gets merged upstream, I guess that > should be fine. Alternatively, we could just add another file > which replaces lib/io.js via metadata.edge, as I initially > suggested. kzar, what do you think? I don't mind too much, happy to go with this approach. (Also assuming it never gets merged upstream.) I'd like the changes to io.js tidied up however, quite a lot of them seem pointless and will make merging / rebasing harder than necessary. https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#oldcode77 lib/io.js:77: Mind going through the diff and fixing pointless changes like these blank lines? That would make rebasing etc easier in the future. https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#newcode27 lib/io.js:27: var key = fileToKey(file); Any reason you've changed `let` and `const` to `var` in the file? https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#newcode28 lib/io.js:28: // Make sure we do not have subscriptions in localStorage from older versions first Nit: This comment is too long, mind wrapping the line? https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#newcode76 lib/io.js:76: for (var _loopIndex15 = 0; _loopIndex15 < entry.content.length; ++_loopIndex15) What's the point of this change to the loop? Seems like the old code would do the same thing if you just added the `if ("content" in entry)` check. https://codereview.adblockplus.org/29367480/diff/29367481/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29367480/diff/29367481/metadata.edge#newcode9 metadata.edge:9: backgroundScripts = lib/polyfills/fetch.js FYI: This just got removed from master, watch out if you rebase again.
https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#oldcode77 lib/io.js:77: On 2016/12/16 13:22:25, kzar wrote: > Mind going through the diff and fixing pointless changes like these blank lines? > That would make rebasing etc easier in the future. Done. https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#newcode27 lib/io.js:27: var key = fileToKey(file); On 2016/12/16 13:22:25, kzar wrote: > Any reason you've changed `let` and `const` to `var` in the file? Done. https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#newcode28 lib/io.js:28: // Make sure we do not have subscriptions in localStorage from older versions first On 2016/12/16 13:22:25, kzar wrote: > Nit: This comment is too long, mind wrapping the line? Done. https://codereview.adblockplus.org/29367480/diff/29367481/lib/io.js#newcode76 lib/io.js:76: for (var _loopIndex15 = 0; _loopIndex15 < entry.content.length; ++_loopIndex15) On 2016/12/16 13:22:25, kzar wrote: > What's the point of this change to the loop? Seems like the old code would do > the same thing if you just added the `if ("content" in entry)` check. Done. https://codereview.adblockplus.org/29367480/diff/29367481/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29367480/diff/29367481/metadata.edge#newcode9 metadata.edge:9: backgroundScripts = lib/polyfills/fetch.js On 2016/12/16 13:22:25, kzar wrote: > FYI: This just got removed from master, watch out if you rebase again. Acknowledged.
https://codereview.adblockplus.org/29367480/diff/29368707/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29367480/diff/29368707/lib/io.js#newcode66 lib/io.js:66: exports.IO = { Nit: looks like you missed this change, please could you move the { back? https://codereview.adblockplus.org/29367480/diff/29368707/lib/io.js#newcode100 lib/io.js:100: loadFile(file, onLoaded, callback); Seems to be a mistake? (Same below.)
https://codereview.adblockplus.org/29367480/diff/29368707/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29367480/diff/29368707/lib/io.js#newcode100 lib/io.js:100: loadFile(file, onLoaded, callback); On 2016/12/19 13:31:04, kzar wrote: > Seems to be a mistake? (Same below.) Yep. We don't use any of these functions anyway, so it went unnoticed. In fact the whole storage code can be refactored now that we don't use backups.
Otherwise LGTM https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js#oldcode28 lib/io.js:28: Nit: Mind adding this blank line back in? https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js#oldcode51 lib/io.js:51: Nit: Mind adding this blank line back in? https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js#newcode59 lib/io.js:59: lastModified: Date.now(), Nit: Mind putting these keys lastModified and content in the order they used to be? (content first, then lastModified) https://codereview.adblockplus.org/29367480/diff/29369404/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29367480/diff/29369404/metadata.edge#newcode5 metadata.edge:5: version = 0.9.10.0 I assume this "change" is actually due to rebase, but if not I guess you need to remove this.
https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js#oldcode28 lib/io.js:28: On 2016/12/20 11:09:01, kzar wrote: > Nit: Mind adding this blank line back in? Done. https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js#oldcode51 lib/io.js:51: On 2016/12/20 11:09:01, kzar wrote: > Nit: Mind adding this blank line back in? Done. https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29367480/diff/29369404/lib/io.js#newcode59 lib/io.js:59: lastModified: Date.now(), On 2016/12/20 11:09:01, kzar wrote: > Nit: Mind putting these keys lastModified and content in the order they used to > be? (content first, then lastModified) Done. https://codereview.adblockplus.org/29367480/diff/29369404/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29367480/diff/29369404/metadata.edge#newcode5 metadata.edge:5: version = 0.9.10.0 On 2016/12/20 11:09:02, kzar wrote: > I assume this "change" is actually due to rebase, but if not I guess you need to > remove this. Yes, it is due to rebase.
https://codereview.adblockplus.org/29367480/diff/29369419/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29367480/diff/29369419/metadata.edge#newco... metadata.edge:10: backgroundScripts = lib/polyfills/fetch.js Is it necessary to duplicate all scripts here? Can't you just do it like that: backgroundScripts += lib/localforage.min.js
https://codereview.adblockplus.org/29367480/diff/29369419/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29367480/diff/29369419/metadata.edge#newco... metadata.edge:10: backgroundScripts = lib/polyfills/fetch.js On 2017/01/11 15:17:23, Sebastian Noack wrote: > Is it necessary to duplicate all scripts here? Can't you just do it like that: > > backgroundScripts += lib/localforage.min.js The order is important here. localforage has to be included before adblockplus.js, for example.
LGTM (for the "edge" branch only). As discussed before this change should never make it into upstream. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
