645049 - Remove [deprecated] methods on mozIStorageBaseStatement.idl

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P3)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: Fix tests v1.0 (checked in)

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.

Comment 2

[Shawn Wilsher :sdwilsh]

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.

Comment 3

[Shawn Wilsher :sdwilsh]

All dependent bugs now have patches up.  When those all land, we can actually do this!

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: Drop Methods v1.0

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Will this resolve bug 589032 and bug 589035?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Will this resolve bug 589032
No, that's different.

> and bug 589035?
Bug 645094 does.

Comment 7

[Dave Townsend [:mossop]]

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?

Comment 8

[Shawn Wilsher :sdwilsh]

(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.

Comment 9

[Robert Strong (they/them - no direct email)]

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

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 11

[Shawn Wilsher :sdwilsh]

(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.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 521862 [details] [diff] [review]
Fix tests v1.0 (checked in)

http://hg.mozilla.org/mozilla-central/rev/046c166f999f

Comment 13

[Shawn Wilsher :sdwilsh]

Not comfortable landing this so late in Firefox 6's cycle.  This will happen for 7.

Comment 14

[Andrew McCreight [:mccr8]]

This appears to have slipped the Firefox 7 date by a little bit.  Is this still something that should be landed?

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 16

[Marco Bonardo [:mak]]

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...

Comment 17

[Marco Bonardo [:mak]]

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.

Comment 18

[Jorge Villalobos [:jorgev] (he/him)]

Are we showing console warnings when those methods are used? That would also help push add-on devs to look for alternatives.

Comment 19

[Marco Bonardo [:mak]]

nope, I'm going to file a bug for that.

Comment 20

[Marco Bonardo [:mak]]

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.

Comment 21

[Marco Bonardo [:mak]]

we can do this from FF57 on.

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: Bug 645049 - Remove [deprecated] methods on mozIStorageBaseStatement.idl.

Review commit: https://reviewboard.mozilla.org/r/165194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165194/

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

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!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46153627ee5f
Remove [deprecated] methods on mozIStorageBaseStatement.idl. r=asuth

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46153627ee5f