555218 - Fix Places usage of deprecated Storage binding APIs

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

bug 507414 declared deprecated a bunch of binding functions we use, we should update to the new code.

Comment 1

[Marco Bonardo [:mak]]

I will have to do this in pieces, or i'll get mad.
I must admit the new API is much more verbose...

OLD:

nsresult rv = getTagsStatement->BindInt64Parameter(0, history->GetTagsFolder());
NS_ENSURE_SUCCESS(rv, rv);
rv = getTagsStatement->BindUTF8StringParameter(1, );
NS_ENSURE_SUCCESS(rv, rv);

NEW:

nsCOMPtr<mozIStorageBindingParamsArray> paramsArray;
stmt->NewBindingParamsArray(getter_AddRefs(paramsArray));
NS_ENSURE_STATE(paramsArray);
nsCOMPtr<mozIStorageBindingParams> params;
paramsArray->NewBindingParams(getter_AddRefs(params));
NS_ENSURE_STATE(params);
nsresult rv = params->BindInt64ByIndex(0, history->GetTagsFolder());
NS_ENSURE_SUCCESS(rv, rv);
rv = params->BindUTF8StringByIndex(1, mURI);
NS_ENSURE_SUCCESS(rv, rv);
rv = paramsArray->AddParams(params);
NS_ENSURE_SUCCESS(rv, rv);
rv = stmt->BindParameters(paramsArray);
NS_ENSURE_SUCCESS(rv, rv);

this is 10 more lines, to do the same. I have managed a couple macros to remove some boilerplate and redure these to 6 lines, not much funny though.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> I will have to do this in pieces, or i'll get mad.
> I must admit the new API is much more verbose...
I think you are confused.  The new way to write the old one is this:

> nsresult rv = getTagsStatement->BindInt64ByIndex(0,
> history->GetTagsFolder());
> NS_ENSURE_SUCCESS(rv, rv);
> rv = getTagsStatement->BindUTF8StringByIndex(1, );
> NS_ENSURE_SUCCESS(rv, rv);
It's really just s/Parameter/ByIndex/g

Comment 3

[Marco Bonardo [:mak]]

Attachment: nsNavHistoryResults.cpp conversion + macros + helpers

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 441135 [details] [diff] [review]
nsNavHistoryResults.cpp conversion + macros + helpers

gaaak, thanks for clearing up my confusion, the lack of blog posts or new documentation confused me.

The new APIs are fine instead.

Comment 5

[Marco Bonardo [:mak]]

Attachment: Part 1: nsNavHistoryResult.cpp + new URIBinder + macros

This fixes nsNavHistoryResults.cpp
I have introduced a new URIBinder static class that will replace the 2 helpers in nsNavHistory.h, i'll replace calls to them in next parts and finally remove the helpers in a next patch. It has only one Bind() method with various overloads, thus we can call URIBinder::Bind(StmtOrParams, IndexOrName, URIOrURLString).
I had already written a couple boilerplate macros for bindingParamsArray, thus i did not remove them, they could come at hand in future.

Comment 6

[Marco Bonardo [:mak]]

Attachment: Part 2: History.cpp

Comment 7

[Marco Bonardo [:mak]]

Attachment: 1: nsNavHistoryResults + URIBinder + macros

Comment 8

[Marco Bonardo [:mak]]

Attachment: 2: History.cpp

Comment 9

[Marco Bonardo [:mak]]

Attachment: 3: nsAnnotationService.cpp

Comment 10

[Marco Bonardo [:mak]]

Attachment: 4: nsFaviconService.cpp

Comment 11

[Marco Bonardo [:mak]]

Attachment: 5: AsyncFaviconsHelpers.cpp

Comment 12

[Marco Bonardo [:mak]]

Attachment: 6: nsNavBookmarks.cpp

Comment 13

[Marco Bonardo [:mak]]

Attachment: 6: nsNavBookmarks.cpp

Comment 14

[Marco Bonardo [:mak]]

Attachment: 7: nsNavHistory.cpp

ok, these should be the last deprecate warnings.

now i'm going to do 2 things:
- push to tryserver
- check other recently introduced warning with Linux (that is usually the pickier one)

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 441267 [details] [diff] [review]
1: nsNavHistoryResults + URIBinder + macros

># HG changeset patch
># Parent cea8bca74947e7f5d6ce2ef6ba70c2f42b7ee170
>Bug 555218 : 1 - Fix Places usage of just deprecated Storage APIs.
>
>diff --git a/toolkit/components/places/src/Helpers.cpp b/toolkit/components/places/src/Helpers.cpp
>--- a/toolkit/components/places/src/Helpers.cpp
>+++ b/toolkit/components/places/src/Helpers.cpp
>@@ -39,6 +39,7 @@
> #include "Helpers.h"
> #include "mozIStorageError.h"
> #include "nsString.h"
>+#include "nsNavHistory.h"

not used?

>+nsresult // static
>+URIBinder::Bind(mozIStorageStatement* aStatement,
>+                PRInt32 aIndex,
>+                nsIURI* aURI)
>+{
>+  NS_ASSERTION(aStatement, "Must have non-null statement");
>+  NS_ASSERTION(aURI, "Must have non-null uri");
>+
>+  URI_TO_URLCSTRING(aURI, spec);
>+  return URIBinder::Bind(aStatement, aIndex, spec);
>+}

can you copy over the comments you have on each of these in the header file to here?

also, these aren't specific to Places. should these be in a Storage helper file instead?

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 441267 [details] [diff] [review]
1: nsNavHistoryResults + URIBinder + macros



>+// Boilerplate macros to bind multiple sets of parameters to an async statement.
>+// Example:
>+//   mozStorageStatementScoper scoper(stmt);
>+//   PLACES_START_BINDING_PARAMS_ARRAY(stmt, params);
>+//   nsresult rv = params->BindInt64ByIndex(0, integerValue);
>+//   NS_ENSURE_SUCCESS(rv, rv);
>+//   PLACES_END_BINDING_PARAMS_ARRAY(stmt, params);
>+//   PRBool hasResults;
>+//   rv = stmt->ExecuteStep(&hasResults);

you say async in the commeunt, but the example looks sync.

also, if the macro name should mention async as well (i know, it's already so long :P).

Comment 17

[Marco Bonardo [:mak]]

> >diff --git a/toolkit/components/places/src/Helpers.cpp b/toolkit/components/places/src/Helpers.cpp

> >+#include "nsNavHistory.h"
> 
> not used?

it is needed for URI_LENGTH_MAX sharing

> can you copy over the comments you have on each of these in the header file to
> here?

I thought we wanted documentation in headers rather than on the implementation? people should look at headers actually, they don't need anything other

> also, these aren't specific to Places. should these be in a Storage helper file
> instead?

Places way to store URIs is wrong, there is an old bug about that from Brett, we should save channel's charset and code them based on it, plus always escape or always unescape. thus we should not port this bad behavior out of Places (and we can't fix it otherwise half of uris in history will stop working.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 441539 [details] [diff] [review]
7: nsNavHistory.cpp

sheesh, should re-macro NS_LITERAL_CSTRING into NLC!

Comment 19

[Marco Bonardo [:mak]]

Attachment: 1: nsNavHistoryResults + URIBinder

added comments in the URIBinder impl. I removed the async binding macros because I noticed a missing part and adding it I was not saving enough from the original API. Thus i don't think they're useful in this shape.

Comment 20

[Marco Bonardo [:mak]]

Attachment: 8: other misc warnings

This just fixes other misc warnings i found with Linux GCC

Comment 21

[Marco Bonardo [:mak]]

8: changeset ee3ad68b578b
7: changeset 6330016c7cfc
6: changeset 748c6c4fa27a
5: changeset 660a7fe071c9
4: changeset 1b4849f77693
3: changeset a2001f33ecf4
2: changeset 514ec51a4767
1: changeset 61778760cd28