{ From Ere Maijala 2008-09-14 10:33:08 PDT > (In reply to comment #6) > > I wonder if |mDataFlavors| itself really needs to be a pointer, > > or if it could simply be a direct part of the |nsDataObj| class ? > > <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h> I can't see any reason why it should be a pointer. }

(In reply to comment #0) This was a "copy" of bug 78388 comment 21.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080917032624 Minefield/3.1b1pre] (home, optim default) (W2Ksp4) *Remove |*| from 2 variables. *I'd like to update |// we own and its contents|, but I don't know what it should say... *Remove unused |FILETIME ft|. *Cleanup headers. (Compiles with VC8 Express + PSDK 2003R2.) Please, double check... *A few other nits.

Comment on attachment 339001 [details] [diff] [review] (Av1) <nsDataObj*.*> > *Remove |*| from 2 variables. > *I'd like to update |// we own and its contents|, > but I don't know what it should say... Say nothing, it would be just stating the obvious. One thing I started wondering is if nsDataObj is copied around a lot. Perhaps that's why there's just a pointer? Otherwise I'm failing to why m_enumFE would be a pointer either.

Av1, with comment 3 suggestion(s). (In reply to comment #3) > (From update of attachment 339001 [details] [diff] [review]) > One thing I started wondering is if nsDataObj is copied around a lot. Perhaps Not too sure how to check this, but I would say it doesn't look like it is the case !? http://mxr.mozilla.org/mozilla-central/search?string=nsDataObj[^C]&regexp=1&case=1 http://mxr.mozilla.org/mozilla-central/search?string=nsDataObjCollection&case=on > that's why there's just a pointer? Otherwise I'm failing to why m_enumFE would > be a pointer either. Only because its constructor takes an argument !? http://mxr.mozilla.org/mozilla-central/search?string=m_enumFE&case=on&find=%2Fwidget%2Fsrc%2Fwindows%2FnsDataObj |m_enumFE = new CEnumFormatEtc(32);|

Comment on attachment 339604 [details] [diff] [review] (Av1a) <nsDataObj*.*> sr=mats No need to worry about these objects being copied around - neither class has a working copy constructor.

(In reply to comment #4) > > that's why there's just a pointer? Otherwise I'm failing to why m_enumFE would > > be a pointer either. > > Only because its constructor takes an argument !? Not a problem, it could be added to the initializer list of nsDataObj's constructor. It would require moving |#include "IEnumFE.h"| to nsDataObj.h, don't know if that'd matter.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080929201016 Minefield/3.1b1pre] (home, optim default) (W2Ksp4) Av1a, with comment 6 suggestion(s). Compiles, seems to work: double check please.

Comment on attachment 340963 [details] [diff] [review] (Av1b) <nsDataObj*.*> You should remove the following comment (twice) // Ownership Rules: // nsDataObj owns and ref counts CEnumFormatEtc, because we don't ref-count this member (and must not!). Since CEnumFormatEtc is a ref-counting class I find it slightly ugly to use it as a non-heap-allocated member like this, but it should work as long as we don't ref-count it, but I'd like a comment that says it must never be ref-counted. Unrelated to your patch, but it seems like g_cRef, GetCumRefCount() and GetRefCount() is dead code in nsDataObj* Feel free to nuke them while you're here. sr=mats with the comment added

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080929201016 Minefield/3.1b1pre] (home, optim default) (W2Ksp4) Av1b, with comment 8 suggestion(s). (In reply to comment #8) > Since CEnumFormatEtc is a ref-counting class I find it slightly > ugly to use it as a non-heap-allocated member like this, No idea, just did what Ere suggested. > it seems like g_cRef, GetCumRefCount() and GetRefCount() is dead code I removed (unused member) |mStringData| too. (compiled but not run)

(In reply to comment #9) > (In reply to comment #8) > > Since CEnumFormatEtc is a ref-counting class I find it slightly > > ugly to use it as a non-heap-allocated member like this, > > No idea, just did what Ere suggested. And I somehow failed to realize it's ref-counted which is causing me to doubt my suggestion. How about we just go back to Av1a and forget about this?

Av1c, with comment 10 suggestion(s). Keeping "mats.palmgren: superreview+".

Comment on attachment 341826 [details] [diff] [review] (Av1a2) <nsDataObj*.*> [Checkin: Comment 13] Sorry for my slowliness...

Comment on attachment 341826 [details] [diff] [review] (Av1a2) <nsDataObj*.*> [Checkin: Comment 13] http://hg.mozilla.org/mozilla-central/rev/0720b26b66df