Opened 16 months ago

Closed 14 months ago

Last modified 12 months ago

#6621 closed change (fixed)

Use IndexedDB (directly) to store filter data on Microsoft Edge

Reported by: sebastian Assignee: geo
Priority: P3 Milestone: Adblock-Plus-3.3-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: oleksandr, kzar Blocked By: #6625, #6650, #6721
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29796555

Description (last modified by sebastian)

Background

There is currently only one change left in the edge branch that hasn't been addressed in master yet. We initially hoped that Microsoft would address the performance of browser.storage.local on their end so that we wouldn't need to rely on IndexedDB anymore by now. But since this didn't happen, we should prepare a more sustainable workaround, we can land in master (in order to get rid of the edge branch).

There are a couple issues with the current approach implemented in the edge branch, that prevent the change from being merged into master (as-is):

  • It patches lib/io.js, so that if we'd merge this change this would cause IndexedDB to be used everywhere, not only on Microsoft Edge. Instead we should add a separate implementation of the IO API that replaces lib/io.js in the builds for Microsoft Edge only.
  • It uses localForage, a third-party library that provides a redundant abstraction on top of IndexedDB (and other available storage backends). This was already criticized in the review back then, but we merely decided to stick with it as long as the change isn't merged into master.

What to change

  • Implement a drop-in replacement for lib/io.js that uses IndexedDB directly.
  • Integrate it with our build system, so that the current implementation of lib/io.js (using browser.storage.local) keeps being included in the Chrome and Firefox builds, while the new implementation (using IndexedDB) gets included in the builds for Microsoft Edge instead.
  • Migrate existing user's data that were stored in IndexedDB via localForage.

Change History (15)

comment:1 Changed 16 months ago by sebastian

  • Summary changed from Use IndexedDB (directly) to store filter on Microsoft Edge to Use IndexedDB (directly) to store filter data on Microsoft Edge

comment:2 Changed 16 months ago by sebastian

  • Description modified (diff)

comment:3 Changed 16 months ago by geo

  • Owner set to geo

comment:4 Changed 16 months ago by sebastian

  • Priority changed from Unknown to P3
  • Ready set

comment:5 Changed 16 months ago by kzar

  • Cc sebastian kzar added

comment:6 Changed 16 months ago by kzar

  • Cc sebastian removed

comment:7 Changed 16 months ago by sebastian

  • Blocked By 6625 added

comment:8 Changed 16 months ago by geo

Any suggestions on when to migrate the data?
1.When storage is first used?
2.When the extension starts?

comment:9 Changed 16 months ago by sebastian

For reference, on IRC, I suggested to perform the data migration as soon as the code in the script/module is executed, and then have a promise that resolves once the migration is done, while deferring IO operations until that promise is resolved. However, if somebody comes up with a simpler approach that works, fine with me too.

comment:10 Changed 16 months ago by tlucas

  • Blocked By 6650 added

comment:11 Changed 15 months ago by sebastian

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:12 Changed 15 months ago by sebastian

  • Blocked By 6721 added

comment:14 Changed 14 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:15 Changed 12 months ago by Ross

Is this in the current Edge development build? The Edge build below seems fine storage-wise, it's just difficult to tell if I'm testing the old storage or this one.

ABP Edge 0.9.11.2076
Edge 42.17134.1.0

Note: See TracTickets for help on using tickets.