Closed Bug 239115 Opened 21 years ago Closed 21 years ago

deCOM nsDownload

Categories

(SeaMonkey :: Download & File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: Biesinger)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) (deleted) — Splinter Review
Assignee: download-manager → cbiesinger
Status: NEW → ASSIGNED
Attachment #145042 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 145042 [details] [diff] [review] patch Would it be useful to inline some of these?
Comment on attachment 145042 [details] [diff] [review] patch hm, probably
Attachment #145042 - Attachment is obsolete: true
Attachment #145042 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
now with inlines
Attachment #145646 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 145646 [details] [diff] [review] patch v2 >+ if (observer) { > rv = observer->Observe(NS_STATIC_CAST(nsIDownload*, internalDownload), "oncancel", nsnull); >- if (NS_FAILED(rv)) return rv; > } You might want to get rid of the {}, it looks like local style here...
Attachment #145646 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #145646 - Flags: superreview?(roc)
+ void GetTransferInformation(PRInt32* aCurr, PRInt32* aMax) { A slightly better way to do this, IMHO, is to define a little struct: struct TransferInformation { PRInt32 mCurrBytes, mMaxBytes; TransferInformation(PRInt32 aCurr, PRInt32 aMax) { mCurrBytes = aCurr; mMaxBytes = aMax; } }; TransferInformation GetTransferInformation() { return TransferInformation(mCurrBytes, mMaxBytes); }
Comment on attachment 145646 [details] [diff] [review] patch v2 hm, yeah, I like that
Attachment #145646 - Attachment is obsolete: true
Attachment #145646 - Flags: superreview?(roc)
Attached patch patch v2 (deleted) — Splinter Review
now uses a TransferInformation struct
Attachment #145957 - Flags: superreview?(roc)
Comment on attachment 145957 [details] [diff] [review] patch v2 Looks great. Who owns this code? I can't really pretend I'm an owner or peer here.
Attachment #145957 - Flags: superreview?(roc) → superreview+
neil is an xpfe peer; so his review should be valid as moa, I'd think
oh yeah. Go ahead and check in, then.
Checking in src/nsDownloadManager.cpp; /cvsroot/mozilla/xpfe/components/download-manager/src/nsDownloadManager.cpp,v <-- nsDownloadManager.cpp new revision: 1.92; previous revision: 1.91 done Checking in src/nsDownloadManager.h; /cvsroot/mozilla/xpfe/components/download-manager/src/nsDownloadManager.h,v <-- nsDownloadManager.h new revision: 1.31; previous revision: 1.30 done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
so, what about the download manager code in the new toolkit? does it get no similar loving?
not from me :) I'm doing these download manager patches because I want to implement download resuming for seamonkey, and as firefox has a forked and (I presume; the ui certainly looks like it's quite different...) much changed download manager, I don't want to do that work twice, for a browser that I don't use and don't much care about...
/sigh/ ... the code in toolkit/components/downloads is a fork of the code in xpfe/components/download-manager. it is very similar, sharing most of the same code that you changed in this patch.
Maybe the code should not have been forked. In any case, re: comment 14, SeaMonkey is on sustaining engineering footing with mozilla.org. It should not be where people put energy. For one thing, there are distributors and legacy customers who don't want risk and innovation, they want the same old same old. If Neil is ok with the patch here, I'm ok with it, but I am not going to endorse random, patch-wise innovation in general. /be
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: