Closed Bug 555218 Opened 11 years ago Closed 10 years ago

Fix Places usage of deprecated Storage binding APIs

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(8 files, 5 obsolete files)

3.05 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
21.77 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
8.89 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
11.43 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
33.98 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
43.65 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
10.83 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
2.98 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
bug 507414 declared deprecated a bunch of binding functions we use, we should update to the new code.
Summary: Fix Places Storage methods to avoid DEPRECATE warnings → Fix Places Storage usage to avoid DEPRECATE warnings
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(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
Summary: Fix Places Storage usage to avoid DEPRECATE warnings → Fix Places usage of just deprecated Storage binding APIs
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.
Attachment #441135 - Attachment is obsolete: true
Summary: Fix Places usage of just deprecated Storage binding APIs → Fix Places usage of deprecated Storage binding APIs
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.
Attached patch Part 2: History.cpp (obsolete) — Splinter Review
Attachment #441201 - Attachment description: Part 1: nsNavHistoryResults + new URIBinder + macros → Part 1: nsNavHistoryResult.cpp + new URIBinder + macros
Attachment #441201 - Attachment is obsolete: true
Attached patch 2: History.cppSplinter Review
Attachment #441216 - Attachment is obsolete: true
Attached patch 6: nsNavBookmarks.cpp (obsolete) — Splinter Review
Attachment #441471 - Attachment is obsolete: true
Attachment #441267 - Flags: review?(dietrich)
Attachment #441268 - Flags: review?(dietrich)
Attachment #441269 - Flags: review?(dietrich)
Attachment #441272 - Flags: review?(dietrich)
Attachment #441273 - Flags: review?(dietrich)
Attachment #441472 - Flags: review?(dietrich)
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)
Attachment #441539 - Flags: review?(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 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).
> >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.
Attachment #441268 - Flags: review?(dietrich) → review+
Attachment #441269 - Flags: review?(dietrich) → review+
Attachment #441272 - Flags: review?(dietrich) → review+
Attachment #441273 - Flags: review?(dietrich) → review+
Attachment #441472 - Flags: review?(dietrich) → review+
Comment on attachment 441539 [details] [diff] [review]
7: nsNavHistory.cpp

sheesh, should re-macro NS_LITERAL_CSTRING into NLC!
Attachment #441539 - Flags: review?(dietrich) → review+
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.
Attachment #441267 - Attachment is obsolete: true
Attachment #441593 - Flags: review?(dietrich)
Attachment #441267 - Flags: review?(dietrich)
This just fixes other misc warnings i found with Linux GCC
Attachment #441610 - Flags: review?(dietrich)
Attachment #441593 - Flags: review?(dietrich) → review+
Attachment #441610 - Flags: review?(dietrich) → review+
Blocks: 468878
8: changeset ee3ad68b578b
7: changeset 6330016c7cfc
6: changeset 748c6c4fa27a
5: changeset 660a7fe071c9
4: changeset 1b4849f77693
3: changeset a2001f33ecf4
2: changeset 514ec51a4767
1: changeset 61778760cd28
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.