361248 - Synchronize mail/ <-> mailnews/ <msgMail3PaneWindow.js>

Status: ASSIGNED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Serge Gautherie (:sgautherie)]

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-SM) <msgMail3PaneWindow.js>

mail/ -> mailnews/ whitespace cleanup,
and a few code/comment rewrites.
(for future checkin)

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1_Bw-SM) <msgMail3PaneWindow.js> (for review only)

Av1-SM, trimmed for review(s).

Comment 3

[[e-mail]]

Comment on attachment 245999 [details] [diff] [review]
(Av1_Bw-SM) <msgMail3PaneWindow.js> (for review only)

>-      if (event.originalTarget.localName == "treecol")
>+      if (event.originalTarget.localName == "treecol") {
I find this change hard to believe, I always thought that scott and david like to put their {s on their own line.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a-SM) <msgMail3PaneWindow.js> [Checkin: Comment 5]

Av1-SM, with comment 3 suggestion(s).
(for checkin)

NB: I had copied the mail/ line, but I agree that it was not the main style of that file.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 246121 [details] [diff] [review]
(Av1a-SM) <msgMail3PaneWindow.js>
[Checkin: Comment 5]


Checkin: {
2006-11-21 03:05	neil%parkwaycc.co.uk 	mozilla/mailnews/base/resources/content/msgMail3PaneWindow.js 	1.297
}


'approval-seamonkey1.1=?':
Trivial U.I. code cleanup, no risk.

Comment 6

[Andrew Schultz]

Comment on attachment 246121 [details] [diff] [review]
(Av1a-SM) <msgMail3PaneWindow.js>
[Checkin: Comment 5]

this
1) does not apply cleanly to branch
2) does not synchronize the mail/ and mailnews/ versions of the file (they're quite a bit different code-wise).
3) some of the capitalization changes actually move away from what the mail/ version has.

I don't really know why (even without the above problems) we'd want this for the branch.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> (From update of attachment 246121 [details] [diff] [review] [edit])
> this
> 1) does not apply cleanly to branch

May be: I had not requested branch checkin for this (Trunk) patch.

> 2) does not synchronize the mail/ and mailnews/ versions of the file (they're
> quite a bit different code-wise).

True: This should be a first cleanup only, before looking into the real code differences.

> 3) some of the capitalization changes actually move away from what the mail/
> version has.

Sure: I have not posted the TB patch yet, as I was waiting for another bug/patch checkin to its file.

> I don't really know why (even without the above problems) we'd want this for
> the branch.

Well, I don't see why we would not ;-|

(In short, while I often have to wait for someone to checkin my patches;
this time, it seems you tried to apply something not (yet) made to be :-<)

Comment 8

[Andrew Schultz]

> May be: I had not requested branch checkin for this (Trunk) patch.

You requested approval.  You shouldn't request branch approval for a patch that shouldn't land on that branch.

> Well, I don't see why we would not ;-|

We are near releasing 1.1 (there are still issues, so it's not something that will happen tomorrow).  The branch is for low risk / high reward patches.  The patch here has very little reward (although it is low risk).

Comment 9

[Robert Kaiser]

Comment on attachment 246121 [details] [diff] [review]
(Av1a-SM) <msgMail3PaneWindow.js>
[Checkin: Comment 5]

revoking approval, does not improve branch in any useful way (probably nice cleanup, but unneeded on a branch) - esp. as patch developer says "I had not requested branch checkin for this (Trunk) patch."

Am I right that this has been fixed on trunk? If so, please mark the bug as FIXED as it should be.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> Am I right that this has been fixed on trunk?

No, 1+ patches should follow.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM) <msgMail3PaneWindow.js>

New mail/ -> mailnews/ whitespace sync,
and 2 code rewrites:
*line 675: add a semicolon at the end of the line.
*line 882: split the line in two.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-TB) <msgMail3PaneWindow.js>

Minor mailnews/ -> mail/ sync,
partly from Av1a-SM patch.

Comment 13

[[e-mail]]

Comment on attachment 285816 [details] [diff] [review]
(Bv1-SM) <msgMail3PaneWindow.js>

>              {
>                 gDefaultSearchViewTerms = null;
>                 viewDebug("searching gVirtualFolderTerms\n");
>-                 gDBView.viewFolder = gMsgFolderSelected;
>-                 ViewChangeByFolder(gMsgFolderSelected);
>+                gDBView.viewFolder = gMsgFolderSelected;
>+                ViewChangeByFolder(gMsgFolderSelected);
>              }
That's not exactly better, since the indent should be 2 spaces.

On the lines you're touching anyway, please a) add a space after if, for etc. b) put return on its own line, e.g.

>+  if(gThreadAndMessagePaneSplitter) return gThreadAndMessagePaneSplitter;
if (gThreadAndMessagePaneSplitter)
  return gThreadAndMessagePaneSplitter;

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2-SM) <msgMail3PaneWindow.js>

Bv1-SM, updated per comment 13,
and a little more.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Bv3-SM) <msgMail3PaneWindow.js> [Checkin: comment 23]

Bv2-SM, with 3 function optimizations (as I did in Av1-SM).
(Sorry, that's a long time since I have worked on this :-<)

Comment 16

[[e-mail]]

Comment on attachment 285937 [details] [diff] [review]
(Bv3-SM) <msgMail3PaneWindow.js>
[Checkin: comment 23]

> }
> function GetTotalCountElement()
Sorry I didn't notice before, but this could do with a blank line too.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2-TB) <msgMail3PaneWindow.js> [Checkin: comment 26]

Bv1-SM, sync'ed with Bv3-SM.
(Sorry, that's a long time since I have worked on this :-<)

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> (From update of attachment 285937 [details] [diff] [review])
> > }
> > function GetTotalCountElement()
> Sorry I didn't notice before, but this could do with a blank line too.

Agreed, yet let's settle for the current patch for now;
I'll do this in a next (code sync') patch...

Comment 19

[Scott MacGregor]

Comment on attachment 285942 [details] [diff] [review]
(Cv2-TB) <msgMail3PaneWindow.js>
[Checkin: comment 26]

serge, is this change purely white space or is there a code change in here too? Thanks!

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2-TB_Bw) <msgMail3PaneWindow.js> (for review only)

> is there a code change in here too?

Scott, there are some minor comment changes, and 3 trivial function optimizations.

Comment 21

[Scott MacGregor]

Comment on attachment 286663 [details] [diff] [review]
(Cv2-TB_Bw) <msgMail3PaneWindow.js> (for review only)

thanks

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 285942 [details] [diff] [review]
(Cv2-TB) <msgMail3PaneWindow.js>
[Checkin: comment 26]

(Still need a r+ on either of the patches)

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 285937 [details] [diff] [review]
(Bv3-SM) <msgMail3PaneWindow.js>
[Checkin: comment 23]

{{
2007-11-04 06:14	bugzilla%standard8.plus.com 	mozilla/mailnews/base/resources/content/msgMail3PaneWindow.js 	1.316
}}

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> (In reply to comment #16)
> > (From update of attachment 285937 [details] [diff] [review] [details])
> > > }
> > > function GetTotalCountElement()
> > Sorry I didn't notice before, but this could do with a blank line too.
> 
> I'll do this in a next (code sync') patch...

Done, in bug 365049 attachment 287347 [details] [diff] [review].

Comment 25

[Scott MacGregor]

Serge, can we clear this review request? I reviewed the -uw version of the same patch didn't I?

Comment 26

[Reed Loden [:reed]]

Cv2-TB:

Checking in mail/base/content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mail/base/content/msgMail3PaneWindow.js,v  <--  msgMail3PaneWindow.js
new revision: 1.139; previous revision: 1.138
done

Leaving open, as I'm not sure if this is done or not.

Comment 27

[Robert Kaiser]

This bug is open but targeted for seamonkey2.0a1, which has been released a long time ago. Please set the target milestone to an appropriate value, "---" if it has no specific target.