239115 - deCOM nsDownload

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[[e-mail]]

Comment on attachment 145042 [details] [diff] [review]
patch

Would it be useful to inline some of these?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 145042 [details] [diff] [review]
patch

hm, probably

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

now with inlines

Comment 5

[[e-mail]]

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...

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 145646 [details] [diff] [review]
patch v2

hm, yeah, I like that

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

now uses a TransferInformation struct

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

neil is an xpfe peer; so his review should be valid as moa, I'd think

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oh yeah. Go ahead and check in, then.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 13

[Darin Fisher]

so, what about the download manager code in the new toolkit?  does it get no
similar loving?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

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

[Darin Fisher]

/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

[Brendan Eich [:brendan]]

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