1002370 - Investigate if primaryToolbar.css and primaryToolbar-aero.css can share some code

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

primaryToolbar.css and primaryToolbar-aero.css aren't sharing their code but have some in common. This bug is to look if the code can be shared.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: bug1002370.patch

I compared the XP before patch and after patch primaryToolbar.css and they are the same except some newlines which are coming from the %ifdefs. On Win7/8 I saw no difference in appearance.

Comment 2

[Josiah Bruner [:jsbruner]]

Comment on attachment 8426484 [details] [diff] [review]
bug1002370.patch

Review of attachment 8426484 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't apply this or anything, but I trust that there are no real visual changes. I did have a couple comments, but they didn't really have to do with your patch in particular. So r+.

::: mail/themes/windows/mail/primaryToolbar.css
@@ +244,5 @@
>  }
>  
>  #button-getmsg:hover {
>    -moz-image-region: rect(24px 24px 48px 0px);
>  } 

Trailing whitespace, could you remove that?

@@ +1024,1 @@
>      -moz-margin-start: calc(1.45em + 4px);

Could you add a comment here explaining why we are calculating ems and pixels together? What's the reason for this?

Comment 3

[Richard Marti (:Paenglab)]

Attachment: Bug1002370.patch

(In reply to Josiah Bruner [:JosiahOne] from comment #2)
> Comment on attachment 8426484 [details] [diff] [review]
> bug1002370.patch
> 
> Review of attachment 8426484 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't apply this or anything, but I trust that there are no real visual
> changes. I did have a couple comments, but they didn't really have to do
> with your patch in particular. So r+.
> 
> ::: mail/themes/windows/mail/primaryToolbar.css
> @@ +244,5 @@
> >  }
> >  
> >  #button-getmsg:hover {
> >    -moz-image-region: rect(24px 24px 48px 0px);
> >  } 
> 
> Trailing whitespace, could you remove that?

Done, plus a lot of others.
 
> @@ +1024,1 @@
> >      -moz-margin-start: calc(1.45em + 4px);
> 
> Could you add a comment here explaining why we are calculating ems and
> pixels together? What's the reason for this?

Done.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: Bug1002370.patch

(In reply to Richard Marti (:Paenglab) from comment #3)
> Created attachment 8428365 [details] [diff] [review]
> Bug1002370.patch
> 
> (In reply to Josiah Bruner [:JosiahOne] from comment #2)
> > Comment on attachment 8426484 [details] [diff] [review]
> > bug1002370.patch
> > 
> > Review of attachment 8426484 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I didn't apply this or anything, but I trust that there are no real visual
> > changes. I did have a couple comments, but they didn't really have to do
> > with your patch in particular. So r+.
> > 
> > ::: mail/themes/windows/mail/primaryToolbar.css
> > @@ +244,5 @@
> > >  }
> > >  
> > >  #button-getmsg:hover {
> > >    -moz-image-region: rect(24px 24px 48px 0px);
> > >  } 
> > 
> > Trailing whitespace, could you remove that?
> 
> Done, plus a lot of others.

Found three more. ;)

Comment 5

[Richard Marti (:Paenglab)]

No more found. I should rename this bug to whitespace cleaning. ;)

Comment 6

[Josiah Bruner [:jsbruner]]

https://hg.mozilla.org/comm-central/rev/b9b60216226f