438342 - Build the download list asynchronously

Status: NEW

(Toolkit :: Downloads API, defect)

Description

[Shawn Wilsher :sdwilsh]

Let's go ahead and use the new storage API for this instead of doing it by hand (and not technically async either)

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Ran into a weird issue with some of our tests.  It seems that we need the endTime in the DB with this patch, and didn't before.  Not 100% sure why though.

The API for async statements may change, but this passes all of our tests, and I can at least get a basic review done.

Comment 2

[Shawn Wilsher :sdwilsh]

Edward - was there a reason why we didn't have the downloadMatchesSearch function be done through SQLite?  We could put even more work to be done on the background by doing this.

Comment 3

[Ed Lee :Mardak]

We need to create a download item to determine if it matches the search. There's text attributes like the status that get generated by javascript.

Comment 4

[Shawn Wilsher :sdwilsh]

oh bother - that's right.  I don't think some of the interfaces we use are threadsafe either, so we couldn't call them on the background to get the right text anyway.  Might be something to look into for the future still.

Comment 5

[Ed Lee :Mardak]

Comment on attachment 324497 [details] [diff] [review]
v1.0

Where are the interface files for the new async API stuff? or just look at..
http://developer.mozilla.org/en/docs/User_talk:Comrade693#storageIStatementCallback

Any idea how this actually performs? The bottleneck is updating the DOM when adding the item to the document. (Not querying or creating the item node.) I'm curious what happens when the DOM is updated async.. would the UI thread get pounded as it tries to update the list?

>@@ -1136,92 +1143,85 @@ function buildDownloadList(aForceBuild)
>-  // Take a quick break before we actually start building the list
>-  gBuilder = setTimeout(function() {
>-    // Start building the list and select the first item
>-    stepListBuilder(1);
>-    gDownloadsView.selectedIndex = 0;
>-
>-    // We just tried to add a single item, so we probably need to enable
>-    updateClearListButton();
This updateClearListButton is called right after the first item is added. Notice originally it'll add one item, do a setTimeout and return right away so updateClearListButton happens before the second batch of stepListBuilder happens. This makes it so the clear button can be made clickable right away just after a single item was added.

Perhaps if async update isn't killing the ui thread, might as well just update the clear list button each time. I suppose potentially it could be optimized that once it's been enabled, it stays enabled. (Need to disable it just before building the list then.)

>+let ListBuilder =
>+{
>+  handleError: function DMUI_handleError(aError)
>+  {
>+    Components.utils.reportError(aError); 
nit: We've got Cu set to Components.utils for Cu.import already. :)

...
>+};
Don't we need a QueryInterface for storageIStatementCallback? ;) (I was trying to figure out what this thing was and noticed executeAsync for moz storage stuff didn't exist in the codebase and finally found your dev talk page. :p)

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> (From update of attachment 324497 [details] [diff] [review])
> Where are the interface files for the new async API stuff? or just look at..
> http://developer.mozilla.org/en/docs/User_talk:Comrade693#storageIStatementCallback
Nothing has landed yet - note the dependencies on this bug.

> Any idea how this actually performs? The bottleneck is updating the DOM when
> adding the item to the document. (Not querying or creating the item node.) I'm
> curious what happens when the DOM is updated async.. would the UI thread get
> pounded as it tries to update the list?
This performs way better than it used to, but not good enough yet.  Probably should be a v0.1

> This updateClearListButton is called right after the first item is added.
> Notice originally it'll add one item, do a setTimeout and return right away so
> updateClearListButton happens before the second batch of stepListBuilder
> happens. This makes it so the clear button can be made clickable right away
> just after a single item was added.
Seems like the DOM isn't displayed until the first set of results is completed anyway (the redraw doesn't happen until the main thread can process a new event maybe?).

> Don't we need a QueryInterface for storageIStatementCallback? ;) (I was trying
> to figure out what this thing was and noticed executeAsync for moz storage
> stuff didn't exist in the codebase and finally found your dev talk page. :p)
It turns out XPConnect does that magic for us, and returns an error code if the JS object doesn't implement a certain method.  We can actually remove our import of XPCUtils.jsm in the download manager to help load times.

Comment 7

[Ed Lee :Mardak]

Comment on attachment 324497 [details] [diff] [review]
v1.0

>+++ b/toolkit/mozapps/downloads/content/downloads.js
> let gStmt = gDownloadManager.DBConnection.createStatement(
We probably want createAsyncStatement now.

>+  "SELECT id AS dlid, target AS file, name AS target, source AS uri, state, " +
>+         "round(startTime / 1000) AS startTime, " +
>+         "round(endTime / 1000) AS endTime, referrer, currBytes, maxBytes, " +
>+         "state IN (?1, ?2, ?3, ?4, ?5) isActive " +
And probably update these to named parameters while we touch this. Yay!

>@@ -482,8 +484,11 @@ function Shutdown()
>+  try {
>+    gBuilder.cancel(Cr.NS_ERROR_ABORT);
I believe cancel doesn't take any arguments now. Also this should only throw if we try calling it multiple times? Perhaps we should have a helper cancel function that.. if (gBuilder == null) return; gBuilder.cancel() gBuilder = null;

>+let ListBuilder =
>+{
nit: I believe object { are on the same line
>+  handleResult: function DMUI_handleResult(aResultSet)
>+  {
nit: the file doesn't seem to have any other inline functions but all the top-level ones have { on new line. I believe normal style to have { on the same line as "function" in js?

>+    while ((row = aResultSet.getNextTuple())) {
.getNextRow()

>+  handleCompletion: function DMUI_handleCompletion(aReason)
..
Probably should null out gBuilder here if we do rely on that to .cancel()