584316 - Fix DownloadManager usage of deprecated Storage binding APIs

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[timeless]

bug 507414 declared deprecated a bunch of binding functions download manager uses,
you should update to the new code.

storage/public/mozIStorageBaseStatement.idl (View Hg log or Hg annotations)
line 91 -- [deprecated] void bindStringParameter(in unsigned long aParamIndex,

toolkit/components/downloads/src/nsDownloadManager.cpp (View Hg log or Hg annotations)
line 742 -- rv = stmt->BindStringParameter(i++, aName);
line 754 -- rv = stmt->BindStringParameter(i++, aTempPath);
line 3008 -- nsresult rv = stmt->BindStringParameter(i++, tempPath);

Comment 1

[timeless]

Attachment: untested

Comment 2

[Ed Lee :Mardak]

Comment on attachment 463347 [details] [diff] [review]
untested

>-    "VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)"), getter_AddRefs(stmt));
Woohoo!

Comment 3

[Shawn Wilsher :sdwilsh]

actually, we need to fix the UI here too...

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: ui v1.0 (checked in)

Comment 5

[Ed Lee :Mardak]

(In reply to comment #4)
> Created attachment 521875 [details] [diff] [review]
The other patch for this bug switches to binding by name instead of index, but it doesn't really make much sense here as the names would probably be state_notstarted, state_downloading; or state0, state1.

Probably not too much of an overhead, but couldn't we just hardcode the values into the query instead of dynamically binding each time? At least only reference  nsIDM.DOWNLOAD_NOTSTARTED at query creation time?

[Side note.. turns out gStmt is only used in downloads.js ! :p]

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Probably not too much of an overhead, but couldn't we just hardcode the values
> into the query instead of dynamically binding each time? At least only
> reference  nsIDM.DOWNLOAD_NOTSTARTED at query creation time?
We could, but I don't see much value to doing so.  The general practice is to *always* bind so you are always safe.  In places, we enforce that whenever possible.

Comment 7

[Ed Lee :Mardak]

Comment on attachment 521875 [details] [diff] [review]
ui v1.0 (checked in)

Just checking, there isn't any magic for binding an array of ints for bindParameters?

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Just checking, there isn't any magic for binding an array of ints for
> bindParameters?
Not quite, but it's something I'd like to make happen.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 521875 [details] [diff] [review]
ui v1.0 (checked in)

I forgot there were two patches to this bug...I landed this patch:
http://hg.mozilla.org/mozilla-central/rev/a76f473d9f99

Comment 10

[(no longer active)]

Part 1 landed on cedar: http://hg.mozilla.org/projects/cedar/rev/9e9e1e3a05a1

Comment 11

[Henri Sivonen (:hsivonen) (away from Bugzilla until 2025-10-27)]

Backed out due to orange:
http://hg.mozilla.org/projects/cedar/rev/9b459316a572
http://hg.mozilla.org/projects/cedar/rev/7edef9fb11df

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> Backed out due to orange:
> http://hg.mozilla.org/projects/cedar/rev/9b459316a572
> http://hg.mozilla.org/projects/cedar/rev/7edef9fb11df
What was the orange?  Linking to logs when something is backed out is often helpful...

Comment 13

[(no longer active)]

(In reply to comment #12)
> (In reply to comment #11)
> > Backed out due to orange:
> > http://hg.mozilla.org/projects/cedar/rev/9b459316a572
> > http://hg.mozilla.org/projects/cedar/rev/7edef9fb11df
> What was the orange?  Linking to logs when something is backed out is often
> helpful...

http://tbpl.mozilla.org/?tree=Cedar&rev=bbd0f34afe0f

Comment 14

[Shawn Wilsher :sdwilsh]

Looks to me like nsDownloadManager::CancelDownload is broken in this patch

Comment 15

[timeless]

i won't be able to fix this up. sorry. my guess is that it's relatively easy for someone else to adopt.

Comment 16

[Shawn Wilsher :sdwilsh]

Attachment: tested

Pushed to try server: http://tbpl.mozilla.org/?tree=Try&rev=8bd5fcabd486

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 531167 [details] [diff] [review]
tested

Review of attachment 531167 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +1666,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>    for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(states); ++i) {
> +    rv = stmt->BindInt32ByIndex(0, states[i]);

sometimes I'm an idiot...

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: tested for realz

Now that I've run all the tests that were failing before on try locally, all the errors have been fixed.  This is ready to go.

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/aaaed66cef2e