Closed Bug 645049 Opened 9 years ago Closed 2 years ago

Remove [deprecated] methods on mozIStorageBaseStatement.idl

Categories

(Toolkit :: Storage, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: sdwilsh, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

There aren't many uses left in the tree.  This has been marked as deprecated for a while now, so it's time to nuke them.
Depends on: 584316
This updates all the tests to use the newer API.  Going to push this all to try at the same time, but things passed locally.  I probably don't need review for this change, but I figure it doesn't hurt to ask.
Attachment #521862 - Flags: review?(dtownsend)
These were actually marked as deprecated in bug 507414 which landed in 1.9.3a3.  We have only shipped one major version with the new API, but I still think we should remove the old one.
Depends on: 645086
Depends on: 645089
Depends on: 645092
Depends on: 645093
Depends on: 645094
Depends on: 645099
Depends on: 645101
All dependent bugs now have patches up.  When those all land, we can actually do this!
Attachment #521930 - Flags: superreview?(robert.bugzilla)
Attachment #521930 - Flags: review?(bugmail)
Will this resolve bug 589032 and bug 589035?
(In reply to comment #5)
> Will this resolve bug 589032
No, that's different.

> and bug 589035?
Bug 645094 does.
Whiteboard: [needs review asuth][needs review Mossop][needs sr rstrong]
Comment on attachment 521862 [details] [diff] [review]
Fix tests v1.0 (checked in)

Have you done a search on AMO to figure out how many add-ons will get hurt by this?
Attachment #521862 - Flags: review?(dtownsend) → review+
(In reply to comment #7)
> Have you done a search on AMO to figure out how many add-ons will get hurt by
> this?
bindUTF8StringParameter: Too many hits, displaying the first 1000
bindStringParameter: Found 876 matching lines in 138 files
bindDoubleParameter: Found 81 matching lines in 34 files
bindInt32Parameter: Too many hits, displaying the first 1000
bindInt64Parameter: Found 767 matching lines in 229 files
bindNullParameter: Found 83 matching lines in 43 files
bindBlobParameter: Found 115 matching lines in 113 files

UTF8String and Int32 parameter are clearly used a lot.  I'm fine with waiting until Firefox 6 to land these changes, but not any longer.  The JS helper (without creating an explicit wrapper) has been available since 1.9.1 (Firefox 3.5) which allows JS consumers to just do stmt.params[index].  I would prefer to land this for Firefox 5, however.
Whiteboard: [needs review asuth][needs review Mossop][needs sr rstrong] → [needs review asuth][needs sr rstrong]
Comment on attachment 521930 [details] [diff] [review]
Drop Methods v1.0

Looks like this will cause a lot of extension breakage so without an overriding reason or mitigation of this breakage in place both along with driver understanding this and approving I don't think this should go in for Firefox 5.

Looks fine otherwise
Attachment #521930 - Flags: superreview?(robert.bugzilla) → superreview+
Whiteboard: [needs review asuth][needs sr rstrong] → [needs review asuth]
Comment on attachment 521930 [details] [diff] [review]
Drop Methods v1.0

Looks technically fine to me.

How do you search extension code on AMO?  That sounds like a neat trick I would like to be able to do.
Attachment #521930 - Flags: review?(bugmail) → review+
(In reply to comment #10)
> How do you search extension code on AMO?  That sounds like a neat trick I would
> like to be able to do.
https://mxr.mozilla.org/addons/ but it does require LDAP authentication.
Keywords: dev-doc-needed
Whiteboard: [needs review asuth] → [land for Firefox 6]
Comment on attachment 521862 [details] [diff] [review]
Fix tests v1.0 (checked in)

http://hg.mozilla.org/mozilla-central/rev/046c166f999f
Attachment #521862 - Attachment description: Fix tests v1.0 → Fix tests v1.0 (checked in)
Not comfortable landing this so late in Firefox 6's cycle.  This will happen for 7.
Whiteboard: [land for Firefox 6] → [land for Firefox 7]
Blocks: 658959
This appears to have slipped the Firefox 7 date by a little bit.  Is this still something that should be landed?
Flags: needinfo?(bugmail)
Whoops!  Guess that whiteboard field did not have magic powers after all.  This may need to be re-audited and such.  :mak is owner now, switching over to him.
Flags: needinfo?(bugmail) → needinfo?(mak77)
We need updated stats to be able to decide how to move.
Surely we still would like to do this, but we might have to reach most add-ons by mail through the AMO team first.

From what I see in
https://mxr.mozilla.org/addons/search?string=bind%23Parameter\(&regexp=on
there are still thousands of consumers, we should try to reduce those, or this would break too many add-ons.

Something we can do in the meanwhile is to fix in-tree usage
http://mxr.mozilla.org/mozilla-central/search?string=bind%23Parameter\(&regexp=1
this part can be a simple mentored task.

Also the mdn documentation should be fixed, if we still suggest using those methods in the docs, it's unlikely add-ons may stop using them...
Flags: needinfo?(mak77)
Keywords: addon-compat
Whiteboard: [land for Firefox 7] → [good first bug][mentor=mak][lang=js]
the first mentored things to do here are file a dependency to fix in-tree usage (and make a patch for that) and update mdn examples/docs.
Assignee: sdwilsh → nobody
Status: ASSIGNED → NEW
Are we showing console warnings when those methods are used? That would also help push add-on devs to look for alternatives.
nope, I'm going to file a bug for that.
Depends on: 1007034
Depends on: 1024360
So now we have a dependency to warn and one to remove in-tree consumers. both are mentored. here we have basically done patches and to update the docs, only the latter is actionable atm.
Whiteboard: [good first bug][mentor=mak][lang=js]
Priority: -- → P3
we can do this from FF57 on.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Blocks: 1387803
Comment on attachment 8894138 [details]
Bug 645049 - Remove [deprecated] methods on mozIStorageBaseStatement.idl.

https://reviewboard.mozilla.org/r/165194/#review170578

It's great to have this done!  Thank you!
Attachment #8894138 - Flags: review?(bugmail) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/46153627ee5f
Remove [deprecated] methods on mozIStorageBaseStatement.idl. r=asuth
https://hg.mozilla.org/mozilla-central/rev/46153627ee5f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.