Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 11 years ago

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;

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 1 • 11 years ago

Unfortunately sqlite3.c does not currently compile with SQLITE_ENABLE_ICU.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 2 • 11 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 3 • 11 years ago

I'm still interested in this, but will not have a chance to work on it immediately.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 4 • 11 years ago

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?

	Gregor Wagner [:gwagner]
	Comment 5 • 11 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 6 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 7 • 11 years ago

Isn't koi branching on Monday?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 8 • 11 years ago

Yeah, there's no way that this is going to make koi.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 9 • 11 years ago

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.

	Mike Lee [:mlee]
	Comment 10 • 11 years ago

Clearing 1.3? flag. Tracking this in our FxOS Perf backlog http://scrumbu.gs/p/fxos-perf/

	Mike Lee [:mlee]
	Comment 11 • 11 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 12 • 11 years ago

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.

	Andrew Sutherland [:asuth] (he/him)
	Comment 15 • 10 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 19 • 10 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 20 • 10 years ago

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.

	Axel Hecht [:Pike]
	Comment 21 • 10 years ago

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?

	Shihua Zheng
	Comment 22 • 10 years ago

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

	Shihua Zheng
	Comment 25 • 10 years ago

ping..
Any update?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 26 • 10 years ago

Hi Shihua! Sorry for the delay here. We've got someone scheduled to merge all your changes into the reworked IndexedDB code really soon!

	Reuben Morais [:reuben] Assignee
	Comment 31 • 9 years ago

Carrying r=bent

	Reuben Morais [:reuben] Assignee
	Comment 32 • 9 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 33 • 9 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 34 • 9 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 43 • 9 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 44 • 9 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 45 • 9 years ago

(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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 46 • 9 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 47 • 9 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 48 • 9 years ago

If that's defined behavior elsewhere, then that sounds great to me.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 49 • 9 years ago

Hey Reuben, any update here? I'd love to get a feel for the progress you've made.

	Reuben Morais [:reuben] Assignee
	Comment 50 • 9 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 51 • 9 years ago

Comments addressed, encoding now shares buffers when possible and uses passed in locale name instead of always the current locale.

	Reuben Morais [:reuben] Assignee
	Comment 52 • 9 years ago

Comments addressed, indexes have a locale now.

	Reuben Morais [:reuben] Assignee
	Comment 53 • 9 years ago

This will eventually be folded on top of 3. Builds on top of the new stuff from bug 1131776.

	Reuben Morais [:reuben] Assignee
	Comment 54 • 9 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 55 • 9 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 62 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bb1bafef7a5

	Reuben Morais [:reuben] Assignee
	Comment 63 • 9 years ago

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!

	Reuben Morais [:reuben] Assignee
	Comment 64 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=698dfc951002

yay

	Reuben Morais [:reuben] Assignee
	Comment 68 • 9 years ago

Comment on attachment 8634489 [details] [diff] [review]
6. Tests, v2

This is just rebased, carrying r=bent.

	Jan Varga [:janv]
	Comment 69 • 9 years ago

I'm going to do the reviews.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 70 • 9 years ago

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

	Jan Varga [:janv]
	Comment 71 • 9 years ago

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

	Gregor Wagner [:gwagner]
	Comment 73 • 9 years ago

Jan, any ETA for the review?

	Jan Varga [:janv]
	Comment 74 • 9 years ago

Sorry, I was busy and then on vacation last week. I'll get back to it soon.

	Jan Varga [:janv]
	Comment 75 • 9 years ago

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.

	Jan Varga [:janv]
	Comment 76 • 9 years ago

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

	Jan Varga [:janv]
	Comment 77 • 9 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 78 • 9 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 79 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6a46f96c0e9

	Reuben Morais [:reuben] Assignee
	Comment 80 • 9 years ago

Comments addressed, except for the xpcshell timing problem, since I couldn't reproduce it.

	Jan Varga [:janv]
	Comment 81 • 9 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 82 • 9 years ago

Er, sorry about that, I missed those comments when updating the patch. Thanks for the review!

	Jan Varga [:janv]
	Comment 83 • 9 years ago

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

	Jan Varga [:janv]
	Comment 84 • 9 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 85 • 9 years ago

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"

	Jan Varga [:janv]
	Comment 86 • 9 years ago

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

	Reuben Morais [:reuben] Assignee
	Comment 87 • 9 years ago

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.

	Reuben Morais [:reuben] Assignee
	Comment 88 • 9 years ago

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)

	Jan Varga [:janv]
	Comment 89 • 9 years ago

Ok then, sorry for the extra time you had to spend on this.

	Reuben Morais [:reuben] Assignee
	Comment 90 • 9 years ago

Comment on attachment 8635476 [details] [diff] [review]
2. API changes, v4

Jonas, can you review the WebIDL changes here?

	Reuben Morais [:reuben] Assignee
	Comment 91 • 9 years ago

Comment on attachment 8634488 [details] [diff] [review]
5. IDBLocaleAwareKeyRange, v2

And these.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 92 • 9 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 93 • 9 years ago

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?

	Reuben Morais [:reuben] Assignee
	Comment 94 • 9 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 95 • 9 years ago

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

	Pulsebot
	Comment 96 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/7bfd5e3fc601
https://hg.mozilla.org/integration/b2g-inbound/rev/bab596a35486
https://hg.mozilla.org/integration/b2g-inbound/rev/4dc86060d913
https://hg.mozilla.org/integration/b2g-inbound/rev/15acf4f1961a
https://hg.mozilla.org/integration/b2g-inbound/rev/592e1140be53
https://hg.mozilla.org/integration/b2g-inbound/rev/7c84781f2dbf

	Jan Varga [:janv]
	Comment 97 • 9 years ago

You could also add r=bent since he did the original review.

	Phil Ringnalda (:philor)
	Comment 98 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7bfd5e3fc601
https://hg.mozilla.org/mozilla-central/rev/bab596a35486
https://hg.mozilla.org/mozilla-central/rev/4dc86060d913
https://hg.mozilla.org/mozilla-central/rev/15acf4f1961a
https://hg.mozilla.org/mozilla-central/rev/592e1140be53
https://hg.mozilla.org/mozilla-central/rev/7c84781f2dbf

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 99 • 9 years ago

Thanks Reuben! \o/ \o/ \o/

	Reuben Morais [:reuben] Assignee
	Comment 100 • 9 years ago

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

	Julien Wajsberg [:julienw]
	Comment 101 • 9 years ago

I'm so excited by this bug, I wasn't realizing you were working on it :)

	Chris Mills [:cmills]
	Comment 102 • 9 years ago

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

	Chris Mills [:cmills]
	Comment 103 • 9 years ago

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!

	Chris Mills [:cmills]
	Comment 104 • 9 years ago

Release note entry:

https://developer.mozilla.org/en-US/Firefox/Releases/43#IndexedDB

	Reuben Morais [:reuben] Assignee
	Comment 105 • 9 years ago

I reviewed all the linked docs. Thanks for documenting this work Chris!

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 106 • 9 years ago

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.

	Anne (:annevk)
	Comment 107 • 9 years ago

Sure: https://github.com/w3c/IndexedDB/issues/38.

	neil@parkwaycc.co.uk
	Comment 108 • 9 years ago

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