947657 - [Shared Themes] Pt. 1 - Move messenger.css code to shared/

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Comment 1

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Here's the patch that:

A. Creates a shared directory. So we now have themes/linux, themes/osx/, themes/windows/, and themes/shared.

B. Moves shared code from all platforms and adds them to themes/shared/mail/messenger.inc.css.

Richard, I have only tested this on OS X, (Linux builds are failing for me, and my Windows VM isn't functioning properly). I am trying to fix the Linux builds, so if I find there is an issue before you do I'll clear the review flags.

In addition,

I am currently using %include's to import the proper style sheets, and although this works fine, it has the disadvantage of requiring a re-build (Files must get pre-processed to import the CSS). Although I don't really care, I would like to hear your thoughts on that issue.

And thanks so much for reviewing this!

Comment 2

[Richard Marti (:Paenglab)]

Comment on attachment 8344249 [details] [diff] [review]
Patch.

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

This looks already good. But needs some changes to become a r+.

::: mail/themes/linux/mail/messenger.css
@@ +12,2 @@
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");

Please remove the namespace line. It's already defined in messenger.inc.css and gives a error in console.

@@ +15,5 @@
>  toolbar:not(.inline-toolbar):not(:-moz-lwtheme) {
>    -moz-appearance: menubar;
>  }
>  
>  #tabbar-toolbar {

This and #tabbar-toolbar[customizing="true"] are in messenger.inc.css and you can remove it from here.

@@ +369,4 @@
>  
>  notification[value="addon-install-failed"] .messageCloseButton {
>    -moz-appearance: none;
>    list-style-image: url("chrome://messenger/skin/icons/close-inverted.png");

You can remove the list-style-image. It's defined in messenger.inc.css.

::: mail/themes/osx/mail/messenger.css
@@ +12,2 @@
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");

Please remove the namespace line. It's already defined in messenger.inc.css and gives a error in console.

@@ +12,4 @@
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
> +#tabbar-toolbar {

Why are you adding this? This is already in messenger.inc.css.

@@ +38,5 @@
>  /* :::::: throbber :::::::::: */
>  
>  #throbber-box {
> +  width: 17px !important;
> +  margin: 0 4px !important;

Are both !important needed? This comes after the messenger.inc.css definition and should oversteer it without !important.

::: mail/themes/shared/mail/messenger.inc.css
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

With a 

%if 0
/*...*/
%endif

around this comment it isn't added to the output file. I think one time in the file is enough :)

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ===== messenger.inc.css ==================================================

I think this comment isn't needed in this file. One time in messenger.css is enough.

@@ +71,5 @@
> +notification[value="addon-install-failed"] {
> +  background-image: linear-gradient(rgb(255, 100, 100), rgb(204, 85, 85));
> +}
> +
> +notification[value="addon-install-complete"] {

This is a OS X only rule and doesn't fit to the other themes. Please leave it there.

@@ +82,5 @@
> +notification[value="addon-install-complete"] .notification-inner {
> +  border: none;
> +}
> +
> +notification[value="addon-install-blocked"] > button,

The notification > button rules are OS X only and are making the buttons on the other platforms bigger. Please let all this rules in osx.

::: mail/themes/windows/mail/messenger-aero.css
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  %include messenger.css
> +%include ../../shared/mail/messenger.inc.css

No need to include it here. It's already included in messenger.css which is added the line above.

::: mail/themes/windows/mail/messenger.css
@@ +12,2 @@
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");

Please remove the namespace line. It's already defined in messenger.inc.css and gives a error in console.

Comment 3

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Addresses review feedback.

Comment 4

[Richard Marti (:Paenglab)]

Comment on attachment 8344370 [details] [diff] [review]
Patch.

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

Yeah great. 

Sorry to not mention the two comments in previous patch. With this fixed r=me

::: mail/themes/linux/mail/messenger.css
@@ +50,3 @@
>  /* ::::: throbber ::::: */
>  
>  #throbber-box {

The throbber-box can be removed, it is already in messenger.inc.css.

::: mail/themes/osx/mail/messenger.css
@@ +13,5 @@
>  #titlebar {
>    -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
>    height: 22px;
> +
> +  /* Until we draw tabs in the titlebar on OS X we must

Thank you for removing the whitespaces :)

::: mail/themes/windows/mail/messenger.css
@@ +41,3 @@
>  /* ::::: throbber ::::: */
>  
>  #throbber-box {

The throbber-box can be removed, it is already in messenger.inc.css.

Comment 5

[Josiah Bruner [:jsbruner]]

Attachment: Final Patch.

Thanks Richard!

Running through try before landing...

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=0a9970f7b8b9

Comment 6

[Josiah Bruner [:jsbruner]]

https://hg.mozilla.org/comm-central/rev/0d59aed637f9