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)
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)
35.30 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
Component: Build Config → Storage
Product: Core → Toolkit
QA Contact: build-config → storage
Comment 1•14 years ago
|
||
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•14 years ago
|
||
How long do deprecated functions remain around before being removed?
Comment 4•14 years ago
|
||
(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.
Updated•13 years ago
|
Blocks: buildwarning
Reporter | ||
Comment 5•12 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•12 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.
Comment 8•12 years ago
|
||
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•12 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
Comment 10•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
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 14•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Now with .idl comments really fixed.
Attachment #620359 -
Attachment is obsolete: true
Attachment #620375 -
Flags: review?(mak77)
Attachment #620359 -
Flags: review?(mak77)
Updated•12 years ago
|
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Comment 17•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5371c239bbf6
Target Milestone: --- → mozilla15
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5371c239bbf6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 20•12 years ago
|
||
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•12 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 ? If the add-on uses really old deprecated APIs, it's possible.
Assignee | ||
Comment 22•12 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.
Comment 23•12 years ago
|
||
Thanks Marco & Nathan will post to the thread.
Comment 24•12 years ago
|
||
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•12 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.
Comment 26•12 years ago
|
||
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•12 years ago
|
||
As reference, http://blog.mozilla.org/nfroyd/2012/05/14/statement-wrappers-have-been-deprecated/
Updated•12 years ago
|
Flags: in-testsuite+
Comment 28•12 years ago
|
||
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 }
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•