Closed Bug 589032 Opened 14 years ago Closed 12 years ago

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

Categories

(Toolkit :: Storage, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: RyanVM, Assigned: froydnj)

References

(Blocks 1 open bug, )

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: [build_warning])

Attachments

(1 file, 2 obsolete files)

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'
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)
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: buildwarning
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?
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
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.
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+
Attached patch patch (obsolete) — Splinter Review
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)
Attached patch patchSplinter Review
Now with .idl comments really fixed.
Attachment #620359 - Attachment is obsolete: true
Attachment #620375 - Flags: review?(mak77)
Attachment #620359 - Flags: review?(mak77)
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+
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/5371c239bbf6
Status: ASSIGNED → RESOLVED
Closed: 12 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.
(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.
(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.
Blocks: 755458
Flags: in-testsuite+
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.