Closed Bug 239115 Opened 19 years ago Closed 19 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) — 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) — 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 v2Splinter 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: 19 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.