395349 - Move nsDeque to xpcom glue

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[aaronr]

As part of porting XForms to libxul, we found that a class that we are dependent on, nsDeque, isn't available in libxul.  bsmedberg has agreed that we can move nsDeque into xpcom/glue.

Comment 1

[aaronr]

Attachment: patch

Comment 2

[Benjamin Smedberg]

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

>Index: xpcom/glue/nsDeque.h

>+class NS_COM nsDeque {

>+class NS_COM nsDequeIterator {

These need to be NS_COM_GLUE now.

Comment 3

[aaronr]

checked into trunk

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  This was done in about the worst possible way imaginable:

1)  You didn't get a server-side CVS copy done, so you lost the CVS blame
2)  You didn't replicate the checkins in the new location, so you lost the CVS
    log
3)  Your CVS comment doesn't even say where the file moved from
    (xpcom/ds/nsDeque.cpp and xpcom/ds/nsDeque.h), necessitating going to this
    bug and reading the patch to figure out where to look for the infomation you
    decided to throw away.

Frankly, I don't see why this patch got review the way it did.  This should have been done via CVS copy, so as not to waste the time of everyone after you who needs to see the CVS log for nsDeque.

Comment 5

[aaronr]

(In reply to comment #4)
> OK.  This was done in about the worst possible way imaginable:
> 
> 1)  You didn't get a server-side CVS copy done, so you lost the CVS blame
> 2)  You didn't replicate the checkins in the new location, so you lost the CVS
>     log
> 3)  Your CVS comment doesn't even say where the file moved from
>     (xpcom/ds/nsDeque.cpp and xpcom/ds/nsDeque.h), necessitating going to this
>     bug and reading the patch to figure out where to look for the infomation
> you
>     decided to throw away.
> 
> Frankly, I don't see why this patch got review the way it did.  This should
> have been done via CVS copy, so as not to waste the time of everyone after you
> who needs to see the CVS log for nsDeque.
> 

I don't mind being called a stupid ass if I should have known better.  But I'm finding very little documentation on mozilla.org as to using cvs copy to move a file from one place to another.  Could you please point me to the documentation that I obviously should have known but didn't?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Aaron, I wasn't trying to come down on you, and I'm sorry if it came across this way.  In this case the reviewer should have known better, imo.  You're correct that this is not well-documented, at least in part because we don't really have any "you must read this before getting CVS access" documentation that I know of.  Which is too bad.  We should (and it should include this item until we move to hg or whatnot).  But module owners should know things well enough to know that we don't just lose revision history at will.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Though item 3, in the absence of knowing about CVS copy, seems like common sense to me...

Comment 8

[Benjamin Smedberg]

I do not think that it was necessary in this case to preserve history... this is a small file. I should have required (for both license and archaeology reasons) that the file state that it was copied from xpcom/ds).

Comment 9

[Brendan Eich [:brendan]]

Dave, where is your cvs move magic documented?

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

It's not too late to get the CVS comments fixed, at least.  File a bug to cvs admin them to include that information, please.

Comment 11

[Boris Zbarsky [:bzbarsky]]

And for what it's worth, I've already wasted about 30 minutes today due to the history loss this move caused.  I fully expect that this will get worse as time goes on.  I don't really see what "it's a small file" has to do with anything, to be honest.

Comment 12

[aaronr]

Sorry to be so pissy, bz, and for causing extra work for others.  The last time
I needed to move stuff in cvs was like 10 years ago and we just plain lost file
history.  I had no idea there was an alternative.

I'd suggest that a good place to mention how to move files around (and why to
move as opposed to removing and adding) would be at:
http://www.mozilla.org/contribute/writing/cvs and/or
http://developer.mozilla.org/en/docs/Creating_a_patch.  I believe both are in
the 'required reading' list for contributors.

I'll file the bug for including the move information.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Honestly, I'd really like to see nsDeque die. Preferably through a painful death. It has a horrible interface an even worse implementation. What we really should have is something based on nsTArray.

But i guess we can do this for now. Though ideally I'd like to see big warnings put at the top of the files saying that they are deprecated and will die. Painfully.