Closed Bug 871846 Opened 7 years ago Closed 4 years ago

Support sorting based on specified locale in IndexedDB

Categories

(Core :: DOM: IndexedDB, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla43
blocking-b2g -
Tracking Status
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: reuben)

References

Details

(Keywords: dev-doc-complete, perf, Whiteboard: [c= p=5 s= u=])

Attachments

(7 files, 29 obsolete files)

14.28 KB, patch
Details | Diff | Splinter Review
6.89 KB, patch
sicking
: review+
Details | Diff | Splinter Review
66.56 KB, patch
reuben
: review+
Details | Diff | Splinter Review
12.62 KB, patch
sicking
: review+
Details | Diff | Splinter Review
45.59 KB, patch
Details | Diff | Splinter Review
20.68 KB, patch
Details | Diff | Splinter Review
167.35 KB, patch
janv
: review+
Details | Diff | Splinter Review
Currently ContactDB.jsm implements its own sorting infrastructure in order to support different locales.  It would be nice if IndexedDB provided an interface for specifying the locale to use in sorting when performing a query.

The above is based on discussion with gwagner.

In terms of possible implementation, the sqlite3.c file CAF currently contains the ICU extension that provides localized collation.  In theory it can be enabled by defining the SQLITE_ENABLE_ICU symbol and then you can do something like:

  SELECT icu_load_collation('pl_PL', 'POLISH');
  SELECT * FROM some_table ORDER BY name COLLATE POLISH;
Unfortunately sqlite3.c does not currently compile with SQLITE_ENABLE_ICU.
From talking with Jonas on IRC, one of the main hurdles to overcome is how to perform locale based sorting on keys encoded as blobs.  He suggested investigating if the locale information could be incorporated into the encoding step such that the resulting blob can then be sorted using strcmp().

So it looks like the sqlite ICU module won't work out-of-box for us, but might offer some clues on how to incorporate locale information into the encoding.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Whiteboard: c=
Blocks: 865747
Whiteboard: c= → c= p=5
No longer blocks: 865747
I'm still interested in this, but will not have a chance to work on it immediately.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Also, I was asked recently why we believe this to be a performance issue.

From what I understand the issue stems from ContactsAPI offering locale based sorting to its clients.  In order to implement this at the moment ContactsAPI must:

  - Load all values from IDB
  - Perform locale based sorting in memory
  - Save the resulting key order back to a separate IDB store as a cache
  - The next query then reads the key order from this cache
  - Each key is queried from IDB indvidually and sent back to client

This whole complicated set of code would go away if IDB supported locale based sorting.  This would then allow further optimizations to be more easily implemented.

Gregor, do you have any additional detail beyond that?  Any numbers from past implementations, etc?
Flags: needinfo?(anygregor)
(In reply to Ben Kelly [:bkelly] (Vacation+JSConf till June 1) from comment #4)
> Also, I was asked recently why we believe this to be a performance issue.
> 
> From what I understand the issue stems from ContactsAPI offering locale
> based sorting to its clients.  In order to implement this at the moment
> ContactsAPI must:
> 
>   - Load all values from IDB
>   - Perform locale based sorting in memory
>   - Save the resulting key order back to a separate IDB store as a cache
>   - The next query then reads the key order from this cache
>   - Each key is queried from IDB indvidually and sent back to client
> 
> This whole complicated set of code would go away if IDB supported locale
> based sorting.  This would then allow further optimizations to be more
> easily implemented.
> 
> Gregor, do you have any additional detail beyond that?  Any numbers from
> past implementations, etc?

That's about it. When we change/delete/insert a new contact we have to throw away our cached sorting-array and rebuild it.
Flags: needinfo?(anygregor)
Keywords: perf
Whiteboard: c= p=5 → c= p=5 ,
Blocks: 888666
This was discussed at the Oslo contacts app/api meeting:

  https://etherpad.mozilla.org/qwiresWpj9

The conclusion from the group was that locale based sorting IDB was a critical requirement for long term performance of contacts functionality.

Therefore I'm going to increase the priority and nom this to get it into our triage.  I'm nom'ing for koi, although 1.3 might be more realistic.  It just seems that we need to start working on this soon since it is likely to take many sprints to complete.

If possible I'd like to start looking at this in our next sprint.
blocking-b2g: --- → koi?
Priority: -- → P1
Whiteboard: c= p=5 , → [ c= p=5 u= s= ]
Yeah, there's no way that this is going to make koi.
blocking-b2g: koi? → ---
Sorry, I guess I wanted some tracking flag to get it into our triage.  I see now there is a 1.3? flag.  Let's try that.
blocking-b2g: --- → 1.3?
Depends on: 915735
Blocks: 909935
Clearing 1.3? flag. Tracking this in our FxOS Perf backlog http://scrumbu.gs/p/fxos-perf/
blocking-b2g: 1.3? → -
Priority: P1 → --
Whiteboard: [ c= p=5 u= s= ] → [c= p=5 s= u=]
Ben T,

Ben Kelly mentioned that as of our Oslo Workweek you may be looking into this or related work. Can you provide any more info on that?

FxOS Perf Triage
Flags: needinfo?(bent.mozilla)
Priority: -- → P3
This bug was basically on hold until bug 915735 got sorted out (which took far longer than we'd hoped). Now we should be free to proceed here.

In the meantime my priority list got shuffled a bit and I'm not going to get around to this at least until mid-Q1, maybe later. We should find someone to get it sooner.
Flags: needinfo?(bent.mozilla)
Depends on: 864843, 866301
Attached patch Mochitest Patch (obsolete) — Splinter Review
Assignee: nobody → szheng
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
What's the spec status on this?  I saw Jonas mention that we wanted this in http://lists.w3.org/Archives/Public/public-webapps/2014AprJun/0177.html but there doesn't seem to be much else about it on the list and neither the latest editor's draft or the bugzilla at https://www.w3.org/Bugs/Public/buglist.cgi?product=WebAppsWG&component=Indexed%20Database%20API seem to have coverage.
Attached patch Mochitest Patch (obsolete) — Splinter Review
Attachment #8454935 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #8454936 - Attachment is obsolete: true
Attachment #8460678 - Flags: review?(bent.mozilla)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8460679 - Attachment is obsolete: true
Attachment #8463467 - Flags: review?(bent.mozilla)
Comment on attachment 8463467 [details] [diff] [review]
Patch

Review of attachment 8463467 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should take a different direction with respect to keys, but the SQL and ICU stuff look great!

::: dom/bindings/moz.build
@@ +61,5 @@
>      '/dom/xbl',
>      '/dom/xslt/base',
>      '/dom/xslt/xpath',
> +    '/intl/icu/source/common',
> +    '/intl/icu/source/i18n',

I'm not sure why the changes here (and in all the other moz.build files you modified) are necessary. What requires this?

::: dom/indexedDB/IDBFactory.cpp
@@ +446,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Load index information
>    rv = aConnection->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT object_store_id, id, name, key_path, unique_index, multientry, locale "

Nit: Please wrap this to 80 chars.

@@ +494,5 @@
>  
> +  // Update locale-aware indexes if necessary
> +  nsCString locale = IndexedDatabaseManager::GetLocale();
> +
> +  if (!locale.IsEmpty()) {

Hopefully this will never be empty...

@@ +859,5 @@
> +                                   const nsACString& aLocale)
> +{
> +  nsresult rv;
> +  mozStorageTransaction transaction(aConnection, false,
> +                                    mozIStorageConnection::TRANSACTION_IMMEDIATE);

Rather than have one transaction per index I think you want just one for all indexes, so move this outside the for-loop in LoadDatabaseInformation.

@@ +869,5 @@
> +  else {
> +    indexTable.AssignLiteral("index_data");
> +  }
> +
> +  nsCString readQuery = NS_LITERAL_CSTRING("SELECT value, object_data_key FROM ") +

I think you can make this faster (and avoid all the conditions in the UPDATE query) if you select the rowid here too. Then you can update each row directly rather than trying to match other conditions.

@@ +872,5 @@
> +
> +  nsCString readQuery = NS_LITERAL_CSTRING("SELECT value, object_data_key FROM ") +
> +                        indexTable +
> +                        NS_LITERAL_CSTRING(" WHERE index_id = :index_id");
> +  nsCOMPtr<mozIStorageStatement> stmt;

Nit: Let's call this |readStmt|.

@@ +886,5 @@
> +                                            "WHERE index_id = :index_id AND "
> +                                            "value = :value AND "
> +                                            "object_data_key = :object_data_key");
> +  nsCOMPtr<mozIStorageStatement> writeStmt;
> +  rv = aConnection->CreateStatement(writeQuery, getter_AddRefs(writeStmt));

You don't need to create this statement unless there are rows, so try to do this lazily.

@@ +891,5 @@
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +  bool hasResult;
> +  while (NS_SUCCEEDED((rv = stmt->ExecuteStep(&hasResult))) && hasResult) {
> +    rv = writeStmt->BindInt64ByName(NS_LITERAL_CSTRING("index_id"),

Hrm, I think you need a mozStorageStatementScoper inside this loop so that the statement is reset appropriately each time you go through.

::: dom/indexedDB/IDBFactory.h
@@ +13,5 @@
>  #include "mozilla/dom/quota/StoragePrivilege.h"
>  #include "nsCOMPtr.h"
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsWrapperCache.h"
> +#include "DatabaseInfo.h"

This should not be needed, just forward-declare IndexInfo

::: dom/indexedDB/IDBIndex.h
@@ +18,5 @@
>  #include "mozilla/dom/indexedDB/IDBObjectStore.h"
>  #include "mozilla/dom/indexedDB/IDBRequest.h"
>  #include "mozilla/dom/indexedDB/KeyPath.h"
>  
> +#include "unicode/locid.h"

Ah, I think this is why you had to add the LOCAL_INCLUDES thing everywhere. The easy way to fix this is to just remove the locale property from the index. At least for now there's no way that this property will ever return something different than the |navigator.languages| property.

::: dom/indexedDB/IDBKeyRange.cpp
@@ +288,5 @@
>    if (aRv.Failed()) {
>      return nullptr;
>    }
>  
> +  if (!IndexedDatabaseManager::IsExperimentalPref() &&

We'll need some kind of followup bug to handle this better.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +46,5 @@
>  #include "mozilla/dom/IDBRequestBinding.h"
>  #include "mozilla/dom/IDBTransactionBinding.h"
>  #include "mozilla/dom/IDBVersionChangeEventBinding.h"
>  
> +#include "unicode/locid.h"

This needs an #ifdef

@@ +290,5 @@
> +    Preferences::GetLocalizedCString("intl.accept_languages");
> +
> +  // Split values on commas.
> +  nsCCharSeparatedTokenizer langTokenizer(acceptLang, ',');
> +  if (langTokenizer.hasMoreTokens()) {

We need a fallback here in case the prefs are screwed up somehow and there are no locales listed. English, I guess.

@@ +292,5 @@
> +  // Split values on commas.
> +  nsCCharSeparatedTokenizer langTokenizer(acceptLang, ',');
> +  if (langTokenizer.hasMoreTokens()) {
> +    nsDependentCSubstring lang = langTokenizer.nextToken();
> +    mLocale = lang;

I wouldn't assign to mLocale here since you're just going to reassign it below. Instead just make a nsAutoCString to hold the locale prior to testing.

@@ +661,5 @@
>  
>    return NS_OK;
>  }
>  
> +const nsCString IndexedDatabaseManager::sTestLocale = NS_LITERAL_CSTRING("es_ES");

We can't have static string objects like this unfortunately for a variety of reasons. However, I think you don't need it. Just set a member variable in Init to this "es_ES".

@@ +664,5 @@
>  
> +const nsCString IndexedDatabaseManager::sTestLocale = NS_LITERAL_CSTRING("es_ES");
> +
> +const nsCString&
> +IndexedDatabaseManager::GetLocale()

Nit: Add a |// static| comment before each of these static methods.

::: dom/indexedDB/Key.cpp
@@ +14,5 @@
>  #include "nsJSUtils.h"
>  #include "xpcpublic.h"
>  #include "mozilla/Endian.h"
> +#include "unicode/sortkey.h"
> +#include "unicode/locid.h"

These should be #ifdef ENABLE_INTL_API

@@ +319,5 @@
> +
> +void
> +Key::EncodeLocaleString(const nsAString& aString, uint8_t aTypeOffset)
> +{
> +#ifdef ENABLE_INTL_API

It might be simpler to add a |using namespace icu;| here. Then you wouldn't have to prefix everything in this method.

@@ +324,5 @@
> +  icu::UnicodeString ustr;
> +  ustr.setTo((UChar *) PromiseFlatString(aString).get(), aString.Length());
> +  UErrorCode success = U_ZERO_ERROR;
> +  icu::Locale locale = icu::Locale::createCanonical(mLocaleString.get());
> +  icu::Collator* collator = icu::Collator::createInstance(locale, success);

Can this fail? You're not checking |success| anywhere...

Also, this means that we're going to be creating and destroying a lot of these collator objects. Can we cache one somewhere?

@@ +326,5 @@
> +  UErrorCode success = U_ZERO_ERROR;
> +  icu::Locale locale = icu::Locale::createCanonical(mLocaleString.get());
> +  icu::Collator* collator = icu::Collator::createInstance(locale, success);
> +  icu::CollationKey key;
> +  collator->getCollationKey(ustr, key, success);

Here too.

@@ +328,5 @@
> +  icu::Collator* collator = icu::Collator::createInstance(locale, success);
> +  icu::CollationKey key;
> +  collator->getCollationKey(ustr, key, success);
> +  if (collator) {
> +    delete collator;

|collator| should be put into nsAutoPtr to ensure that it always gets cleaned up.

@@ +332,5 @@
> +    delete collator;
> +  }
> +
> +  int32_t size;
> +  const uint8_t* keyBuffer = key.getByteArray(size);

Are there shortcuts you can take if size is 0?

@@ +335,5 @@
> +  int32_t size;
> +  const uint8_t* keyBuffer = key.getByteArray(size);
> +
> +  // First measure how long the encoded string will be.
> +  const uint8_t* end = keyBuffer + size;

This is all duplicated from the original EncodeString method. We should try to refactor this so that the two Encode methods use the same start/finish and do slightly different stuff in the middle.

@@ +365,5 @@
> +      *(buffer++) = *iter + ONE_BYTE_ADJUST;
> +    }
> +    else {
> +      *(buffer++) = (char)(0xFF);
> +      *(buffer++) = (char)(*iter - SHORT_CHAR_ONE_BYTE_LIMIT);

Nit: No need for the (char) casts, this is just integer math.

@@ +505,5 @@
> +    char* buffer;
> +    uint32_t oldLen = mLocaleKeyBuffer.Length();
> +    // Copy array TypeID from raw key
> +    if (*it % eMaxType == 0) {
> +      mLocaleKeyBuffer.GetMutableData(&buffer, oldLen + 1);

GetMutableData can fail...

@@ +515,5 @@
> +      buffer += oldLen;
> +      *(buffer++) = *(it++);
> +
> +      for (uint32_t i = 0; i < sizeof(uint64_t); i++) {
> +        *(buffer++) = it < end ? *(it++) : 0;

It shouldn't be possible for |it >= end| here, right? Could that be asserted?

@@ +522,5 @@
> +    // Decode string and reencode
> +    else {
> +      nsString str;
> +      uint8_t typeOffset = *it - eString;
> +      DecodeString(it, end, str);

It seems wasteful to have to encode a key and then immediately turn around and decode part of it so that we can re-encode it in a different way. Having separate local-aware and non-locale-aware keys is looking a lot simpler...

::: dom/indexedDB/Key.h
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef mozilla_dom_indexeddb_key_h__

The changes to this file are pretty substantial... Rather than making they Key class contain both a normal buffer and a locale-aware buffer wouldn't it be simpler to just use two separate Key objects whenever we need the locale-aware variant? Let's chat about this when I get back.

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ -236,5 @@
>        "name TEXT NOT NULL, "
>        "key_path TEXT NOT NULL, "
>        "unique_index INTEGER NOT NULL, "
>        "multientry INTEGER NOT NULL, "
> -      "UNIQUE (object_store_id, name), "

Hrm, why did you remove this constraint?

@@ +248,5 @@
>    rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>      "CREATE TABLE index_data ("
>        "index_id INTEGER NOT NULL, "
>        "value BLOB NOT NULL, "
> +      "value_locale BLOB, "

Let's make sure this comes after object_data_id, otherwise sqlite has to read this out any time we want to get object_data_key/object_data_id. In fact, since the ALTER command always sticks the column at the end let's just put it there.

Do the same for |unique_index_data|.

@@ +269,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "CREATE INDEX index_data_value_locale_index "
> +    "ON index_data (index_id, value_locale, object_data_key) "

Jonas pointed out today that you must also have |value| at the end here otherwise the index won't cover the field you need for cursors. This is important since sqlite would have to do a join without this. The query planner should show this too.

Do the same for |unique_index_data_value_locale_index|, and you'll need to update the upgrade function too.

@@ +1487,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +UpgradeSchemaFrom15_0To16_0(mozIStorageConnection* aConnection)

This will need to be adjusted (16->17) now (someone added 16 already).

@@ +1498,5 @@
> +    js::ProfileEntry::Category::STORAGE);
> +
> +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "ALTER TABLE object_store_index"
> +      "ADD COLUMN locale TEXT;"

Eek! This doesn't work. You have no spaces between the table name and the ADD command so SQLite won't accept this. You've got the same problem in several other places below. This code needs to be tested!

@@ +1516,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "CREATE INDEX index_data_value_locale_index "
> +    "ON index_data (value_locale, object_data_key) "

Remember to add the |value| column here, and below.

::: dom/indexedDB/ipc/SerializationHelpers.h
@@ +25,3 @@
>  
>  template <>
>  struct ParamTraits<mozilla::dom::indexedDB::Key>

I'd rather we didn't have too many changes to Key in general so hopefully we won't need these either.

::: dom/webidl/IDBIndex.webidl
@@ +21,5 @@
>      readonly    attribute any            keyPath;
>  
>      readonly    attribute boolean        multiEntry;
>      readonly    attribute boolean        unique;
> +    readonly    attribute DOMString      localeAware;

'localeAware' should be a boolean to match the dictionary. It should be guarded by the experimental pref though.
Attachment #8463467 - Flags: review?(bent.mozilla) → review-
Comment on attachment 8460678 [details] [diff] [review]
Mochitest Patch

Review of attachment 8460678 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/test/test_create_locale_aware_index.html
@@ +8,5 @@
> +
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +
> +  <script type="text/javascript;version=1.7" src="unit/test_create_locale_aware_index.js"></script>

This test doesn't really do anything interesting... It just creates indexes with the localeAware property set to true, and that's it. Your other tests will also test this same functionality, so I think we can just remove this one.
Attachment #8460678 - Flags: review?(bent.mozilla) → review+
Is there documentation in the design decisions here?

I'm in particular wondering about:

- collations for searching and ordering differ in quite a few languages, so sorting and range queries could depend on different collations

- collations for contacts and shopping lists differ in quite a few languages (look for <collation type="phonebook"> in CLDRs collation data)

- locale selection. Should sorting be done via OS locale, UI locale, document locale, preferred web content language?
(In reply to Axel Hecht [:Pike] from comment #21)
> Is there documentation in the design decisions here?
> 
> I'm in particular wondering about:
> 
> - collations for searching and ordering differ in quite a few languages, so
> sorting and range queries could depend on different collations
> 
> - collations for contacts and shopping lists differ in quite a few languages
> (look for <collation type="phonebook"> in CLDRs collation data)
> 
> - locale selection. Should sorting be done via OS locale, UI locale,
> document locale, preferred web content language?

No docs unfortunately. Currently, we use the language on the preference, the same as nagivator.language. If locale is changed, the index is to be reconstructed. It is impossible to have illegal queries.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8463467 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #8466829 - Attachment is obsolete: true
Attachment #8467308 - Flags: review?(bent.mozilla)
ping..
Any update?
Flags: needinfo?(bent.mozilla)
Hi Shihua! Sorry for the delay here. We've got someone scheduled to merge all your changes into the reworked IndexedDB code really soon!
Flags: needinfo?(bent.mozilla)
Attachment #8467308 - Attachment is obsolete: true
Attachment #8467308 - Flags: review?(bent.mozilla)
Attached patch Part 2. API changes (obsolete) — Splinter Review
Attached patch Part 4. Database changes (obsolete) — Splinter Review
Attached patch Part 5. Tests, rebased. (obsolete) — Splinter Review
Carrying r=bent
Attachment #8554939 - Flags: review+
Attachment #8460678 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b09bb1107cab

After bug 866301 lands this should work on Gonk platforms, at least on JB+. I still have to check what happens on ICS.
I ran into a problem, the spec has language that is incompatible with these changes: http://w3c.github.io/IndexedDB/#widl-IDBKeyRange-bound-IDBKeyRange-any-lower-any-upper-boolean-lowerOpen-boolean-upperOpen

    If [...] the lower key is greater than the upper key [...] the implementation MUST throw a DOMException of type DataError. 

With locale-aware indexes, a key being greater than another only has any meaning after we know what index the IDBKeyRange is going to be applied to. Locale-aware IDBKeyRange would make for a terrible API, so I propose moving that check to a point where we know what "greater than" means for keys.
Flags: needinfo?(bent.mozilla)
Last time sicking and I discussed this I think the plan was to make a new IDBLocaleKeyRange object (that works just like IDBKeyRange), and requiring that any key range passed to a locale-aware index (via get/openCursor/etc.) must be an IDBLocaleKeyRange. We would throw if you passed a normal IDBKeyRange.
Flags: needinfo?(bent.mozilla)
Attached patch 1. ICU and infrastructure bits (obsolete) — Splinter Review
Attachment #8554934 - Attachment is obsolete: true
Attached patch 2. API changes (obsolete) — Splinter Review
Attachment #8554936 - Attachment is obsolete: true
Attachment #8554938 - Attachment is obsolete: true
Attachment #8554937 - Attachment is obsolete: true
Attached patch 5. IDBLocaleAwareKeyRange (obsolete) — Splinter Review
Attachment #8554939 - Attachment is obsolete: true
Attached patch 1. ICU and infrastructure bits (obsolete) — Splinter Review
Attachment #8565573 - Attachment is obsolete: true
Attachment #8565680 - Flags: review?(bent.mozilla)
Attachment #8565574 - Flags: review?(bent.mozilla)
Attachment #8565576 - Flags: review?(bent.mozilla)
Attachment #8565577 - Flags: review?(bent.mozilla)
Attached patch 5. IDBLocaleAwareKeyRange (obsolete) — Splinter Review
Attachment #8565579 - Attachment is obsolete: true
Attachment #8565684 - Flags: review?(bent.mozilla)
Comment on attachment 8565680 [details] [diff] [review]
1. ICU and infrastructure bits

Review of attachment 8565680 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this Reuben! Sorry for the delay, but I'm finally done with all the other crap I was stuck with :)

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +29,5 @@
>  #include "mozilla/dom/quota/Utilities.h"
>  #include "mozilla/dom/TabContext.h"
>  #include "mozilla/ipc/BackgroundChild.h"
>  #include "mozilla/ipc/PBackgroundChild.h"
> +#include "nsCharSeparatedTokenizer.h"

Nit: Move this into the #ifdef ENABLE_INTL_API block below.

@@ +57,5 @@
>  #include "mozilla/dom/IDBTransactionBinding.h"
>  #include "mozilla/dom/IDBVersionChangeEventBinding.h"
>  
> +#ifdef ENABLE_INTL_API
> +  #include "unicode/locid.h"

Nit: No indenting

@@ +136,5 @@
>  const char kPrefLoggingProfiler[] =
>    IDB_PREF_LOGGING_BRANCH_ROOT "profiler-marks";
>  #endif
>  
> +const char kPrefOverrideLocale[] = IDB_PREF_BRANCH_ROOT "test.overrideLocale";

We've since started using |kTestingPref| that uses the string "testing" instead of "test" so please update this to use the new branch.

@@ +350,5 @@
> +    Preferences::GetLocalizedCString("intl.accept_languages");
> +
> +  // Split values on commas.
> +  nsCCharSeparatedTokenizer langTokenizer(acceptLang, ',');
> +  if (langTokenizer.hasMoreTokens()) {

It seems to me that this should be done in a loop, and we should take the first one that passes the |!isBogus()| test. Only if we get through them all and none pass that test should we fall back to "en-US".

@@ +352,5 @@
> +  // Split values on commas.
> +  nsCCharSeparatedTokenizer langTokenizer(acceptLang, ',');
> +  if (langTokenizer.hasMoreTokens()) {
> +    nsCString lang;
> +    lang.Assign(langTokenizer.nextToken());

Let's make this:

  nsAutoCString lang(langTokenizer.nextToken());

That way we won't have to allocate every time.

@@ +355,5 @@
> +    nsCString lang;
> +    lang.Assign(langTokenizer.nextToken());
> +    icu::Locale locale = icu::Locale::createCanonical(lang.get());
> +    if (locale.isBogus()) {
> +      mLocale.AssignASCII("en-US");

AssignLiteral

@@ +357,5 @@
> +    icu::Locale locale = icu::Locale::createCanonical(lang.get());
> +    if (locale.isBogus()) {
> +      mLocale.AssignASCII("en-US");
> +    } else {
> +      mLocale.AssignASCII(locale.getBaseName());

Are we guaranteed that this is ASCII?

@@ +366,5 @@
> +#endif
> +
> +
> +  Preferences::RegisterCallback(OverrideLocalePrefChangedCallback,
> +                                kPrefOverrideLocale);

This should be inside the #ifdef too

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +148,5 @@
>                              int32_t* aSliceRefCnt,
>                              bool* aResult);
>  
> +  static nsCString
> +  GetLocale();

Let's make this return |const nsCString&|. Then it can, depending on the value of the override bool, return either mLocale or a new mOverrideLocale member. That way we don't create new nsCString objects every time someone calls this method (which, given that we call it in the key encoding path, could be pretty frequent).

And all of these members and functions should be #ifdef ENABLE_INTL_API

@@ +151,5 @@
> +  static nsCString
> +  GetLocale();
> +
> +  static void
> +  OverrideLocalePrefChangedCallback(const char*, void*);

This can now be removed, just use AtomicBoolPrefChangedCallback in IndexedDatabaseManager.cpp exactly like we do for the other bool prefs.

@@ +154,5 @@
> +  static void
> +  OverrideLocalePrefChangedCallback(const char*, void*);
> +
> +  static bool
> +  IsExperimentalPref();

We now have |IndexedDatabaseManager::ExperimentalFeaturesEnabled| so this shouldn't be necessary any more.

@@ +201,5 @@
>    mozilla::Mutex mFileMutex;
>  
> +  nsCString mLocale;
> +  bool mOverrideLocale;
> +  bool mExperimentalPref;

These two bools shouldn't be needed any more.

::: dom/indexedDB/Key.cpp
@@ +113,5 @@
>  
>  nsresult
> +Key::ToLocaleBasedKey(Key& aTarget) const
> +{
> +  if (IsUnset()) {

Let's add shortcuts for other types that we can just immediately set equal to mBuffer and share the data, like numbers and dates.

In fact, it would be really great to avoid copying anything if we don't have a key that contains a string. Maybe start in a "reading" phase that just checks for existence of a string, then if you find a string you switch to the "copy" phase and copy all the data you've already read to a new buffer and then do the fancy new encoding stuff.

@@ +118,5 @@
> +    aTarget.Unset();
> +    return NS_OK;
> +  }
> +
> +  aTarget.mBuffer.Truncate();

I think you should probably go ahead and call SetCapacity here to the length of mBuffer to prevent lots of little mallocs below. It's as good a guess as any.

@@ +122,5 @@
> +  aTarget.mBuffer.Truncate();
> +  const unsigned char* it = reinterpret_cast<const unsigned char*> (
> +                                             mBuffer.BeginReading());
> +  const unsigned char* end = reinterpret_cast<const unsigned char*> (
> +                                             mBuffer.EndReading());

Nit: |auto* foo = reinterpret_cast<...>(...);|

@@ +128,5 @@
> +
> +  while (it < end) {
> +    char* buffer;
> +    uint32_t oldLen = aTarget.mBuffer.Length();
> +    // Copy array TypeID from raw key

Nit: This branch will handle eArray and eTerminator, not just arrays.

@@ +135,5 @@
> +        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +      }
> +      *(buffer + oldLen) = *(it++);
> +    }
> +    // Copy number from raw key

Nit: Move these comments inside the braces so you can cuddle the |} else if (...) {|

@@ +136,5 @@
> +      }
> +      *(buffer + oldLen) = *(it++);
> +    }
> +    // Copy number from raw key
> +    else if (*it % eMaxType <= eDate) {

I would test the types explicitly:

  uint8_t type = *it % eMaxType;
  // ...
  else if (type == eFloat || type == eDate) {
  // ...

Some day we're going to add more types and we don't want this code to silently break. If we do this then we can assert below.

@@ +145,5 @@
> +      buffer += oldLen;
> +      *(buffer++) = *(it++);
> +
> +      for (uint32_t i = 0; i < sizeof(uint64_t); i++) {
> +        MOZ_ASSERT(it <= end);

Hm, I think this is not assertable... This loop should probably be:

  const size_t byteCount = std::min(sizeof(uint64_t), size_t(end - it));
  for (size_t count = 0; count < byteCount; count++);
    *(buffer++) = *(it++);
  }

@@ +152,5 @@
> +    }
> +    // Decode string and reencode
> +    else {
> +      nsString str;
> +      uint8_t typeOffset = *it - eString;

MOZ_ASSERT((typeOffset % eArray == 0) && (typeOffset / eArray <= 2));

This is how we will find problems if we ever add a new key type.

@@ +381,5 @@
>    
>    NS_ASSERTION(buffer == mBuffer.EndReading(), "Wrote wrong number of bytes");
>  }
>  
> +void

This can fail for OOM and if ICU does something funny, so better make it return nsresult and pass that through to the caller.

@@ +382,5 @@
>    NS_ASSERTION(buffer == mBuffer.EndReading(), "Wrote wrong number of bytes");
>  }
>  
> +void
> +Key::EncodeLocaleString(const nsAString& aString, uint8_t aTypeOffset,

This should probably be changed to |const nsDependentString& aString|

@@ +386,5 @@
> +Key::EncodeLocaleString(const nsAString& aString, uint8_t aTypeOffset,
> +                        const nsCString& aLocale)
> +{
> +#ifdef ENABLE_INTL_API
> +  const int length = aString.Length();

We need a fast return for empty strings to avoid setting up all this icu stuff for no reason.

@@ +387,5 @@
> +                        const nsCString& aLocale)
> +{
> +#ifdef ENABLE_INTL_API
> +  const int length = aString.Length();
> +  const UChar* ustr = (const UChar*)PromiseFlatString(aString).get();

This isn't safe, the buffer returned by PromiseFlatString.get() is only valid as long as the PromiseFlatString object is alive.

But I think you don't need to do this anyway as long as you pass the length to |ucol_getSortKey()|. Just pass |reinterpret_cast<const UChar*>(aString.BeginReading())| and |aString.Length()| directly below.

@@ +389,5 @@
> +#ifdef ENABLE_INTL_API
> +  const int length = aString.Length();
> +  const UChar* ustr = (const UChar*)PromiseFlatString(aString).get();
> +
> +  UErrorCode status = U_ZERO_ERROR;

Nit: s/status/uerror/

@@ +391,5 @@
> +  const UChar* ustr = (const UChar*)PromiseFlatString(aString).get();
> +
> +  UErrorCode status = U_ZERO_ERROR;
> +  UCollator* collator = ucol_open(aLocale.get(), &status);
> +  if (U_FAILURE(status)) {

NS_WARN_IF

@@ +396,5 @@
> +    return;
> +  }
> +  MOZ_ASSERT(collator);
> +
> +  int32_t size = ucol_getSortKey(collator, ustr, length, nullptr, 0);

Nit: s/size/sortKeyLength/

@@ +398,5 @@
> +  MOZ_ASSERT(collator);
> +
> +  int32_t size = ucol_getSortKey(collator, ustr, length, nullptr, 0);
> +  nsAutoArrayPtr<uint8_t> keyBuffer(new uint8_t[size]);
> +  size = ucol_getSortKey(collator, ustr, length, keyBuffer, size);

I think we should rework this to use stack space and avoid allocating if possible. See http://userguide.icu-project.org/collation/api#TOC-Sort-Key-Output-Buffer for the basic idea.

We could use nsAutoTArray<uint8_t, 128> or something.

@@ +400,5 @@
> +  int32_t size = ucol_getSortKey(collator, ustr, length, nullptr, 0);
> +  nsAutoArrayPtr<uint8_t> keyBuffer(new uint8_t[size]);
> +  size = ucol_getSortKey(collator, ustr, length, keyBuffer, size);
> +  ucol_close(collator);
> +  if (length != 0 && size <= 0) {

NS_WARN_IF

If we fix empty string handling above then |length != 0| shouldn't be possible here.

@@ +407,5 @@
> +
> +  // First measure how long the encoded string will be.
> +  const uint8_t* end = keyBuffer + size;
> +
> +  // The +2 is for initial 3 and trailing 0. We'll compensate for multi-byte

All of this code needs to be refactored to reuse the guts of Key::EncodeString. (It's all basically copied)

::: dom/indexedDB/Key.h
@@ +188,5 @@
>  
>    nsresult
>    AppendItem(JSContext* aCx, bool aFirstOfArray, JS::Handle<JS::Value> aVal);
>  
> +  nsresult ToLocaleBasedKey(Key& aTarget) const;

All of these should be #ifdef ENABLE_INTL_API

Nit: return type on its own line.

::: dom/indexedDB/moz.build
@@ +95,5 @@
>      '/dom/base',
>      '/dom/storage',
>      '/dom/workers',
> +    '/intl/icu/source/common',
> +    '/intl/icu/source/i18n',

Rather than this I think you should try to copy this: http://mxr.mozilla.org/mozilla-central/source/layout/forms/moz.build#60
Attachment #8565680 - Flags: review?(bent.mozilla) → review-
Comment on attachment 8565574 [details] [diff] [review]
2. API changes

Review of attachment 8565574 [details] [diff] [review]:
-----------------------------------------------------------------

So after talking with Jonas more I think we want something a little different here. Instead of having a simple binary 'localeAware' boolean we want something more flexible like this:

  dictionary IDBIndexParameters {
    // ...

    // <null>:   Not locale-aware, uses normal JS sorting.
    // <string>: Always sorted based on the rules of the specified
    //           locale (e.g. "en-US", etc.).
    // "auto":   Sorted by the platform default, may change based on
    //           user agent options.
    DOMString locale = null;
  };

  interface IDBIndex {
    // ...

    // <null>:   Not locale-aware, uses normal JS sorting.
    // <string>: Sorted based on the rules of the specified locale.
    //           Note: never returns "auto", only the current locale.
    [Func="..."]
    readonly attribute DOMString locale;
  }

The special "rebuild on demand" behavior will only apply to "auto" locale indexes. If a single locale is specified then we will never need to rebuild it and we always sort by that locale's rules.

::: dom/indexedDB/IDBCursor.cpp
@@ +86,5 @@
>  
>  // static
>  already_AddRefed<IDBCursor>
>  IDBCursor::Create(BackgroundCursorChild* aBackgroundActor,
> +                  const Key& aKey)

I'd rather you didn't change the order of these functions.

::: dom/indexedDB/IDBCursor.h
@@ +75,5 @@
>    JS::Heap<JS::Value> mCachedPrimaryKey;
>    JS::Heap<JS::Value> mCachedValue;
>  
>    Key mKey;
> +  Key mOriginalKey;

See later comment about renaming these in PBackgroundIDBCursor.ipdl

@@ +88,5 @@
>    bool mHaveCachedValue : 1;
>    bool mRooted : 1;
>    bool mContinueCalled : 1;
>    bool mHaveValue : 1;
> +  bool mLocaleAware : 1;

Shouldn't be needed, just check for unset-ness of sort key.

@@ +96,5 @@
>    Create(BackgroundCursorChild* aBackgroundActor,
> +         const Key& aKey);
> +
> +  static already_AddRefed<IDBCursor>
> +  Create(BackgroundCursorChild* aBackgroundActor,

Nit: I'd rather you didn't change the order here

@@ +106,5 @@
>           const Key& aKey,
>           const Key& aPrimaryKey,
> +         StructuredCloneReadInfo&& aCloneInfo,
> +         bool aLocaleAware,
> +         const Key& aOriginalKey);

Let's have the order be 3 keys then the clone info. And no need for the bool here or below.

@@ +172,5 @@
>    void
>    Reset(Key&& aKey);
>  
>    void
> +  Reset(Key&& aKey, Key&& aPrimaryKey, StructuredCloneReadInfo&& aValue, bool aLocaleAware, Key&& aOriginalKey);

Same here, 3 keys, then the clone info. No bools.

@@ +197,5 @@
>    IDBCursor(Type aType,
>              BackgroundCursorChild* aBackgroundActor,
>              const Key& aKey);
>  
> +  void SetLocaleAware(const Key& aOriginalKey);

You don't really need a function for this, you can set the key directly in the Cursor::Create function.

::: dom/indexedDB/IDBIndex.cpp
@@ +251,5 @@
>    }
>  
>    const int64_t objectStoreId = mObjectStore->Id();
>    const int64_t indexId = Id();
> +  

Nit: whitespace

@@ +254,5 @@
>    const int64_t indexId = Id();
> +  
> +  SerializedKeyRange serializedKeyRange;
> +  keyRange->ToSerialized(serializedKeyRange);
> +  OptionalKeyRange optionalKeyRange = serializedKeyRange;

No need for the |optionalKeyRange| here, just pass |serializedKeyRange| directly to the IndexGetKeyParams/IndexGetParams constructor below.

::: dom/indexedDB/IDBIndex.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_indexeddb_idbindex_h__
>  #define mozilla_dom_indexeddb_idbindex_h__
>  
> +#include "IndexedDatabaseManager.h"

Not needed I think?

::: dom/indexedDB/PBackgroundIDBCursor.ipdl
@@ +49,5 @@
>  
>  struct IndexCursorResponse
>  {
>    Key key;
> +  Key originalKey;

We should name this something more intuitive... What if we kinda reversed this and kept 'key' as the one that JS sees and made 'sortKey' for the one that has the ICU sort key inside it?

@@ +54,2 @@
>    Key objectKey;
> +  bool localeAware;

This shouldn't be needed, just use the unset-ness of the sort key to tell you if locale stuff is on.

@@ +63,2 @@
>    Key objectKey;
> +  bool localeAware;

Here too.

::: dom/indexedDB/PBackgroundIDBSharedTypes.ipdlh
@@ +86,5 @@
>    nsString name;
>    KeyPath keyPath;
>    bool unique;
>    bool multiEntry;
> +  bool localeAware;

I don't think we need a separate bool here, you can just make |locale| be a void string if we're not using locales.

@@ +87,5 @@
>    KeyPath keyPath;
>    bool unique;
>    bool multiEntry;
> +  bool localeAware;
> +  nsString locale;

Nit: Put |locale| after |keyPath| for better packing.
Attachment #8565574 - Flags: review?(bent.mozilla) → review-
Attachment #8565576 - Flags: review?(bent.mozilla)
Attachment #8565577 - Flags: review?(bent.mozilla)
Attachment #8565684 - Flags: review?(bent.mozilla)
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #44)
> So after talking with Jonas more I think we want something a little
> different here. Instead of having a simple binary 'localeAware' boolean we
> want something more flexible like this:
> 
>   dictionary IDBIndexParameters {
>     // ...
> 
>     // <null>:   Not locale-aware, uses normal JS sorting.
>     // <string>: Always sorted based on the rules of the specified
>     //           locale (e.g. "en-US", etc.).
>     // "auto":   Sorted by the platform default, may change based on
>     //           user agent options.
>     DOMString locale = null;
>   };
> 
>   interface IDBIndex {
>     // ...
> 
>     // <null>:   Not locale-aware, uses normal JS sorting.
>     // <string>: Sorted based on the rules of the specified locale.
>     //           Note: never returns "auto", only the current locale.
>     [Func="..."]
>     readonly attribute DOMString locale;
>   }
> 
> The special "rebuild on demand" behavior will only apply to "auto" locale
> indexes. If a single locale is specified then we will never need to rebuild
> it and we always sort by that locale's rules.

Did you discuss supporting different collation types? With this API, we could very easily expose this feature, but do we actually want to do that? (I think we should). It would look like this:

   store.createIndex("foo", "bar", {locale: "de@collation=phonebook"});

The full list of options is here http://unicode.org/repos/cldr/trunk/common/bcp47/collation.xml
Flags: needinfo?(jonas)
Flags: needinfo?(bent.mozilla)
Sounds good to me. The only issue I think is what syntax to use.

Seems like it would be useful to also allow selecting a collation type, but still use the users default language.
Flags: needinfo?(jonas)
"locale@keyword=value" is what ICU understands, I think it's good enough. And something like "auto@collation=phonebook" should work for your suggested scenario.

Thanks for the quick reply.
Flags: needinfo?(bent.mozilla)
If that's defined behavior elsewhere, then that sounds great to me.
Hey Reuben, any update here? I'd love to get a feel for the progress you've made.
Flags: needinfo?(reuben.bmo)
Hi Ben! I've addressed your comments and have the API changes working, now I'm fixing the DB code. It's taking me a while because it took me a week to track down an off-by-one bug on the ICU encoding code and now I'm rebasing my changes on top of bug 1131776. I expect to have a working patch set by next week.
Flags: needinfo?(reuben.bmo)
Comments addressed, encoding now shares buffers when possible and uses passed in locale name instead of always the current locale.
Attachment #8565680 - Attachment is obsolete: true
Attached patch Part 2. API changes, v2 (obsolete) — Splinter Review
Comments addressed, indexes have a locale now.
Attachment #8565574 - Attachment is obsolete: true
This will eventually be folded on top of 3. Builds on top of the new stuff from bug 1131776.
Attached patch More plumbing, WIP (obsolete) — Splinter Review
All the pieces that now need to hold a locale string do so, but there are still some weird interactions with the code from bug 1131776.
Attachment #8631267 - Attachment is obsolete: true
Attached patch Even more plumbing, WIP (obsolete) — Splinter Review
Final bits of changes needed for the rebase.
To do:
 - Figure out if we can remove the assertion in CompressedByteCountForNumber. I emailed janv about it.
 - Split these changes to the individual patches.
 - Make sure everything builds and passes when building with --disable-intl-api.
Assignee: seward.zheng → reuben.bmo
Attachment #8631383 - Attachment is obsolete: true
Attachment #8631218 - Attachment is obsolete: true
Attachment #8633895 - Attachment is obsolete: true
Attached patch 2. API changes, v3 (obsolete) — Splinter Review
Attachment #8631234 - Attachment is obsolete: true
Attachment #8565576 - Attachment is obsolete: true
Attachment #8565577 - Attachment is obsolete: true
Attachment #8565684 - Attachment is obsolete: true
Attached patch 6. Tests, v2Splinter Review
Attachment #8565580 - Attachment is obsolete: true
Comment on attachment 8634479 [details] [diff] [review]
1. ICU and infrastructure bits, v3

Hey Ben, there are still a couple of test failures I'm looking into but the API part can be reviewed. Gregor said you'd review or defer this. Thank you!
Attachment #8634479 - Flags: review?(bent.mozilla)
Attachment #8634481 - Attachment is obsolete: true
Attachment #8635476 - Flags: review?(bent.mozilla)
Attachment #8634486 - Attachment is obsolete: true
Attachment #8635478 - Flags: review?(bent.mozilla)
Attachment #8634487 - Attachment is obsolete: true
Attachment #8635479 - Flags: review?(bent.mozilla)
Attachment #8634488 - Flags: review?(bent.mozilla)
Comment on attachment 8634489 [details] [diff] [review]
6. Tests, v2

This is just rebased, carrying r=bent.
Attachment #8634489 - Flags: review+
I'm going to do the reviews.
(In reply to Jan Varga [:janv] from comment #69)
> I'm going to do the reviews.

Cool, thanks! ni-? me if you get stuck anywhere ok?
(In reply to Ben Turner (not reading bugmail, use the needinfo flag!) from comment #70)
> (In reply to Jan Varga [:janv] from comment #69)
> > I'm going to do the reviews.
> 
> Cool, thanks! ni-? me if you get stuck anywhere ok?

Ok, good to know.
Attached patch All patches folded into one (obsolete) — Splinter Review
Jan, any ETA for the review?
Flags: needinfo?(Jan.Varga)
Sorry, I was busy and then on vacation last week. I'll get back to it soon.
Flags: needinfo?(Jan.Varga)
Attachment #8643832 - Flags: review?(Jan.Varga)
Comment on attachment 8643832 [details] [diff] [review]
All patches folded into one

Review of attachment 8643832 [details] [diff] [review]:
-----------------------------------------------------------------

First of all nice work!

However, I found some issues with the patch applied locally:

1. --without-intl-api builds fine on mac, but |mach mochitest -f plain dom/indexedDB| fails

2. xpcshell-test doesn't run new local-aware tests (should be added to xpcshell-shared.ini)

3. xpcshell-test (with the patch applied, without new locale-aware tests) runs much slower

|time mach xpcshell-test dom/indexedDB| gives me:
real	0m54.594s
user	1m56.732s
sys	1m11.514s

versus

real	2m25.986s
user	1m58.233s
sys	1m15.415s


I also rebased the patch and pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3945b984be9e

::: dom/indexedDB/ActorsParent.cpp
@@ +25274,5 @@
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        return rv;
>      }
>    }
> +    

trailing whitespace

@@ +25413,5 @@
>  
>    nsCString table = mCursor->mUniqueIndex ?
>      NS_LITERAL_CSTRING("unique_index_data") :
>      NS_LITERAL_CSTRING("index_data");
> +  

trailing whitespace

::: dom/indexedDB/IDBCursor.cpp
@@ +125,5 @@
>  already_AddRefed<IDBCursor>
>  IDBCursor::Create(BackgroundCursorChild* aBackgroundActor,
>                    const Key& aKey,
>                    const Key& aPrimaryKey,
> +                  const Key& aSortKey,

Nit: aSortKey could be moved before aPrimaryKey here and in other Create(0 methods

@@ +319,5 @@
>  void
>  IDBCursor::GetKey(JSContext* aCx, JS::MutableHandle<JS::Value> aResult,
>                    ErrorResult& aRv)
>  {
> +  AssertIsOnOwningThread(); 

Extra whitespace

@@ +803,5 @@
>  
>  void
>  IDBCursor::Reset(Key&& aKey,
>                   Key&& aPrimaryKey,
> +                 Key&& aSortKey,

Again, could be moved before aPrimaryKey.

@@ +814,5 @@
>  
>    mKey = Move(aKey);
>    mPrimaryKey = Move(aPrimaryKey);
>    mCloneInfo = Move(aValue);
> +  mSortKey = Move(aSortKey);

move before |mPrimaryKey = Move(aPrimaryKey);|

@@ +822,5 @@
>  
>  void
> +IDBCursor::Reset(Key&& aKey,
> +                 Key&& aPrimaryKey,
> +                 Key&& aSortKey)

here too, the idea is to match the order in IDBCursor.h member declaration

@@ +831,5 @@
>    Reset();
>  
>    mKey = Move(aKey);
>    mPrimaryKey = Move(aPrimaryKey);
> +  mSortKey = Move(aSortKey);

move before |mPrimaryKey = Move(aPrimaryKey);|

::: dom/indexedDB/IDBIndex.cpp
@@ +278,5 @@
>    }
>  
>    const int64_t objectStoreId = mObjectStore->Id();
>    const int64_t indexId = Id();
> +  

Remove the extra whitespace here.

@@ +281,5 @@
>    const int64_t indexId = Id();
> +  
> +  SerializedKeyRange serializedKeyRange;
> +  keyRange->ToSerialized(serializedKeyRange);
> +  OptionalKeyRange optionalKeyRange = serializedKeyRange;

What bent said: "No need for the |optionalKeyRange| here, just pass |serializedKeyRange| directly to the IndexGetKeyParams/IndexGetParams constructor below."

::: dom/indexedDB/IDBIndex.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_indexeddb_idbindex_h__
>  #define mozilla_dom_indexeddb_idbindex_h__
>  
> +#include "IndexedDatabaseManager.h"

Not needed.

::: dom/indexedDB/IDBKeyRange.h
@@ +180,3 @@
>  };
>  
> +class IDBLocaleAwareKeyRange final : public IDBKeyRange {

I think we started putting |: public| on its own line.

@@ +199,5 @@
> +        JS::Handle<JS::Value> aLower,
> +        JS::Handle<JS::Value> aUpper,
> +        bool aLowerOpen,
> +        bool aUpperOpen,
> +        ErrorResult& aRv);

I know you just followed the order in IDBKeyRange, but I think we started using something like this:

public:
  static already_AddRefed<IDBLocaleAwareKeyRange>
  Bound(...);

  NS_DECL_ISUPPORTS_INHERITED

  // nsWrapperCache
  bool
  WrapObject(...);

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +412,5 @@
> +      // icu::Locale::getBaseName is always ASCII as per BCP 47
> +      mLocale.AssignASCII(locale.getBaseName());
> +      break;
> +    }
> +  }

New line here.
Attachment #8643832 - Flags: review?(Jan.Varga) → review-
(In reply to Jan Varga [:janv] from comment #75)
> I also rebased the patch and pushed to try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3945b984be9e

Android seems to be not happy, see test_locale_aware_indexes.html failures.
(In reply to Jan Varga [:janv] from comment #76)
> (In reply to Jan Varga [:janv] from comment #75)
> > I also rebased the patch and pushed to try:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=3945b984be9e
> 
> Android seems to be not happy, see test_locale_aware_indexes.html failures.

I guess intl-api is not available on Android, so you should be fine if you fix 1. from comment 75.
(In reply to Jan Varga [:janv] from comment #75)
> 3. xpcshell-test (with the patch applied, without new locale-aware tests)
> runs much slower
> 
> |time mach xpcshell-test dom/indexedDB| gives me:
> real	0m54.594s
> user	1m56.732s
> sys	1m11.514s
> 
> versus
> 
> real	2m25.986s
> user	1m58.233s
> sys	1m15.415s

Hm, I can't reproduce this. There's quite a bit of variability in the run time though. Two runs with patches applied took 1m52s and 3m21s and then two without the patches took 2m09s and 2m56s…

> I also rebased the patch and pushed to try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3945b984be9e

Thanks!

I'll address the comments, rebase the individual patches and push to try again.
Comments addressed, except for the xpcshell timing problem, since I couldn't reproduce it.
Attachment #8643832 - Attachment is obsolete: true
Attachment #8654338 - Flags: review?(Jan.Varga)
Attachment #8634479 - Flags: review?(bent.mozilla)
Attachment #8634488 - Flags: review?(bent.mozilla)
Attachment #8635476 - Flags: review?(bent.mozilla)
Attachment #8635478 - Flags: review?(bent.mozilla)
Attachment #8635479 - Flags: review?(bent.mozilla)
Comment on attachment 8654338 [details] [diff] [review]
All patches folded into one, v2

Review of attachment 8654338 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to apply the new patch and see how xpcshell-test performs.

::: dom/indexedDB/IDBIndex.cpp
@@ +278,5 @@
>    }
>  
>    const int64_t objectStoreId = mObjectStore->Id();
>    const int64_t indexId = Id();
> +  

I still see a trailing whitespace here.

@@ +281,5 @@
>    const int64_t indexId = Id();
> +  
> +  SerializedKeyRange serializedKeyRange;
> +  keyRange->ToSerialized(serializedKeyRange);
> +  OptionalKeyRange optionalKeyRange = serializedKeyRange;

As it was pointed out before OptionalKeyRange is not needed.
Remove |OptionalKeyRange optionalKeyRange = serializedKeyRange;|
and then replace |optionalKeyRange| with |serializedKeyRange| below.
Attachment #8654338 - Flags: review?(Jan.Varga) → review+
Er, sorry about that, I missed those comments when updating the patch. Thanks for the review!
I still see a problem with running mach xpcshell-test, it seems that test_temporary_storage.js takes really long time to finish.

without the patch:
0:27.76 TEST_START: Thread-159 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_writer_starvation.js
 0:28.02 TEST_END: Thread-145 PASS
 0:28.16 TEST_END: Thread-149 PASS
 0:28.31 TEST_END: Thread-103 PASS
 0:28.33 TEST_END: Thread-140 PASS
 0:28.50 TEST_END: Thread-63 PASS
 0:28.57 TEST_END: Thread-151 PASS
 0:28.67 TEST_END: Thread-153 PASS
 0:28.84 TEST_END: Thread-155 PASS
 0:28.95 TEST_END: Thread-146 PASS
 0:29.21 TEST_END: Thread-159 PASS
 0:29.45 TEST_END: Thread-157 PASS
 0:29.66 TEST_END: Thread-144 PASS
 0:29.74 TEST_END: Thread-148 PASS
 0:29.83 TEST_END: Thread-152 PASS
 0:29.95 TEST_END: Thread-154 PASS
 0:29.98 TEST_END: Thread-150 PASS
 0:30.11 TEST_END: Thread-142 PASS
 0:30.13 TEST_END: Thread-105 PASS
 0:30.23 TEST_END: Thread-158 PASS
 0:30.43 TEST_END: Thread-156 PASS
 0:30.87 TEST_END: Thread-143 PASS
 0:31.99 TEST_END: Thread-62 PASS
 0:32.10 TEST_END: Thread-136 PASS
 0:34.90 TEST_END: Thread-138 PASS
 0:35.35 TEST_END: Thread-135 PASS
 0:37.07 TEST_END: Thread-137 PASS
 0:46.03 TEST_END: Thread-124 PASS
 0:47.24 TEST_END: Thread-90 PASS
 0:48.35 TEST_END: Thread-89 PASS
 0:48.37 TEST_END: Thread-76 PASS
 0:48.66 TEST_END: Thread-75 PASS
 0:52.80 TEST_END: Thread-139 PASS
 0:52.85 LOG: MainThread INFO INFO | Result summary:
 0:52.85 LOG: MainThread INFO INFO | Passed: 158
 0:52.85 LOG: MainThread INFO INFO | Failed: 0
 0:52.85 LOG: MainThread INFO INFO | Todo: 0
 0:52.85 LOG: MainThread INFO INFO | Retried: 0
 0:52.85 SUITE_END: MainThread 
Summary
=======

Ran 159 tests
Expected results: 158
Unexpected results: 0
Skipped: 1

OK

real	1m0.008s
user	2m2.742s
sys	1m19.133s



with the patch:
0:24.61 TEST_START: Thread-147 dom/indexedDB/test/unit/test_temporary_storage.js
 0:24.90 TEST_END: Thread-128 PASS
 0:24.92 TEST_END: Thread-136 PASS
 0:24.95 TEST_START: Thread-149 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_traffic_jam.js
 0:24.95 TEST_START: Thread-148 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_traffic_jam.js
 0:25.39 TEST_END: Thread-142 PASS
 0:25.44 TEST_START: Thread-150 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_transaction_abort.js
 0:25.49 TEST_END: Thread-140 PASS
 0:25.51 TEST_START: Thread-151 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_transaction_abort.js
 0:25.61 TEST_END: Thread-103 PASS
 0:25.63 TEST_END: Thread-133 PASS
 0:25.68 TEST_START: Thread-153 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_transaction_abort_hang.js
 0:25.69 TEST_START: Thread-152 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_transaction_abort_hang.js
 0:26.61 TEST_END: Thread-138 PASS
 0:26.68 TEST_START: Thread-154 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_transaction_duplicate_store_names.js
 0:27.00 TEST_END: Thread-141 PASS
 0:27.06 TEST_START: Thread-155 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_transaction_duplicate_store_names.js
 0:27.19 TEST_END: Thread-149 PASS
 0:27.23 TEST_START: Thread-156 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_transaction_error.js
 0:27.49 TEST_END: Thread-139 PASS
 0:27.52 TEST_END: Thread-112 PASS
 0:27.56 TEST_START: Thread-157 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_transaction_error.js
 0:27.56 TEST_START: Thread-158 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_transaction_lifetimes.js
 0:27.84 TEST_END: Thread-155 PASS
 0:27.88 TEST_START: Thread-159 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_transaction_lifetimes.js
 0:27.89 TEST_END: Thread-137 PASS
 0:27.91 TEST_START: Thread-160 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_transaction_lifetimes_nested.js
 0:28.69 TEST_END: Thread-148 PASS
 0:28.75 TEST_START: Thread-161 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_transaction_lifetimes_nested.js
 0:28.81 TEST_END: Thread-154 PASS
 0:28.87 TEST_START: Thread-162 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_transaction_ordering.js
 0:29.02 TEST_END: Thread-153 PASS
 0:29.04 TEST_START: Thread-163 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_transaction_ordering.js
 0:29.59 TEST_END: Thread-157 PASS
 0:29.59 TEST_END: Thread-159 PASS
 0:29.63 TEST_START: Thread-164 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_unique_index_update.js
 0:29.63 TEST_START: Thread-165 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_unique_index_update.js
 0:29.78 TEST_END: Thread-161 PASS
 0:29.85 TEST_START: Thread-166 xpcshell-child-process.ini:dom/indexedDB/test/unit/test_writer_starvation.js
 0:29.95 TEST_END: Thread-163 PASS
 0:29.97 TEST_END: Thread-160 PASS
 0:29.97 TEST_START: Thread-167 xpcshell-parent-process.ini:dom/indexedDB/test/unit/test_writer_starvation.js
 0:30.03 TEST_END: Thread-114 PASS
 0:30.09 TEST_END: Thread-135 PASS
 0:30.41 TEST_END: Thread-111 PASS
 0:30.41 TEST_END: Thread-158 PASS
 0:30.48 TEST_END: Thread-152 PASS
 0:30.48 TEST_END: Thread-162 PASS
 0:30.54 TEST_END: Thread-151 PASS
 0:30.60 TEST_END: Thread-167 PASS
 0:30.69 TEST_END: Thread-156 PASS
 0:30.92 TEST_END: Thread-165 PASS
 0:31.10 TEST_END: Thread-166 PASS
 0:31.27 TEST_END: Thread-164 PASS
 0:31.55 TEST_END: Thread-150 PASS
 0:31.83 TEST_END: Thread-65 PASS
 0:32.03 TEST_END: Thread-113 PASS
 0:34.18 TEST_END: Thread-64 PASS
 0:34.22 TEST_END: Thread-84 PASS
 0:34.34 TEST_END: Thread-144 PASS
 0:35.31 TEST_END: Thread-143 PASS
 0:35.74 TEST_END: Thread-8 PASS
 0:35.74 TEST_END: Thread-13 PASS
 0:35.87 TEST_END: Thread-83 PASS
 0:36.50 TEST_END: Thread-146 PASS
 0:37.13 TEST_END: Thread-145 PASS
 0:37.51 TEST_END: Thread-1 PASS
 0:45.05 TEST_END: Thread-132 PASS
 0:46.21 TEST_END: Thread-98 PASS
 0:47.13 TEST_END: Thread-97 PASS
 0:47.28 TEST_END: Thread-78 PASS
 0:47.54 TEST_END: Thread-77 PASS
 3:25.72 TEST_END: Thread-147 PASS
 3:25.78 LOG: MainThread INFO INFO | Result summary:
 3:25.78 LOG: MainThread INFO INFO | Passed: 166
 3:25.78 LOG: MainThread INFO INFO | Failed: 0
 3:25.78 LOG: MainThread INFO INFO | Todo: 0
 3:25.78 LOG: MainThread INFO INFO | Retried: 0
 3:25.78 SUITE_END: MainThread 
Summary
=======

Ran 167 tests
Expected results: 166
Unexpected results: 0
Skipped: 1

OK

real	3m30.666s
user	2m5.943s
sys	1m20.835s
(In reply to Jan Varga [:janv] from comment #83)
> I still see a problem with running mach xpcshell-test, it seems that
> test_temporary_storage.js takes really long time to finish.

We should try to resolve this before landing.
We sometimes take much longer to close these databases the second time around. Weirdly enough, it only happens if the window doesn't have focus. Maybe some time of background throttling? As soon as I focus the runner window it goes back to being fast. I don't think this will affect tests run in automation at all, but I'll double check.

 0:35.77 LOG: Thread-1 INFO "Stage 2 - Closing all databases and then attempting to create one more, then verifying that the oldest origin was cleared"
 0:35.77 LOG: Thread-1 INFO "Closing database for http://foo0.com"
 0:45.87 LOG: Thread-1 INFO "Closing database for http://foo1.com"
 0:55.97 LOG: Thread-1 INFO "Closing database for http://foo2.com"
 1:06.08 LOG: Thread-1 INFO "Closing database for http://foo3.com"
 1:07.69 LOG: Thread-1 INFO "Closing database for http://foo4.com"
 1:13.71 LOG: Thread-1 INFO "Closing database for http://foo5.com"
 1:14.00 LOG: Thread-1 INFO "Closing database for http://foo6.com"
 1:24.10 LOG: Thread-1 INFO "Closing database for http://foo7.com"
 1:27.68 LOG: Thread-1 INFO "Closing database for http://foo8.com"
 1:28.01 LOG: Thread-1 INFO "Closing database for http://foo9.com"
 1:30.33 LOG: Thread-1 INFO "Closing database for http://foo10.com"
 1:30.43 LOG: Thread-1 INFO "Closing database for http://foo11.com"
 1:30.53 LOG: Thread-1 INFO "Closing database for http://foo12.com"
 1:31.41 LOG: Thread-1 INFO "Closing database for http://foo13.com"
 1:31.51 LOG: Thread-1 INFO "Closing database for http://foo14.com"
 1:31.61 LOG: Thread-1 INFO "Closing database for http://foo15.com"
 1:31.73 LOG: Thread-1 INFO "Closing database for http://foo16.com"
 1:31.83 LOG: Thread-1 INFO "Closing database for http://foo17.com"
 1:31.93 LOG: Thread-1 INFO "Closing database for http://foo18.com"
 1:32.03 LOG: Thread-1 INFO "Closing database for http://foo19.com"
 1:42.14 LOG: Thread-1 INFO "Opening database for http://foo20.com with version 2"
(In reply to Reuben Morais [:reuben] from comment #85)
> We sometimes take much longer to close these databases the second time
> around. Weirdly enough, it only happens if the window doesn't have focus.
> Maybe some time of background throttling? As soon as I focus the runner
> window it goes back to being fast. I don't think this will affect tests run
> in automation at all, but I'll double check.
> 

I'm aware of the problem with the focus, but that only happens when I run mochitests (which actually opens a browser window). xpcshell-tests doesn't open a browser window, it's just a shell.
On OS X there's still an icon for the shell on the dock. As soon as I click the icon or cmd+tab to it the test gets fast again.
This doesn't seem to affect timings on automation, inspecting the logs they're all finishing in ~36s: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc57982f133f

This timing matches runs without my patch. (I just looked at a random cset: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=de9571163bbb)
Ok then, sorry for the extra time you had to spend on this.
Comment on attachment 8635476 [details] [diff] [review]
2. API changes, v4

Jonas, can you review the WebIDL changes here?
Attachment #8635476 - Flags: review?(jonas)
Comment on attachment 8634488 [details] [diff] [review]
5. IDBLocaleAwareKeyRange, v2

And these.
Attachment #8634488 - Flags: review?(jonas)
Comment on attachment 8635476 [details] [diff] [review]
2. API changes, v4

Review of attachment 8635476 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: dom/indexedDB/IDBCursor.cpp
@@ +319,5 @@
>  void
>  IDBCursor::GetKey(JSContext* aCx, JS::MutableHandle<JS::Value> aResult,
>                    ErrorResult& aRv)
>  {
> +  AssertIsOnOwningThread(); 

You added a whitespace at the end of the line.

::: dom/webidl/IDBIndex.webidl
@@ +30,5 @@
>      readonly    attribute boolean        unique;
>  
> +    // <null>:   Not locale-aware, uses normal JS sorting.
> +    // <string>: Sorted based on the rules of the specified locale.
> +    //           Note: never returns "auto", only the current locale.

Generally speaking, we should not have internal state which isn't exposed to pages. So I think we should enable pages to see that an index is "auto"-sorted.

I don't care strongly if we do that by having this property return "auto", or if we add a separate
"readonly attribute boolean autoLocale;" or some such.
Attachment #8635476 - Flags: review?(jonas) → review+
Comment on attachment 8634488 [details] [diff] [review]
5. IDBLocaleAwareKeyRange, v2

Review of attachment 8634488 [details] [diff] [review]:
-----------------------------------------------------------------

so is this intended not to compare the upper and lower bound? Normally IDBKeyRange.bound() throws an exception if the lower bound is greater than the upper bound. But since IDBLocaleAwareKeyRange.bound() doesn't know which locale it is created for it can't compare the upper/lower bounds.

Looking at the code, it looks like this will still throw if the upper/lower bounds aren't properly sorted which seems wrong.

Or am I misunderstanding the code?

::: dom/webidl/IDBKeyRange.webidl
@@ +32,5 @@
> +[Exposed=(Window,Worker),
> + Func="mozilla::dom::indexedDB::IndexedDatabaseManager::ExperimentalFeaturesEnabled"]
> +interface IDBLocaleAwareKeyRange : IDBKeyRange {
> +  [NewObject, Throws]
> +  static IDBLocaleAwareKeyRange bound (any lower, any upper, optional boolean lowerOpen = false, optional boolean upperOpen = false);

Why no lowerBound/upperBound functions here?
(In reply to Jonas Sicking (:sicking) from comment #93)
> Comment on attachment 8634488 [details] [diff] [review]
> 5. IDBLocaleAwareKeyRange, v2
> 
> Review of attachment 8634488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> so is this intended not to compare the upper and lower bound? Normally
> IDBKeyRange.bound() throws an exception if the lower bound is greater than
> the upper bound. But since IDBLocaleAwareKeyRange.bound() doesn't know which
> locale it is created for it can't compare the upper/lower bounds.

Yep, exactly.

> Looking at the code, it looks like this will still throw if the upper/lower
> bounds aren't properly sorted which seems wrong.
> 
> Or am I misunderstanding the code?

The only difference between IDBKeyRange::Bound and IDBLocaleAwareKeyRange::Bound is that the latter doesn't check the order of the bounds. We still check if the bounds are equal and either of them is open, aka. (a,a], [a,a), (a,a). I just maintained this behavior from IDBKeyRange::Bound, I don't know why we throw in that case, maybe because it matches a single key?

> ::: dom/webidl/IDBKeyRange.webidl
> @@ +32,5 @@
> > +[Exposed=(Window,Worker),
> > + Func="mozilla::dom::indexedDB::IndexedDatabaseManager::ExperimentalFeaturesEnabled"]
> > +interface IDBLocaleAwareKeyRange : IDBKeyRange {
> > +  [NewObject, Throws]
> > +  static IDBLocaleAwareKeyRange bound (any lower, any upper, optional boolean lowerOpen = false, optional boolean upperOpen = false);
> 
> Why no lowerBound/upperBound functions here?

They're inherited from IDBKeyRange and behave the same way.
Comment on attachment 8634488 [details] [diff] [review]
5. IDBLocaleAwareKeyRange, v2

Review of attachment 8634488 [details] [diff] [review]:
-----------------------------------------------------------------

I thought that static functions didn't inherit, but assuming that's wrong, r=me
Attachment #8634488 - Flags: review?(jonas) → review+
You could also add r=bent since he did the original review.
Depends on: 1202788
Thank you, and thanks to Jan, Jonas, and others who helped along the way. But don't get too excited just yet, as it's getting backed out in bug 1202788 :(
I'm so excited by this bug, I wasn't realizing you were working on it :)
I have documented this new feature, and Reuben has kindly agreed to give me a tech review. 

1. Explanation of feature: https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB#Locale-aware_sorting
2. New IDBIndex properties: https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex#Properties
3. Individual pages for new properties:

3. New parameter inside createIndex(): https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/createIndex#Parameters
4. Mention of the new interface on the homepage: https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API#Retrieving_and_modifying_data
5. New Interface page: https://developer.mozilla.org/en-US/docs/Web/API/IDBLocaleAwareKeyRange
Flags: needinfo?(reuben.bmo)
Dammnit, itchy trigger finger.

Here are the individual pages for new properties:

https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/isAutoLocale
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/locale

I will also add an entry to the Firefox 43 release notes very soon.

Thanks!
I reviewed all the linked docs. Thanks for documenting this work Chris!
Flags: needinfo?(reuben.bmo)
Anne, Marcos, either of you could bring this up with the WG? I'd love to be able to turn this on to all webpages since we've done the engineering to implement.
Depends on: 1215247
Depends on: 1215252
Comment on attachment 8634479 [details] [diff] [review]
1. ICU and infrastructure bits, v3

>+      nsDependentString str;
>+      DecodeString(it, end, str);
>+      aTarget.EncodeLocaleString(str, typeOffset, aLocale);
Nit: str never depends on anything, so nsDependentString is the wrong type for this. You probably want nsAutoString.

>+nsresult
>+Key::EncodeLocaleString(const nsDependentString& aString, uint8_t aTypeOffset,
>+                        const nsCString& aLocale)
Nit: Argument types should always be ns[A][C]String&, not nsDependentString&.
You need to log in before you can comment on or make changes to this bug.