Last Comment Bug 589032 - warning C4996: 'mozIStorageStatementWrapper::Step': was declared deprecated in mozStorageStatementWrapper.h
: warning C4996: 'mozIStorageStatementWrapper::Step': was declared deprecated i...
Status: RESOLVED FIXED
[build_warning]
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Storage (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: buildwarning 518440 754740 755458
  Show dependency treegraph
 
Reported: 2010-08-19 19:22 PDT by Ryan VanderMeulen [:RyanVM]
Modified: 2012-05-19 08:01 PDT (History)
12 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to remove mozIStorageStatementWrapper (33.28 KB, patch)
2012-04-17 13:34 PDT, Nathan Froyd [:froydnj]
mak77: feedback+
Details | Diff | Splinter Review
patch (35.30 KB, patch)
2012-05-02 10:04 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
patch (35.30 KB, patch)
2012-05-02 10:37 PDT, Nathan Froyd [:froydnj]
mak77: review+
Details | Diff | Splinter Review

Description Ryan VanderMeulen [:RyanVM] 2010-08-19 19:22:46 PDT
Another source of MSVC warning spam.

c:\mozbuild\mozilla-central\storage\src\mozStorageStatementWrapper.h(77) : warning C4996: 'mozIStorageStatementWrapper::Step': was declared deprecated
        c:\mozbuild\mozilla-central\objdir-fx\dist\include\mozIStorageStatementWrapper.h(181) : see declaration of 'mozIStorageStatementWrapper::Step'
Comment 1 Shawn Wilsher :sdwilsh 2010-08-19 21:28:37 PDT
Consumers could fix this, but this method is deprecated.  I can't really help that that means even the code that implements it warns :/
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-08-20 04:06:02 PDT
I'm not convinced this is a bad thing.

(We could make deprecated in xpidl not warn, but I don't think that's a good idea)
Comment 3 Ryan VanderMeulen [:RyanVM] 2010-08-20 16:42:32 PDT
How long do deprecated functions remain around before being removed?
Comment 4 Shawn Wilsher :sdwilsh 2010-08-20 17:09:21 PDT
(In reply to comment #3)
> How long do deprecated functions remain around before being removed?
We might still use this in the tree.  If not, we need to fix that, and then we can nuke this.  It's been marked as deprecated for at least two major releases now.
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-02-18 09:33:10 PST
I'd like to take a stab at fixing this. When I search MXR, the only consumers of mozIStorageStatementWrapper that I see are unit tests. Does that mean that this is safe to remove?
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-02-18 10:48:51 PST
Extensions?
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-02-18 10:59:45 PST
Jorge, can you see in the addon mxr if any extensions are using this? Note that this has been deprecated since 4.0.
Comment 8 Jorge Villalobos [:jorgev] 2012-02-20 11:51:23 PST
It's being used by a relatively small number of add-ons: https://mxr.mozilla.org/addons/search?string=mozIStorageStatementWrapper

What is the alternative to using this interface? If there is one, then it's not a problem to remove it. We have ways to communicate this to developers with add-ons on AMO.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-02-20 13:32:47 PST
I'm a little confused. Bug 518440 comment 0 seems to only be about deprecating step() in favor of executeStep(), but then proceeded to deprecate the entire interface. Paul, is the entire interface indeed OK to nuke?
Comment 10 Marco Bonardo [::mak] 2012-02-21 01:57:24 PST
the interface is useless, since the same methods are provided by mozIStorageStatement, also mozIStorageStatement::Step is deprecated, so you are actually looking at two separate things that may be removed.

(In reply to Jorge Villalobos [:jorgev] from comment #8)
> What is the alternative to using this interface?

mozIStorageStatement directly instead of having to QI to mozIStorageStatementWrapper.

For mozIStorageStatement::Step the alternative is mozIStorageStatement::ExecuteStep
Comment 11 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-21 11:06:46 PST
(In reply to Ryan VanderMeulen from comment #9)
> I'm a little confused. Bug 518440 comment 0 seems to only be about
> deprecating step() in favor of executeStep(), but then proceeded to
> deprecate the entire interface. Paul, is the entire interface indeed OK to
> nuke?

That was way too long ago for me and I was mostly working with direction there (I don't think I've touched storage/ since, nor had I done much before). Macro or Shawn are the best to listen to here.
Comment 12 Shawn Wilsher :sdwilsh 2012-02-21 20:18:57 PST
(In reply to Marco Bonardo [:mak] from comment #10)
> mozIStorageStatement directly instead of having to QI to
> mozIStorageStatementWrapper.
You actually have to create a new wrapper.  We provide it automagically now on the statement.

> For mozIStorageStatement::Step the alternative is
> mozIStorageStatement::ExecuteStep
Correct.
Comment 13 Nathan Froyd [:froydnj] 2012-04-17 13:34:44 PDT
Created attachment 615851 [details] [diff] [review]
patch to remove mozIStorageStatementWrapper

Here's a patch to remove mozIStorageStatementWrapper and fixup the fallout.

I removed the tests that used it directly; I suppose those could be fixed up to use mozIStorageStatement directly instead.

Untested, but it does compile at least.
Comment 14 Marco Bonardo [::mak] 2012-04-24 06:56:10 PDT
Comment on attachment 615851 [details] [diff] [review]
patch to remove mozIStorageStatementWrapper

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

what about test_storage_statement_wrapper.js, I doubt it can work with this patch. It should be removed.

I think some of the tests are worth a check if there is some other test covering the same stuff. for example test_bug-429521.js is covering the fact requesting a nonexisting column name doesn't throw, would be nice to have a test doing this for both sync and async statements (not using the wrapper clearly,  but directly the magic helpers). As well as test_bug-444233.js tests passing an array as a param value. These are still valuable basic tests that don't steal a lot of time to do.  Both of them may probably be added as sub-tests to test_statement_wrapper_automatically.js

::: storage/public/mozIStorageStatementParams.idl
@@ +33,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL2 please

@@ +39,5 @@
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(e65fe6e2-2643-463c-97e2-27665efe2386)]
> +interface mozIStorageStatementParams : nsISupports {
> +  // magic interface for parameter setting that implements nsIXPCScriptable.

While here ucfirst please

::: storage/public/mozIStorageStatementRow.idl
@@ +33,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL2 please

@@ +39,5 @@
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(02eeaf95-c3db-4182-9340-222c29f68f02)]
> +interface mozIStorageStatementRow : nsISupports {
> +  // magic interface for parameter setting that implements nsIXPCScriptable.

Looks like you copy-pasted twice the same comment in both interfaces?
Comment 15 Nathan Froyd [:froydnj] 2012-05-02 10:04:25 PDT
Created attachment 620359 [details] [diff] [review]
patch

New patch.  Tests tweaked to just use statements directly without wrappers, and test_storage_statement_wrapper.js removed.  MPL2 used, comments updated.  At least the storage tests pass locally, haven't tried full tests.
Comment 16 Nathan Froyd [:froydnj] 2012-05-02 10:37:01 PDT
Created attachment 620375 [details] [diff] [review]
patch

Now with .idl comments really fixed.
Comment 17 Marco Bonardo [::mak] 2012-05-08 05:53:51 PDT
Comment on attachment 620375 [details] [diff] [review]
patch

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

Thank you.  Ideally you need an SR, though someone already SRed the deprecation, so the removal may be considered just part of that process.
Comment 19 Ed Morley [:emorley] 2012-05-10 07:38:49 PDT
https://hg.mozilla.org/mozilla-central/rev/5371c239bbf6
Comment 20 Jim Jeffery not reading bug-mail 1/2/11 2012-05-11 05:48:23 PDT
Is it possible this is responsible for breaking ForecastFox ?  It no longer displays, and the config page the addon does nothing.

Tested using latest hourly m-c win32 build on win7 x64 
ForecastFox Addon: https://addons.mozilla.org/en-US/firefox/addon/forecastfox-weather/?src=ss

I also have opened a thread in their support group:
http://groups.google.com/group/forecastfox-users/t/7817b96ca6aa75e7
Comment 21 Marco Bonardo [::mak] 2012-05-11 05:52:04 PDT
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #20)
> Is it possible this is responsible for breaking ForecastFox ?

If the add-on uses really old deprecated APIs, it's possible.
Comment 22 Nathan Froyd [:froydnj] 2012-05-11 05:56:25 PDT
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #20)
> Is it possible this is responsible for breaking ForecastFox ?  It no longer
> displays, and the config page the addon does nothing.

Yes, chrome/forecastfox.jar:content/sqlite.js will need to remove its use of statement wrappers.  This is a straightforward process.
Comment 23 Jim Jeffery not reading bug-mail 1/2/11 2012-05-11 05:58:53 PDT
Thanks Marco & Nathan will post to the thread.
Comment 24 Philipp Kewisch [:Fallen] 2012-05-14 02:21:48 PDT
Next time you nuke things, please check comm-central too. This broke Lightning, as did CPG and possibly a few other things. If it were just one aspect alone it wouldn't be that bad, but the vast amount of things breaking is really killing me, especially since I am supposed to be on vacation and won't be getting back until a few days before the next release.

I now know this has been deprecated a while, but I wasn't aware since we have had the wrapper in use since before 4.0.
Comment 25 wayne kroncke 2012-05-14 08:51:19 PDT
(In reply to Nathan Froyd (:froydnj) from comment #22)
... This is a straightforward process.

if so, and no-one wants to have a look themselves, please let us know how to do it. is there a work-around? if so what? i had a quick look thru the xpi, but it's not 'straight forward' to me.
Comment 26 Marco Bonardo [::mak] 2012-05-14 10:56:48 PDT
If you have a step() call, just use executeStep().

If you were creating a mozIStorageStatementWrapper, you don't need it anymore, just directly use the original statement, it exposes the same properties (.row is the mozIStorageStatementRow, .params is the mozIStorageStatementParams).
Basically, before you had to create a wrapper, and initialize it with the statement, then you were using the wrapper, now you directly use the statement, skipping the first 2 steps.
Comment 28 Serge Gautherie (:sgautherie) 2012-05-16 07:17:41 PDT
Missed 1 test:
{
/storage/test/unit/test_statement_wrapper_automatically.js (View Hg log or Hg annotations)
    * line 41 -- // This file tests the functions of mozIStorageStatementWrapper
}

Note You need to log in before you can comment on or make changes to this bug.