Store autoResume in moz_downloads table to indicate if a download should auto-resume

RESOLVED FIXED in mozilla1.9beta1

Status

()

Toolkit
Downloads API
--
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

Trunk
mozilla1.9beta1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 284880 [details] [diff] [review]
v1

We can use this value to determine if we should auto-resume downloads when quitting the app.

.. no testcase yet.. :p
Attachment #284880 - Flags: review?(comrade693+bmo)
(Assignee)

Updated

10 years ago
Blocks: 399816
(Assignee)

Comment 1

10 years ago
Don't check in before bug 395144 which puts us at schema version 7.
Depends on: 395144
Comment on attachment 284880 [details] [diff] [review]
v1

>+  // Convert the DB's int back into the enum
>+  dl->mAutoStart = nsDownload::AutoStart(stmt->AsInt32(i++));
comment isn't necessary - just toss |enum| before nsDownload::AutoStart

>+  enum AutoStart { DONT_START, AUTO_RESUME };
|enum AutoResume { DONT_RESUME, AUTO_RESUME };|
Attachment #284880 - Flags: review?(comrade693+bmo) → review-
also, lets call the column autoResume (and I noticed we haven't been very consistent with our naming and camelCase / underscores with moz_downloads being the table name...)
(Assignee)

Comment 4

10 years ago
Created attachment 284889 [details] [diff] [review]
v1.1

(In reply to comment #2)
> >+  // Convert the DB's int back into the enum
> >+  dl->mAutoStart = nsDownload::AutoStart(stmt->AsInt32(i++));
> comment isn't necessary - just toss |enum| before nsDownload::AutoStart
Hrm? Like ... = enum nsDownload::AutoResume(val); ??
That doesn't compile.

> >+  enum AutoStart { DONT_START, AUTO_RESUME };
> |enum AutoResume { DONT_RESUME, AUTO_RESUME };|
Changed.

(In reply to comment #3)
> also, lets call the column autoResume
Fixed column and other comments
Attachment #284880 - Attachment is obsolete: true
Attachment #284889 - Flags: review?(comrade693+bmo)
(In reply to comment #4)
> Hrm? Like ... = enum nsDownload::AutoResume(val); ??
> That doesn't compile.
neat - I generally static_cast to the enum actually, which I know compiles
... = static_cast<enum nsDownload::AutoResume>(val);
(Assignee)

Comment 6

10 years ago
Ah, it seems like it was a parsing issue.. these work:

nsDownload::AutoResume(stmt->AsInt32(i++))
(nsDownload::AutoResume)stmt->AsInt32(i++)
(enum nsDownload::AutoResume)stmt->AsInt32(i++)
static_cast<enum nsDownload::AutoResume>(stmt->AsInt32(i++))

Which should I use? The current patch has the first..
note: last one line wraps :p
(In reply to comment #6)
> (nsDownload::AutoResume)stmt->AsInt32(i++)
> (enum nsDownload::AutoResume)stmt->AsInt32(i++)
These are C casts, which I'd rather not do...

> static_cast<enum nsDownload::AutoResume>(stmt->AsInt32(i++))
I'd prefer this, even if there is line wrapping.
(Assignee)

Comment 8

10 years ago
Created attachment 284908 [details] [diff] [review]
v1.2

Now with more testcase! and static_cast
Attachment #284889 - Attachment is obsolete: true
Attachment #284908 - Flags: review?(comrade693+bmo)
Attachment #284889 - Flags: review?(comrade693+bmo)
(Assignee)

Comment 9

10 years ago
Created attachment 284909 [details]
v7.sqlite
Comment on attachment 284908 [details] [diff] [review]
v1.2

> PRBool
>+nsDownload::ShouldAutoResume()
>+{
>+  return mAutoResume== AUTO_RESUME;
nit: space after mAutoResume

>   /**
>+   * Download should try to automatically resume?
>+   */
>+  PRBool ShouldAutoResume();
eh, let's try "Indicates if the download should try to automatically resume or not."

>+  /**
>+   * Track various states of the download trying to auto-resume.
>+   */
>+  enum AutoResume { DONT_RESUME, AUTO_RESUME };
>+  AutoResume mAutoResume;
While it seems pretty clear, would you mind doing a small description of each enum value please?

>+  do_check_true(stmt.getIsNull(11));
>+  do_check_true(stmt.getIsNull(12));
>+  do_check_eq(0, stmt.getInt64(13));
>+  do_check_eq(0, stmt.getInt32(14));
I would love to get a new DB with actual entries for 11 and 12, but it's not a huge deal.

r=sdwilsh
Attachment #284908 - Flags: review?(comrade693+bmo) → review+
(Assignee)

Comment 11

10 years ago
Created attachment 284964 [details]
v7.sqlite v2
Attachment #284909 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
Created attachment 284966 [details] [diff] [review]
v1.3

(In reply to comment #10)
> >+  return mAutoResume== AUTO_RESUME;
> nit: space after mAutoResume
Spaced.

> >+   * Download should try to automatically resume?
> eh, let's try "Indicates if the download should try to automatically resume or
> not."
OK.

> >+  enum AutoResume { DONT_RESUME, AUTO_RESUME };
> While it seems pretty clear, would you mind doing a small description of each
> enum value please?
Described.

> >+  do_check_true(stmt.getIsNull(11));
> >+  do_check_true(stmt.getIsNull(12));
> I would love to get a new DB with actual entries for 11 and 12, but it's not a
> huge deal.
And you get even more than you asked for! r? for testcase v2! Perhaps we should get Brahmana to use this format for the exthandler patch.
Attachment #284908 - Attachment is obsolete: true
Attachment #284966 - Flags: review?(comrade693+bmo)
Comment on attachment 284966 [details] [diff] [review]
v1.3

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
>+ *   Srirang G Doddihal <brahmana@doddihal.com>
>+ *   Edward Lee <edward.lee@engineering.uiuc.edu>
I'm pretty sure you just rewrote the whole file, so feel free to take copyright on this ;)

>+  stmt.reset();
can we add a |stmt.finalize();| here as well please?

r=sdwilsh
Attachment #284966 - Flags: review?(comrade693+bmo) → review+
(Assignee)

Comment 14

10 years ago
Created attachment 285330 [details] [diff] [review]
v1.4

Unbitrot from openwith exthandler changes..

(In reply to comment #13)
> >+ *   Edward Lee <edward.lee@engineering.uiuc.edu>
> I'm pretty sure you just rewrote the whole file
Updated this comment and several other comments.

> >+  stmt.reset();
> can we add a |stmt.finalize();| here as well please?
Finalized.

Additionally, there's a targetVersion variable in the test instead of using 7 then 8 in separate places. And I cleaned up the sqlite file to be smaller. 3k instead of 7k.
Attachment #284966 - Attachment is obsolete: true
Attachment #285330 - Flags: approval1.9?
(Assignee)

Comment 15

10 years ago
Created attachment 285331 [details]
v7.sqlite v3

Seems like interdiff isn't working here.. if you really wanted the diff of diffs between 1.3 and 1.4..

-   // Fallthrough to next upgrade
+     // Fallthrough to next upgrade
--      "preferredAction INTEGER DEFAULT 0"  // This means we default to saveToDisk
-+      "preferredAction INTEGER DEFAULT 0, "  // This means we default to saveToDisk
+-      "preferredAction INTEGER NOT NULL DEFAULT 0"
++      "preferredAction INTEGER NOT NULL DEFAULT 0, "
-     // Compensate for the i++s skipped in the true block 
+     // Compensate for the i++s skipped in the true block
-+
+ 
-   return NS_OK;
-+ * Mozilla Corporation.
++ * Edward Lee <edward.lee@engineering.uiuc.edu>.
-+ *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
-+ *   Srirang G Doddihal <brahmana@doddihal.com>
-+ *   Edward Lee <edward.lee@engineering.uiuc.edu>
-+// This file tests migration from v7 to v8
-+
++  // We're testing migration to this version from one version below
++  var targetVersion = 8;
++
-+  importDatabaseFile("v7.sqlite");
++  importDatabaseFile("v" + (targetVersion - 1) + ".sqlite");
-+  // ok, now it is OK to init the download manager - this will perform the
-+  // migration!
++  // Init the download manager which will try migrating to the new version
-+  var stmt = null;
-+  // check schema version
-+  do_check_true(dbConn.schemaVersion >= 8);
++  // Check schema version
++  do_check_true(dbConn.schemaVersion >= targetVersion);
-+  // now we check the entries
-+  stmt = dbConn.createStatement(
++  // Make sure all the columns are there
++  var stmt = dbConn.createStatement(
++  // This data is based on the original values in the table
++    // For the new columns added, check for null or default values
++  // Make sure the values are correct after the migration
++  stmt.finalize();
-+
Attachment #284964 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][has reviews]

Updated

10 years ago
Attachment #285330 - Flags: approvalM9+
Attachment #285330 - Flags: approval1.9?
Attachment #285330 - Flags: approval1.9+
(Assignee)

Comment 16

10 years ago
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.141; previous revision: 1.140
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.53; previous revision: 1.52
done
RCS file: /cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/test_migration_to_8.js,v
done
Checking in toolkit/components/downloads/test/schema_migration/test_migration_to_8.js;
/cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/test_migration_to_8.js,v  <--  test_migration_to_8.js
initial revision: 1.1
done
Checking in toolkit/components/downloads/test/schema_migration/v7.sqlite;
/cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/v7.sqlite,v  <--  v7.sqlite
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Summary: Store autoStart in moz_downloads table to indicate if a download should auto-resume → Store autoResume in moz_downloads table to indicate if a download should auto-resume
Whiteboard: [has patch][has reviews]
Target Milestone: --- → Firefox 3 M9
(Assignee)

Comment 17

10 years ago
Tests don't work on other machines. Reopening and backing out just the tests.

Removing toolkit/components/downloads/test/schema_migration/test_migration_to_8.js;
/cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/test_migration_to_8.js,v  <--  test_migration_to_8.js
new revision: delete; previous revision: 1.1
done
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 286217 [details] [diff] [review]
test fixup v1.0
Attachment #286217 - Flags: review?(edilee)
Created attachment 286219 [details]
v7.sqlite v4
Attachment #285331 - Attachment is obsolete: true
(Assignee)

Comment 20

10 years ago
Comment on attachment 286217 [details] [diff] [review]
test fixup v1.0

Looks good like bug 395144. Some reason the sqlite file bloats up to 4KB when updating that field. I tried doing it myself and it also went 3KB to 4KB... silly sqlite.

r=edilee or r=Mardak or something..
Attachment #286217 - Flags: review?(edilee) → review+
a=mconnor over irc to land the updated test file and database file
Keywords: checkin-needed
Checking in toolkit/components/downloads/test/schema_migration/test_migration_to_8.js;
/cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/test_migration_to_8.js,v  <--  test_migration_to_8.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/downloads/test/schema_migration/v7.sqlite;
/cvsroot/mozilla/toolkit/components/downloads/test/schema_migration/v7.sqlite,v  <--  v7.sqlite
new revision: 1.3; previous revision: 1.2
done
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.