Remove download manager search feature

VERIFIED FIXED

Status

VERIFIED FIXED
10 years ago
5 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Trunk
x86
Linux
Dependency tree / graph

Details

Attachments

(1 attachment)

Created attachment 400661 [details] [diff] [review]
patch

Remove the UI and related code. We will likely revisit this feature after 1.0
Attachment #400661 - Flags: review?(gavin.sharp)
Duplicate of this bug: 496896
Duplicate of this bug: 508648
tracking-fennec: --- → ?
Blocks: 508648
Blocks: 496896
Comment on attachment 400661 [details] [diff] [review]
patch

>diff --git a/chrome/content/downloads.js b/chrome/content/downloads.js

>-      // Make the item and add it to the end if it's active or matches the search
>+      // Make the item and add it to the end
>       let item = this._createItem(attrs);
>-      if (item && (isActive || this._matchesSearch(item))) {
>-        // Add item to the end
>-        this._list.appendChild(item);
>-      }
>-      else {
>-        // We didn't add an item, so bump up the number of items to process, but
>-        // not a whole number so that we eventually do pause for a chunk break
>-        aNumItems += .9;
>-      }
>+      this._list.appendChild(item);

I don't understand why removing the isActive check is OK (though this code is hard enough to follow that I'm probably missing something).

>-      this._initStatement(mode.value);

Much of _initStatement is now unused given that we only call it without aMode, so it could be simplified and/or inlined.

Looks good otherwise.
(In reply to comment #3)
> (From update of attachment 400661 [details] [diff] [review])

> I don't understand why removing the isActive check is OK (though this code is
> hard enough to follow that I'm probably missing something).

The check was a bit weird: _createItem can't really fail and without a search, all items will match. The isActive (which was used to add items that were downloading, but not matching the current search) wasn't needed anymore.

> Much of _initStatement is now unused given that we only call it without aMode,
> so it could be simplified and/or inlined.

I can look into that.
Comment on attachment 400661 [details] [diff] [review]
patch

ok, r=me with _initStatement cleaned up or a followup filed.
Attachment #400661 - Flags: review?(gavin.sharp) → review+
I cleaned up _initStatment and pushed:
https://hg.mozilla.org/mobile-browser/rev/cb8d30f915c4
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
verified on my n810 with 20090921 1.9.2 nightly build
Status: RESOLVED → VERIFIED
bugspam
Assignee: nobody → mark.finkle
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.