Open Bug 361248 Opened 18 years ago Updated 12 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
minor

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

Attachments

(3 files, 6 obsolete files)

 
Attached patch (Av1-SM) <msgMail3PaneWindow.js> (obsolete) — Splinter Review
mail/ -> mailnews/ whitespace cleanup,
and a few code/comment rewrites.
(for future checkin)
Assignee: mail → sgautherie.bz
Status: NEW → ASSIGNED
Av1-SM, trimmed for review(s).
Attachment #245999 - Flags: superreview?(neil)
Attachment #245999 - Flags: review?
Attachment #245999 - Flags: review? → review?(iann_bugzilla)
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.
Attachment #245999 - Flags: superreview?(neil) → superreview+
Attachment #245999 - Flags: review?(iann_bugzilla) → review+
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.
Attachment #245998 - Attachment is obsolete: true
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.
Attachment #246121 - Attachment description: (Av1a-SM) <msgMail3PaneWindow.js> → (Av1a-SM) <msgMail3PaneWindow.js> [Checkin: Comment 5]
Attachment #246121 - Flags: approval-seamonkey1.1?
Attachment #246121 - Flags: approval-seamonkey1.1? → approval-seamonkey1.1+
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.
(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 :-<)
> 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 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.
Attachment #246121 - Flags: approval-seamonkey1.1+ → approval-seamonkey1.1-
(In reply to comment #9)
> Am I right that this has been fixed on trunk?

No, 1+ patches should follow.
Attached patch (Bv1-SM) <msgMail3PaneWindow.js> (obsolete) — Splinter Review
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.
Attachment #285816 - Flags: superreview?(neil)
Attachment #285816 - Flags: review?(iann_bugzilla)
Attached patch (Cv1-TB) <msgMail3PaneWindow.js> (obsolete) — Splinter Review
Minor mailnews/ -> mail/ sync,
partly from Av1a-SM patch.
Attachment #285821 - Flags: superreview?(mscott)
Attachment #285821 - Flags: review?(mscott)
Attachment #285821 - Flags: superreview?(mscott)
Attachment #285821 - Flags: superreview+
Attachment #285821 - Flags: review?(mscott)
Attachment #285821 - Flags: review+
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;
Attachment #285816 - Flags: superreview?(neil) → superreview+
Attached patch (Bv2-SM) <msgMail3PaneWindow.js> (obsolete) — Splinter Review
Bv1-SM, updated per comment 13,
and a little more.
Attachment #285816 - Attachment is obsolete: true
Attachment #285882 - Flags: superreview?(neil)
Attachment #285882 - Flags: review?(iann_bugzilla)
Attachment #285816 - Flags: review?(iann_bugzilla)
Attachment #285882 - Flags: superreview?(neil) → superreview+
Attachment #285882 - Flags: review?(iann_bugzilla) → review+
Bv2-SM, with 3 function optimizations (as I did in Av1-SM).
(Sorry, that's a long time since I have worked on this :-<)
Attachment #285882 - Attachment is obsolete: true
Attachment #285937 - Flags: superreview?(neil)
Attachment #285937 - Flags: review?(iann_bugzilla)
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.
Attachment #285937 - Flags: superreview?(neil) → superreview+
Bv1-SM, sync'ed with Bv3-SM.
(Sorry, that's a long time since I have worked on this :-<)
Attachment #285821 - Attachment is obsolete: true
Attachment #285942 - Flags: superreview?(mscott)
Attachment #285942 - Flags: review?(mscott)
(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...
Attachment #285937 - Flags: review?(iann_bugzilla) → review+
Keywords: checkin-needed
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!
> is there a code change in here too?

Scott, there are some minor comment changes, and 3 trivial function optimizations.
Attachment #286663 - Flags: superreview?(mscott)
Comment on attachment 286663 [details] [diff] [review]
(Cv2-TB_Bw) <msgMail3PaneWindow.js> (for review only)

thanks
Attachment #286663 - Flags: superreview?(mscott) → superreview+
Comment on attachment 285942 [details] [diff] [review]
(Cv2-TB) <msgMail3PaneWindow.js>
[Checkin: comment 26]

(Still need a r+ on either of the patches)
Attachment #285942 - Flags: superreview?(mscott)
Whiteboard: [c-n: Bv3-SM]
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
}}
Attachment #285937 - Attachment description: (Bv3-SM) <msgMail3PaneWindow.js> → (Bv3-SM) <msgMail3PaneWindow.js> [Checkin: comment 23]
Keywords: checkin-needed
Whiteboard: [c-n: Bv3-SM]
Target Milestone: --- → seamonkey2.0alpha
(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].
Blocks: 365049
Serge, can we clear this review request? I reviewed the -uw version of the same patch didn't I?
Attachment #285942 - Flags: review?(mscott)
Keywords: checkin-needed
Whiteboard: [c-n: Cv2-TB, comment 25]
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.
Keywords: checkin-needed
Whiteboard: [c-n: Cv2-TB, comment 25]
Attachment #285942 - Attachment description: (Cv2-TB) <msgMail3PaneWindow.js> → (Cv2-TB) <msgMail3PaneWindow.js> [Checkin: comment 26]
No longer blocks: 365049
Depends on: 365049
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.
Attachment #245999 - Attachment is obsolete: true
Attachment #286663 - Attachment is obsolete: true
Target Milestone: seamonkey2.0a1 → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: