Closed Bug 1730706 Opened 3 years ago Closed 4 months ago

Remove IDBLocaleAwareKeyRange

Categories

(Core :: Storage: IndexedDB, task)

task

Tracking

()

RESOLVED FIXED
123 Branch
Tracking Status
firefox123 --- fixed
firefox124 --- fixed

People

(Reporter: hsivonen, Assigned: canadahonk)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

According to MDN, this is a behind-flag Firefox-specific feature. It looks like we haven't made the effort to move this into a standard and not-behind-flag in 6 years.

Implementation-wise, this feature is unusual in Gecko in the sense that it uses collation key objects that even SQLite doesn't appear to use.

I've proposed that collation key objects not be part of ICU4X collator MVP.

Let's remove IDBLocaleAwareKeyRange if we aren't moving to standard and enabled-by-default.

Blocks: 1254928

FWIW, this seems reasonable to me. Once we do this I'll close https://github.com/w3c/IndexedDB/issues/38.

asuth, is this removal OK, and do you see entanglements in other features that would make this tricky to remove?

(Again, the context is getting rid of stored collation key objects in preparation for ICU4X.)

Flags: needinfo?(bugmail)

AFAICT, removing this would also remove our call to ICU's locale canonicalizer.

https://searchfox.org/mozilla-central/search?q=CanonicalizeICULevel1

(In reply to Henri Sivonen (:hsivonen) from comment #2)

asuth, is this removal OK, and do you see entanglements in other features that would make this tricky to remove?

It should be fine to remove. I think all the Firefox OS related APIs like mozContacts that used the mechanism no longer exist in-tree, and for KaiOS it looks like the mozContacts API is now implemented as a much simpler rust service.

The largest complication is that IndexedDB obviously involves a database schema. The schema v18 to v19 upgrade logic makes clear what the changes were to add locale support. The reverse operation is a little more involved than adding the column was because we can't drop a column from a table while other parts of the schema reference it. However, we can probably avoid the more involved schema change process by just dropping the indices first and then dropping the columns. It's reasonably straightforward to define the upgrade for this and this is not a situation where we would need to induce a QuotaManager sweep of all IDB databases to compel upgrades proactively; we can just do the upgrades on demand.

In general the test coverage seems quite localized to files that can just be removed, with the exception being https://searchfox.org/mozilla-central/source/dom/indexedDB/test/gtest/TestKey.cpp where the test case should be removed but the file retained.

Flags: needinfo?(bugmail)
Blocks: 1815871

MDN page for this should probably be removed once this is done, as there would be 0 implementers (and 0 on by default ever).

Keywords: dev-doc-needed

I think this should generally be about removing locale aware support from IndexedDB rather than just IDBLocaleAwareKeyRange? Since it's also an option inside IDBObjectStore.createIndex() which relies on ICU's locale canonicalizer also afaik.

:asuth, could you clarify what would actually be needed as of now to update the database schema to drop support for this? Thanks!

Flags: needinfo?(bugmail)
Summary: Remove IDBLocaleAwareKeyRange → Drop all IDB locale custom support (IDBLocaleAwareKeyRange, createIndex locale option)

Moved scope of this to generally remove our custom locale support for IDB as they depend on ICU's locale canonicalizer too afaik (see comments above). Also taking this.

Assignee: nobody → omedhurst
Status: NEW → ASSIGNED

(In reply to Oliver Medhurst [:canadahonk] from comment #7)

:asuth, could you clarify what would actually be needed as of now to update the database schema to drop support for this? Thanks!

I believe comment 4 remains accurate; is there something not covered there you're wondering about? The Workers & Storage matrix channel can be a good place to ask more general questions.

Flags: needinfo?(bugmail)

Something that occurred to me only after :canadahonk mentioned working on this:

Sort keys are invalidated by a CLDR/ICU4C upgrade. If we don't already have migration code to deal with stored sort keys becoming bogus upon CLDR/ICU4C upgrade, chances are that locale-aware IndexedDB is already broken enough that we can assume that it's not actually in use.

asuth, does the locale-aware stuff participate latently in the schema for normal on-by-default Web-exposed IndexedDB, and that's why a schema migration is needed, or is schema migration needed only for potentially-existing databases that someone might have if they've turned the relevant pref on?

Flags: needinfo?(bugmail)

(In reply to Henri Sivonen (:hsivonen) from comment #10)

Sort keys are invalidated by a CLDR/ICU4C upgrade. If we don't already have migration code to deal with stored sort keys becoming bogus upon CLDR/ICU4C upgrade, chances are that locale-aware IndexedDB is already broken enough that we can assume that it's not actually in use.

It's definitely not in use.

Looking at the docs at https://unicode-org.github.io/icu/userguide/collation/architecture.html#versioning it seems like if we were ever to implement something related to this again that we could store the ICU version information at the database level and lazily update the databases.

asuth, does the locale-aware stuff participate latently in the schema for normal on-by-default Web-exposed IndexedDB, and that's why a schema migration is needed, or is schema migration needed only for potentially-existing databases that someone might have if they've turned the relevant pref on?

The former. We use the same database schema regardless of the feature being in use primarily for complexity reasons, but it's also the case that for functionality like this (when enabled) you can't know whether it would be used when the database is first created on opening.

Flags: needinfo?(bugmail)

Unexpose and remove it, but keep locale internals
(as it is used by IDBObjectStore.createIndex() locale option)

Also remove test just for this (keep others for now)

(Attempt to) completely remove most IDB locale code and options.

WIP. Todo:

  • Testing
  • Migration (part 3?)

Do not push individually

Depends on D192064

Pushed by omedhurst@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/872f9480eb5b
Part 1: Remove IDBLocaleAwareKeyRange r=webidl,emilio,dom-storage-reviewers,janv,smaug
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 123 Branch

Reopened for an unpublished patch.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
Keywords: leave-open
Status: NEW → ASSIGNED
Attachment #9360958 - Attachment description: WIP: Bug 1730706 - Part 2: Purge IDB locale code → Bug 1730706 - Part 2: Remove all IDB locale code
Depends on: 1872675

MDN docs work for this tracked in https://github.com/mdn/content/issues/31912.

  1. My understanding is that there is nothing to do here yet because this is not "fixed" right?
  2. Do you know whether dom.indexedDB.experimental preference has always gated this and other locale feature (since FF43+)? If this has always been behind the preference then we can remove all mention of the feature. If it was hidden two years ago we can do the same thing. But if it only just was hidden we can deprecate but not "delete".

When this go in, I'd expect to:

Note that I have marked the local option to createIndex as deprecated as part of https://github.com/mdn/content/issues/31915 (https://bugzilla.mozilla.org/show_bug.cgi?id=1872675).

Flags: needinfo?(omedhurst)
  1. Right now, IDBLocaleAwareKeyRange was always gated behind dom.indexedDB.experimental, but it was removed a month ago.
  2. It has always gated IDBLocaleAwareKeyRange, however the IDBObjectStore.createIndex() locale option was never gated and released in FF43, although only implemented in Gecko. The option was deprecated in Bug 1872675, and should be removed hopefully soon.

I haven't touched the pref generally/outside of these.

Flags: needinfo?(omedhurst)

Thanks @Oliver.

I will remove IDBLocaleAwareKeyRange docs.

Is there a timeline for part 2 of this issue to be published? Specifically, will these changes be in FF123 - "yes/no/maybe"? From what I can see, from a website perspective this means removal of IDBIndex.isAutoLocale and IDBIndex.locale. This would also remove the locale option IDBObjectStore.createIndex() so I guess you're waiting on user data to do that?

Flags: needinfo?(omedhurst)

This isn't a priority for me at the moment so, 99% no. My guess is 125 at least, probably more like 126/127? I'm waiting for data on the deprecation of the locale option, which also delays it, yes. Thanks!

Flags: needinfo?(omedhurst)

Thanks very much @Oliver. More than sufficient. FYI only for FF123 I will remove IDBLocaleAwareKeyRange, mark IDBIndex.isAutoLocale and IDBIndex.locale as deprecated, and remove the info on locale specific sorting to discourage use.

(In reply to Oliver Medhurst [:canadahonk] from comment #20)

This isn't a priority for me at the moment so, 99% no. My guess is 125 at least, probably more like 126/127? I'm waiting for data on the deprecation of the locale option, which also delays it, yes. Thanks!

Since part 1 landed in Firefox 123, it would be easier to track the status if this bug was closed and the remaining patch was moved to a new bug, What do you think?

Flags: needinfo?(omedhurst)

Sorry yes, I agree this makes sense, will do.

Flags: needinfo?(omedhurst)
Summary: Drop all IDB locale custom support (IDBLocaleAwareKeyRange, createIndex locale option) → Remove IDBLocaleAwareKeyRange
Status: ASSIGNED → RESOLVED
Closed: 5 months ago4 months ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1878907
No longer blocks: 1815871

Comment on attachment 9360958 [details]
Bug 1730706 - Part 2: Remove all IDB locale code

Revision D192198 was moved to bug 1878907. Setting attachment 9360958 [details] to obsolete.

Attachment #9360958 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: