warning C4996: 'mozIStorageStatementWrapper::Step': was declared deprecated in mozStorageStatementWrapper.h

RESOLVED FIXED in mozilla15

Status

()

Toolkit
Storage
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: RyanVM, Assigned: froydnj)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-needed})

Trunk
mozilla15
x86
Windows 7
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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'
(Reporter)

Updated

7 years ago
Component: Build Config → Storage
Product: Core → Toolkit
QA Contact: build-config → storage
Consumers could fix this, but this method is deprecated.  I can't really help that that means even the code that implements it warns :/
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)
(Reporter)

Comment 3

7 years ago
How long do deprecated functions remain around before being removed?
(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.
Blocks: 187528
(Reporter)

Comment 5

5 years ago
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?
Extensions?
(Reporter)

Comment 7

5 years ago
Jorge, can you see in the addon mxr if any extensions are using this? Note that this has been deprecated since 4.0.
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.
Keywords: addon-compat
(Reporter)

Comment 9

5 years ago
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?
Blocks: 518440
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
(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.
(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.
(Assignee)

Comment 13

5 years ago
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.
Attachment #615851 - Flags: review?(mak77)
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?
Attachment #615851 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 15

5 years ago
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.
Attachment #615851 - Attachment is obsolete: true
Attachment #620359 - Flags: review?(mak77)
(Assignee)

Comment 16

5 years ago
Created attachment 620375 [details] [diff] [review]
patch

Now with .idl comments really fixed.
Attachment #620359 - Attachment is obsolete: true
Attachment #620375 - Flags: review?(mak77)
Attachment #620359 - Flags: review?(mak77)

Updated

5 years ago
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
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.
Attachment #620375 - Flags: review?(mak77) → review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5371c239bbf6
Target Milestone: --- → mozilla15

Updated

5 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/5371c239bbf6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
(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.
(Assignee)

Comment 22

5 years ago
(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.
Thanks Marco & Nathan will post to the thread.
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

5 years ago
(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.
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 27

5 years ago
As reference, http://blog.mozilla.org/nfroyd/2012/05/14/statement-wrappers-have-been-deprecated/

Updated

5 years ago
Blocks: 755458
Flags: in-testsuite+
Blocks: 755733
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
}
Blocks: 754740
No longer blocks: 755733
No longer blocks: 754740
Blocks: 754740
You need to log in before you can comment on or make changes to this bug.