1186791 - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are three occurrences of EnumerateRead() in this directory.

A note to the assignee: to preserve existing behaviour, you should probably use
nsBaseHashtable::Iterator::UserData() rather than nsBaseHashtable::Iterator::Data(). (The latter should be used when replacing nsBaseHashtable::Enumerate()).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8679783 [details] [diff] [review]
(part 1) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

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

::: storage/mozStorageBindingParams.cpp
@@ +197,5 @@
>    // hashtable.
>    if (!mNamedParameters.Count())
>      return BindingParams::bind(aStatement);
>  
> +  nsCOMPtr<mozIStorageError> err = nullptr;

afaik, there's no need to assign nullptr to nsCOMPtr, it should already be initialized to that, unless it changed in the last months

@@ +226,5 @@
> +      // We had an error while trying to bind.  Now we need to create an error
> +      // object with the right message.  Note that we special case
> +      // SQLITE_MISMATCH, but otherwise get the message from SQLite.
> +      const char *msg = "Could not covert nsIVariant to SQLite type.";
> +      if (rc != SQLITE_MISMATCH)

nit: while here, please brace this if...

@@ +229,5 @@
> +      const char *msg = "Could not covert nsIVariant to SQLite type.";
> +      if (rc != SQLITE_MISMATCH)
> +        msg = ::sqlite3_errmsg(::sqlite3_db_handle(aStatement));
> +
> +      err = new Error(rc, msg);

...and then you can remove the newline before this.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8679785 [details] [diff] [review]
(part 3) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

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

::: storage/mozStorageConnection.cpp
@@ +1317,5 @@
> +  for (auto iter = mFunctions.Iter(); !iter.Done(); iter.Next()) {
> +    const nsACString &key = iter.Key();
> +    Connection::FunctionInfo data = iter.UserData();
> +
> +    NS_PRECONDITION(data.type == Connection::FunctionInfo::SIMPLE ||

please change this to a MOZ_ASSERT, it would be a coding mistake.

@@ +1324,5 @@
> +
> +    if (data.type == Connection::FunctionInfo::SIMPLE) {
> +      mozIStorageFunction *function =
> +        static_cast<mozIStorageFunction *>(data.function.get());
> +      (void)aClone->CreateFunction(key, data.numArgs, function);

while here, this should probably warn and not just swallow any error... could define a DebugOnly<rv> and warn something like "Failed to copy function '$key' to cloned connection".

@@ +1328,5 @@
> +      (void)aClone->CreateFunction(key, data.numArgs, function);
> +    } else {
> +      mozIStorageAggregateFunction *function =
> +        static_cast<mozIStorageAggregateFunction *>(data.function.get());
> +      (void)aClone->CreateAggregateFunction(key, data.numArgs, function);

ditto

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thanks for this cleanup!

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/582ecb12478ae567b439bb5aa01a1ac3b9551e61
Bug 1186791 (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators. r=mak.

https://hg.mozilla.org/integration/mozilla-inbound/rev/15aad0a5b3cb1695bacaf3e4ecb40339cb7a2a1c
Bug 1186791 (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators. r=mak.

https://hg.mozilla.org/integration/mozilla-inbound/rev/234e629e87cc1761289130f85039bba64a6ea07e
Bug 1186791 (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators. r=mak.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/582ecb12478a
https://hg.mozilla.org/mozilla-central/rev/15aad0a5b3cb
https://hg.mozilla.org/mozilla-central/rev/234e629e87cc

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/582ecb12478a
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/15aad0a5b3cb
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/234e629e87cc