Issue 29367151: Issue 4721 - Use IndexedDB for storage in Edge (Closed)
|
Created:
Dec. 9, 2016, 6:38 a.m. by Oleksandr Modified:
Dec. 14, 2016, 1:54 a.m. Visibility:
Public. |
DescriptionIssue 4721 - Use IndexedDB for storage in Edge
Patch Set 1 #
Total comments: 21
Patch Set 2 : Use arrow functions. Remove code duplication. Address comments and nits. #
Total comments: 6
MessagesTotal messages: 9
I have not reviewed this yet but I agree with Sebastian here. I think a better approach is to push the Edge extension team to fix the storage problem. We don't have to come out of beta before their API works. (I assume there is already a bug filed for that with them.)
On 2016/12/12 12:16:40, kzar wrote: > I have not reviewed this yet but I agree with Sebastian here. I think a better > approach is to push the Edge extension team to fix the storage problem. We don't > have to come out of beta before their API works. (I assume there is already a > bug filed for that with them.) After a discussion in IRC there are two things I did not realise: 1. Having our Adblock Plus for Edge release based on the edge branch of adblockpluschrome is an aim in itself for legal reasons. 2. This IndexedDB code is never intended for the master branch anyway. So I'm no longer against the idea of this change set landing in the edge branch and will start to review it. (I have not had a chance to see what Sebastian's opinion is however.) (Also for reference here is the review of the similar change to the existing release https://codereview.adblockplus.org/29355962 .)
https://codereview.adblockplus.org/29367151/diff/29367152/README.md File README.md (right): https://codereview.adblockplus.org/29367151/diff/29367152/README.md#newcode54 README.md:54: For Edge you should load _devenv.edge_ as an unpacked extension directory. After Seems like these REAMDE changes clash with the other review https://codereview.adblockplus.org/29365879/diff/29367022/README.md ? They should be moved there, they are unrelated to the IndexedDB change. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background.js File edge/ext/background.js (right): https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:20: /* Storage redirection to localforage */ If you're going to add a comment here you might as well give some more information about the Edge limitation including a link to the bug report and an explanation about how this workaround helps. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:25: var promisedValues = keys.map(function(key) Why not just use localforage.getItem directly? Also we might as well use arrow functions (same elsewhere). How about something like this: Promise.all(keys.map(localforage.getItem)).then(values => { ... https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:31: if (!Array.isArray(keys)) If it's possible that keys isn't an array then wouldn't the earlier code which maps it have broken? https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:37: keys.forEach(function(key, idx) Would be clearer like this: for (let i = 0; i < keys.lengthl i++) if (values[i] != null) items[keys[i]] = values[i]; https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:52: localforage.setItem(key, value, callback); Shouldn't we wait until setItem finishes before dispatching the event? (If you call setItem without the callback argument I guess you'll be given a promise which you could use for that.) Same below. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:62: localforage.setItem(key, value, callback); Seems ugly how this nearly identical logic is duplicated, any way to avoid that? https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:74: localforage.removeItem(key); What about the callback? https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:78: Nit: No need for this blank line.
Patch Set 2 : Use arrow functions. Remove code duplication. Address comments and nits. https://codereview.adblockplus.org/29367151/diff/29367152/README.md File README.md (right): https://codereview.adblockplus.org/29367151/diff/29367152/README.md#newcode54 README.md:54: For Edge you should load _devenv.edge_ as an unpacked extension directory. After On 2016/12/12 14:17:50, kzar wrote: > Seems like these REAMDE changes clash with the other review > https://codereview.adblockplus.org/29365879/diff/29367022/README.md ? They > should be moved there, they are unrelated to the IndexedDB change. Done. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background.js File edge/ext/background.js (right): https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:20: /* Storage redirection to localforage */ On 2016/12/12 14:17:50, kzar wrote: > If you're going to add a comment here you might as well give some more > information about the Edge limitation including a link to the bug report and an > explanation about how this workaround helps. Done. There is no bug report in Microsoft Edge issue tracker on this, however. And since it is supposedly fixed in the Fast Ring already there is no point creating one now. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:25: var promisedValues = keys.map(function(key) On 2016/12/12 14:17:50, kzar wrote: > Why not just use localforage.getItem directly? Also we might as well use arrow > functions (same elsewhere). How about something like this: > > Promise.all(keys.map(localforage.getItem)).then(values => > { > ... localForage.getItem can't handle the array of keys, unfortunately. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:31: if (!Array.isArray(keys)) On 2016/12/12 14:17:50, kzar wrote: > If it's possible that keys isn't an array then wouldn't the earlier code which > maps it have broken? Fair enough. Removed this check. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:37: keys.forEach(function(key, idx) On 2016/12/12 14:17:50, kzar wrote: > Would be clearer like this: > > for (let i = 0; i < keys.lengthl i++) > if (values[i] != null) > items[keys[i]] = values[i]; Done. I thought functional way was less C++'y :) https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:52: localforage.setItem(key, value, callback); On 2016/12/12 14:17:50, kzar wrote: > Shouldn't we wait until setItem finishes before dispatching the event? (If you > call setItem without the callback argument I guess you'll be given a promise > which you could use for that.) > > Same below. Done. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:62: localforage.setItem(key, value, callback); On 2016/12/12 14:17:50, kzar wrote: > Seems ugly how this nearly identical logic is duplicated, any way to avoid that? Done. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:74: localforage.removeItem(key); On 2016/12/12 14:17:50, kzar wrote: > What about the callback? Done. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:78: On 2016/12/12 14:17:50, kzar wrote: > Nit: No need for this blank line. Done.
https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background.js File edge/ext/background.js (right): https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:25: var promisedValues = keys.map(function(key) On 2016/12/13 06:21:18, Oleksandr wrote: > On 2016/12/12 14:17:50, kzar wrote: > > Why not just use localforage.getItem directly? Also we might as well use arrow > > functions (same elsewhere). How about something like this: > > > > Promise.all(keys.map(localforage.getItem)).then(values => > > { > > ... > > localForage.getItem can't handle the array of keys, unfortunately. I know. I don't think I worded my suggestion very well but look at the code snippet in my above comment to see what I meant. https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:31: if (!Array.isArray(keys)) On 2016/12/13 06:21:19, Oleksandr wrote: > On 2016/12/12 14:17:50, kzar wrote: > > If it's possible that keys isn't an array then wouldn't the earlier code which > > maps it have broken? > > Fair enough. Removed this check. Well are you sure that keys will always be an Array? Did you check the code? https://codereview.adblockplus.org/29367151/diff/29367152/edge/ext/background... edge/ext/background.js:37: keys.forEach(function(key, idx) On 2016/12/13 06:21:19, Oleksandr wrote: > Done. I thought functional way was less C++'y :) Thanks. (It's not functional to iteratively mutate state, even if you use a function to do it.) https://codereview.adblockplus.org/29367151/diff/29367297/edge/ext/background.js File edge/ext/background.js (right): https://codereview.adblockplus.org/29367151/diff/29367297/edge/ext/background... edge/ext/background.js:21: * Storage redirection to localforage. In Windows 10 Anniversary Update The comment looks much more useful now thanks. Could you fix the indentation though? /* * ... */ not /* * ... */ (Yes you'll need to wrap the long lines.) https://codereview.adblockplus.org/29367151/diff/29367297/edge/ext/background... edge/ext/background.js:44: }).catch(err => The indentation here for the catch is wrong. Also why not just do something like this? .catch(callback.bind(null, null)); https://codereview.adblockplus.org/29367151/diff/29367297/edge/ext/background... edge/ext/background.js:51: function internalSet(oldValue) Nit: setThenDispatch or setThenCallback or something like that? https://codereview.adblockplus.org/29367151/diff/29367297/edge/ext/background... edge/ext/background.js:57: var changes = {}; Please move the `var changes = {};` line down, so that the blank line is above it. That keeps the event dispatching logic together. https://codereview.adblockplus.org/29367151/diff/29367297/edge/ext/background... edge/ext/background.js:70: localforage.getItem(key).then(oldValue => internalSet(oldValue)) No need to wrap internalSet with another function. Also then can take a second argument for the onRejected function. How about this? localforage.getItem(key).then(internalSet, internalSet.bind(null, null)); https://codereview.adblockplus.org/29367151/diff/29367297/edge/ext/background... edge/ext/background.js:75: localforage.removeItem(key, callback); Why wrap localforage.removeItem at all? How about this? remove: localforage.removeItem,
This seems to be a whole different approach than what we did in adblockplusedgems: https://hg.adblockplus.org/adblockplusedgems/rev/5a8b109447d3 There we hooked the logic into the io module, while we now implement it in ext. I see some problems with that approach: * ext is matter to go away, now where Safari is gone, and the differences between Chrome, Edge and Firefox (WebExtensions) can be handled by other means. So I'm opposed to any addition to the abstraction layer. * As far as I understand we currently (in adblockplusedgems i.e. the code that is on the store) only store filter data in IndexedDB. If we hook into ext.storage instead we would save all data in Indexed DB. That seems unnecessary, and would require additional migration logic. Please just use the existing code we have in adblockplusedgems, and put it into edge/io.js, and replace the io module with that code when building the extension for Edge, just like we did it in the past for Opera.
On 2016/12/13 10:57:07, Sebastian Noack wrote: > Please just use the existing code we have in adblockplusedgems, and put it into > edge/io.js, and replace the io module with that code when building the extension > for Edge, just like we did it in the past for Opera. (This is exactly why I wanted to wait for Sebastian's input before continuing with the review... Oh well.)
Closing in favor of https://codereview.adblockplus.org/29367480 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
