1730706 - Remove IDBLocaleAwareKeyRange

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, task)

Description

[Henri Sivonen (:hsivonen)]

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.

Comment 1

[Anne (:annevk)]

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

Comment 2

[Henri Sivonen (:hsivonen)]

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.)

Comment 3

[Henri Sivonen (:hsivonen)]

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

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

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 5

[Oliver Medhurst [:canadahonk]]

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

Comment 6

[Oliver Medhurst [:canadahonk]]

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.

Comment 7

[Oliver Medhurst [:canadahonk]]

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

Comment 8

[Oliver Medhurst [:canadahonk]]

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.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 10

[Henri Sivonen (:hsivonen)]

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?

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 12

[Oliver Medhurst [:canadahonk]]

Attachment: Bug 1730706 - Part 1: Remove IDBLocaleAwareKeyRange

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)

Comment 13

[Oliver Medhurst [:canadahonk]]

Attachment: Bug 1730706 - Part 2: Remove all IDB locale code

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

WIP. Todo:

• Testing
• Migration (part 3?)

Do not push individually

Depends on D192064

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/872f9480eb5b
Part 1: Remove IDBLocaleAwareKeyRange r=webidl,emilio,dom-storage-reviewers,janv,smaug

Comment 15

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/872f9480eb5b

Comment 16

[Takanori MATSUURA]

Reopened for an unpublished patch.

Comment 17

[Hamish Willee]

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:

• Mark IDBLocaleAwareKeyRange as removed and deprecated.
• Markup https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB#locale-aware_sorting and anything linked by that as removed/deprecated.
• Remove or deprecate anything gated behind dom.indexedDB.experimental (depending on yor

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).

Comment 18

[Oliver Medhurst [:canadahonk]]

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.

Comment 19

[Hamish Willee]

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?

Comment 20

[Oliver Medhurst [:canadahonk]]

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!

Comment 21

[Hamish Willee]

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.

Comment 22

[Mathew Hodson]

(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?

Comment 23

[Oliver Medhurst [:canadahonk]]

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

Comment 24

[Phabricator Automation]

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.