90741 - XBL <xul:box> tags that don't have inherits="orient" should be changed to <xul:hbox> or <xul:vbox>

Status: NEW

(Core :: XUL, defect)

Description

[Mark Anderson]

This bug tracks finishing off bug 73671.  It will encompass the XBL changes, one 
change that was missed on the trunk, and some mailnews and security changes that 
were left for later due to (now shown to be invalid) bug 88332.  These have all 
been tested locally, but I'm Cc:ing everyone I think would want to test and 
review this stuff.  The major part of this bug are the XBL changes; the rest of 
them are real easy to test and should be able to be cleared quickly.

(Also Cc:ing rginda, to let him know that he can change Venkman at his 
convenience. :) )

Comment 1

[Mark Anderson]

Attachment: Security patch, including one regression fix (every dialog affected has been tested locally)

Comment 2

[Mark Anderson]

Attachment: The one XUL change that was missed in the carpool (pref-applications-new.xul)

Comment 3

[Mark Anderson]

I'm not gonna post a diff for mailnews until the folder outliner lands. :)

Comment 4

[David P. Drinan]

r=ddrinan.

Comment 5

[Joe Hewitt (gone)]

sr=hewitt

Comment 6

[Mark Anderson]

Attachment: XBL changes (as instructed by hewitt) - needs testing from someone who knows more than I about where these widgets are used

Comment 7

[Mark Anderson]

The security changes (r=ddrinan, sr=hewitt) have been checked in.

Still waiting on r= for the other small change (the sr=hewitt was for both this 
and the security patch).

No reviews as yet for the XBL stuff, which is not surprising. :)

Comment 8

[Mark Anderson]

Attachment: Final regressions (lacking mailnews changes touched in folder outliner patch and files with patches already posted)

Comment 9

[Mark Anderson]

I swept through every overlay in the entire tree and found any instance of a 
<vbox> that overlayed an <hbox> in the main file and corrected it (hence the 
latest patch).  Only a couple of these are actually visible in the product (the 
Edit List function in the Address Book, for instance), thanks to additional 
<hbox> or <vbox> elements inside of the opening <vbox>, but these changes are 
good to make nonetheless.

This does not touch pref-applications-new.xul (which already has a patch 
submitted) and the mailnews changes which are contingent on the folder outliner 
(I don't want to screw up their landing by relatively small changes in two XUL 
files).

Comment 10

[Blake Ross]

Seth should look at the mailnews changes, and someone from the editor team
(probably cmanske) needs to see the editor ones. sr=blake for the lone xpfe change

Comment 11

[Håkan Waara]

I would be happy if we could wait with this for a while (at least the mailnews
part)... It's low gain and pretty high risk.

Comment 12

[Mark Anderson]

Fixes for regressions are high risk?

Comment 13

[Håkan Waara]

Last week you horked us... I just want to make sure you don't do that again.
Especially not now when we're anticipating the folder_outliner branch.

Comment 14

[Håkan Waara]

Mark explained to me that the XBL changes are for later.

I looked through the patch from 07/14/01 13:17, and it looks good to me. It
fixes regressions (as opposed to spawning new ones. :)

r=hwaara on that one, (attachment 42301 [details] [diff] [review])

Comment 15

[Charles Manske]

r=cmanske for all composer and mailnews changes. All of this looks good.

Comment 16

[Mark Anderson]

Attachment: Reduced xpfe patch for landing while waiting for sr= on the rest of the regression fixes

Comment 17

[kinmoz]

sr=[e-mail] on the editor changes. 

Comment 18

[Mark Anderson]

Attachment: Editor changes only (r=cmanske,sr=kin)

Comment 19

[Akkana Peck]

I checked the editor patch in to the trunk for Mark, per our IRC conversation.

Comment 20

[Mark Anderson]

Attachment: Final mailnews regression fixes (r=hwaara, awaiting sr=)

Comment 21

[(not reading, please use [e-mail] instead)]

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=43322 seems ok to me.

last time mailnews got horked by innocent looking changes.

please double check that all affected dialogs are not going to be messed up.

after that,
sr=sspitzer

Comment 22

[Mark Anderson]

I double and then triple checked it before hwaara would give me an r=.  It's 
quite well tested. :)

Comment 23

[Mark Anderson]

Attachment: Smaller mailnews patch; doesn't touch any files that folder_outliner will touch (r=hwaara,sr=sspitzer)

Comment 24

[Mark Anderson]

Yay!  All the regression fixes are in (as of timeless' checkin today, July 24)! 
 Updating summary appropriately.

Comment 25

[Mark Anderson]

Attachment: XBL changes

Comment 26

[Mark Anderson]

The most recent attachment is a set of smaller XBL changes that only do three 
things.

1) <xul:box orient="vertical"> -> <xul:vbox>
2) <xul:box orient="horizontal"> -> <xul:hbox>
3) A bunch of minor whitespace fixes (including whacking some tabs)

This excludes the fourth, and much more risky, change of <xul:box> (without 
orient="horizontal") -> <xul:hbox>.  hewitt didn't think there was a risk in 
that, but this new patch is much easier to review (and I've been running with it 
locally for some time with no regressions [actually, I've been running the full 
patch for some time with no regressions] that I could see; there aren't test 
cases for all XBL widgets, though, especially on platforms that I can't test on) 
and is safer.  Unless <xul:box> is going to have an alignment change at some 
point, I'm happy with just doing the smaller change (after the last time and 
causing regressions, I'm being more cautious).  <xul:box> == <xul:hbox>, but 
it's just not worth it.

Comment 27

[Joe Hewitt (gone)]

sr=hewitt

Comment 28

[Joe Hewitt (gone)]

fixed

Comment 29

[Joe Hewitt (gone)]

whoops, not really fixed, I only landed the xbl portion of mark's changes.

Comment 30

[Thomas K. (:tom)]

Resetting assignee to default after no activity for 7 years