Closed Bug 511404 Opened 11 years ago Closed 11 years ago

tab related bindings should all move to content/tabmail.xml

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: mkmelin, Assigned: mkmelin)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch proposed fix (obsolete) — Splinter Review
We currently have a tabmailBindings.xml for each theme, but the bindings are virtually the same. They should be unified and put in tabmail.xml instead.

(Also, need to clean up a bit after bug 465324 - see Bug #465324 comment #15.)

So, pinstripe and qute tabmailBindings.xmlversions only had whitespace differences. gnomestripe and qute, only a one bug difference, http://hg.mozilla.org/comm-central/rev/3084

tab-icon wasn't used for anything AFAIKT so i removed it.

Tested only on linux - would be good if someone else can check other platforms didn't break. (Just apply the patch, tabs should look/work the same as before.)
Attachment #395314 - Flags: review?(bugmail)
FTR I was going to remove tab-icon on gnomestripe when I fixed the rest (i.e. the linux part) of bug 502616, but you doing it saves me the effort ;-)
Looks good!

Two comments though: 
1. the overall structure can be simplified, reducing number of boxes and stacks.
2. tab-image-left, -middle, and -right are no longer needed, as border-image can now be used to specify the images to be used for the left/right borders.

Maybe for a next step of performance optimization?
> Maybe for a next step of performance optimization?

Yeah, i like to get this first step in first.
It'll be a few days before I can get to this.  If you can get philor to take it (his queue does seem to be longer than mine), or think bwinton is sufficiently familiar with the space and can get him to take it, feel free;  otherwise I'll probably get to it Sun/Mon.
Attached patch proposed fix, v2Splinter Review
Unbitrotted, punting to Blake.
Attachment #395314 - Attachment is obsolete: true
Attachment #398723 - Flags: review?(bwinton)
Attachment #395314 - Flags: review?(bugmail)
(In reply to comment #5)
> Created an attachment (id=398723) [details]
> proposed fix, v2
> Unbitrotted, punting to Blake.

Heh, thanks.

>-
>- <binding id="tabmail-tab" display="xul:box"
>+
>+  <binding id="tabmail-tab" display="xul:box"

nit: indentation.

>+      <xul:hbox class="tab-image-middle box-inherit" align="center"
>+                xbl:inherits="align,dir,pack,orient,selected" flex="1">

If you're inheriting the align, does it make sense to also specify align="center"?

>-* skin/classic/messenger/tabmailBindings.xml                  (mail/tabmailBindings.xml)

I think we should probably also remove the line:
* skin/classic/aero/messenger/tabmailBindings.xml                  (mail/tabmailBindings.xml)
in the same file.

One thing I found while testing on OS X was that opening the Blogs & News Feeds in a new tab didn't use the same icon in the tab as in the folder pane.  But it ended up having the same behaviour without your patch, so I don't mind if you don't fix that.

With those fixed/explained, r=me.

Later,
Blake.
Attachment #398723 - Flags: review?(bwinton) → review+
(In reply to comment #6)
> >- <binding id="tabmail-tab" display="xul:box"
> >+
> >+  <binding id="tabmail-tab" display="xul:box"
> 
> nit: indentation.

The original was wrong ;)
 
> >+      <xul:hbox class="tab-image-middle box-inherit" align="center"
> >+                xbl:inherits="align,dir,pack,orient,selected" flex="1">
> 
> If you're inheriting the align, does it make sense to also specify
> align="center"?

Yeah seems odd. No longer inheriting align.
 
> >-* skin/classic/messenger/tabmailBindings.xml                  (mail/tabmailBindings.xml)
> 
> I think we should probably also remove the line:
> * skin/classic/aero/messenger/tabmailBindings.xml                 
> (mail/tabmailBindings.xml)
> in the same file.

Good catch!

> One thing I found while testing on OS X was that opening the Blogs & News Feeds
> in a new tab didn't use the same icon in the tab as in the folder pane.  But it
> ended up having the same behaviour without your patch, so I don't mind if you
> don't fix that.

Hm, should be easier to sort out after landed. Especially as i don't have a mac.

changeset:   3572:4629520c1b99
http://hg.mozilla.org/comm-central/rev/4629520c1b99

->FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #7)
> (In reply to comment #6)
> > >- <binding id="tabmail-tab" display="xul:box"
> > >+
> > >+  <binding id="tabmail-tab" display="xul:box"
> > nit: indentation.
> The original was wrong ;)

Ah, yes, I thought it had changed from 0 to 1 space.  I must be too used to diff's output.  :)

> Hm, should be easier to sort out after landed. Especially as i don't have a
> mac.

I'll be happy to test any patches you may have for that.  :)

Thanks,
Blake.
Blake, the problem (if you see it still) should be with the rules around
http://mxr.mozilla.org/comm-central/source/mail/themes/pinstripe/mail/folderPane.css#25

If there's a problem there, can you file a bug?
You need to log in before you can comment on or make changes to this bug.