Closed
Bug 239115
Opened 21 years ago
Closed 21 years ago
deCOM nsDownload
Categories
(SeaMonkey :: Download & File Handling, defect)
SeaMonkey
Download & File Handling
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Biesinger, Assigned: Biesinger)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•21 years ago
|
||
Assignee: download-manager → cbiesinger
Status: NEW → ASSIGNED
Assignee | ||
Updated•21 years ago
|
Attachment #145042 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 2•21 years ago
|
||
Comment on attachment 145042 [details] [diff] [review]
patch
Would it be useful to inline some of these?
Assignee | ||
Comment 3•21 years ago
|
||
Comment on attachment 145042 [details] [diff] [review]
patch
hm, probably
Attachment #145042 -
Attachment is obsolete: true
Attachment #145042 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 4•21 years ago
|
||
now with inlines
Assignee | ||
Updated•21 years ago
|
Attachment #145646 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 5•21 years ago
|
||
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+
Assignee | ||
Updated•21 years ago
|
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);
}
Assignee | ||
Comment 7•21 years ago
|
||
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)
Assignee | ||
Comment 8•21 years ago
|
||
now uses a TransferInformation struct
Assignee | ||
Updated•21 years ago
|
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+
Assignee | ||
Comment 10•21 years ago
|
||
neil is an xpfe peer; so his review should be valid as moa, I'd think
oh yeah. Go ahead and check in, then.
Assignee | ||
Comment 12•21 years ago
|
||
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
Comment 13•21 years ago
|
||
so, what about the download manager code in the new toolkit? does it get no
similar loving?
Assignee | ||
Comment 14•21 years ago
|
||
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...
Comment 15•21 years ago
|
||
/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.
Comment 16•21 years ago
|
||
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
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•