1150006 - Get rid of "-aero" file name suffixes

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Dão Gottwald [:dao]]

The "-aero" prefix is confusing, because it doesn't really refer to the Windows Aero theme anymore. Effectively, it means "Windows Vista and later" or "Windows other than XP" if you will. We should get rid of that prefix / replace it with something clearer.

Comment 1

[Dão Gottwald [:dao]]

Attachment: part 1: CSS files

Comment 2

[Robert Kaiser]

Why are you doing defines, which cause even more grief for theme designers wanting to follow the default theme (well, doing that is a near-to-hell things already) instead of using CSS variables?

Comment 3

[Dão Gottwald [:dao]]

(In reply to Robert Kaiser (:[e-mail]) from comment #2)
> Why are you doing defines, which cause even more grief for theme designers
> wanting to follow the default theme (well, doing that is a near-to-hell
> things already) instead of using CSS variables?

Discussion for a different bug / forum. My patch doesn't introduce defines, it merely moves them.

Comment 4

[Stefan [:stefanh]]

Comment on attachment 8586770 [details] [diff] [review]
part 1: CSS files

+%ifdef XP_WIN
Hmm, afaics this is a windows-only file.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Stefan [:stefanh] from comment #4)
> Comment on attachment 8586770 [details] [diff] [review]
> part 1: CSS files
> 
> +%ifdef XP_WIN
> Hmm, afaics this is a windows-only file.

None of these are windows-only:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/moz.build

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8586770 [details] [diff] [review]
part 1: CSS files

https://hg.mozilla.org/integration/fx-team/rev/cba8cbff84d2

Comment 7

[Dão Gottwald [:dao]]

Attachment: part 2a: rename XP-specific PNGs to *-XP.png

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> Created attachment 8587354 [details] [diff] [review]
> part 2a: rename XP-specific PNGs to *-XP.png

This looks like it's the hidpi Windows patch?

Comment 9

[Dão Gottwald [:dao]]

Attachment: part 2b: rename foo-aero.png to foo.png

Parts 2a and 2b need to be applied together, otherwise the build will fail. They're two separate patches because renaming foo.png to foo-XP.png and foo-aero.png to foo.png in the same patch makes hg produce a binary diff for foo.png.

Comment 10

[Dão Gottwald [:dao]]

Attachment: part 2a: rename XP-specific PNGs to *-XP.png

Comment 11

[Dão Gottwald [:dao]]

Note that there are some duplicate images making hg interpret the renames of these files as copies and a deletion of the renamed file. That makes these parts of patches 2a and 2b slightly confusing.

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8587356 [details] [diff] [review]
part 2a: rename XP-specific PNGs to *-XP.png

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

This looks OK, though it's confusing that HG logs deletions for some files which it then has as renames from a different file. For instance, the patch says:

category-experiments.png got deleted
experimentGeneric.png got renamed to category-experiments-XP.png
experimentGeneric.png got copied to experimentGeneric-XP.png

which is counterintuitive but, I guess, net the same as category-experiments being copied/moved to the -XP variant? I thought HG logged moves explicitly and therefore wouldn't have this problem... :-\

Comment 13

[:Gijs (out for now; he/him)]

Heh, and then I missed the comment where you explained basically just that. OK! :-)

Comment 14

[:Gijs (out for now; he/him)]

Comment on attachment 8587355 [details] [diff] [review]
part 2b: rename foo-aero.png to foo.png

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

LGTM.

Comment 15

[Dão Gottwald [:dao]]

Attachment: part 3: remove more duplicates

Comment 16

[:Gijs (out for now; he/him)]

Err, just realized... does this mean Linux will now get the aero icons instead of the XP icons?

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 8587374 [details] [diff] [review]
part 3: remove more duplicates

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

r=me with further removal addressed.

::: toolkit/themes/windows/mozapps/jar.mn
@@ +21,1 @@
>          skin/classic/mozapps/extensions/category-experiments.png   (extensions/category-experiments.png)

AFAICT this can go now, and the file can also be removed?

Comment 18

[leichixian]

I want to point out such big changes will cause problem to 3rd party theme writer, as they don't know the way how your CSS file is served. After the change you need to write a blog on how the file is served and how to overide it.

Comment 19

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #16)
> Err, just realized... does this mean Linux will now get the aero icons
> instead of the XP icons?

Linux overrides most of these files with its own versions, but not all. We can make Linux explicitly package the XP icons, but I don't think it matters much. There's one particular class of icons Linux decidedly doesn't override: question, error and info icons. That's because the Linux theme uses Gtk stock icons for those. When it doesn't do so, that would probably be a bug.

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #16)
> > Err, just realized... does this mean Linux will now get the aero icons
> > instead of the XP icons?
> 
> Linux overrides most of these files with its own versions, but not all. We
> can make Linux explicitly package the XP icons, but I don't think it matters
> much.

Agreed, though it might be worth doing a checkup on things looking weirdly out of place where we forgot something (I'm thinking things like toolbar/menupanel icons that mismatch with the rest of the toolbar/menupanel icons (we override the sprites with a copy of one of the Windows ones (sigh), but some icons are not in the sprites...)).

> There's one particular class of icons Linux decidedly doesn't
> override: question, error and info icons. That's because the Linux theme
> uses Gtk stock icons for those. When it doesn't do so, that would probably
> be a bug.

Fair!

Comment 21

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #17)
> ::: toolkit/themes/windows/mozapps/jar.mn
> @@ +21,1 @@
> >          skin/classic/mozapps/extensions/category-experiments.png   (extensions/category-experiments.png)
> 
> AFAICT this can go now, and the file can also be removed?

indeed

(In reply to leichixian from comment #18)
> I want to point out such big changes will cause problem to 3rd party theme
> writer, as they don't know the way how your CSS file is served.

I don't understand what this means.

Comment 22

[Dão Gottwald [:dao]]

Attachment: part 4: replace some Windows icons with stock icons on Linux

chrome://global/skin/icons/question-16.png and chrome://global/skin/icons/information-16.png are used as favicons, so I added overrides for them. Otherwise I referenced the stock icons directly like we usually do.

This isn't necessarily comprehensive, i.e. there might be more cleanup due, but I think this will be my last patch in this bug.

Comment 23

[Robert Kaiser]

BTW, in general, note that any override you add anywhere means that 3rd-party themes either need to package additional files on the override URL or avoid referring to the chrome URLs of those files at all and rename the files and references to them.

Comment 24

[:Gijs (out for now; he/him)]

(In reply to Robert Kaiser (:[e-mail]) from comment #23)
> BTW, in general, note that any override you add anywhere means that
> 3rd-party themes either need to package additional files on the override URL
> or avoid referring to the chrome URLs of those files at all and rename the
> files and references to them.

We're aware of this. AIUI Dão will file another followup bug and we'll move the overrides (and potentially the rest of the theme stuff, not sure how easy it is to split them apart) into a separate chrome.manifest which will only be loaded for the default theme instead of for omni.jar.

Comment 25

[:Gijs (out for now; he/him)]

Comment on attachment 8587396 [details] [diff] [review]
part 4: replace some Windows icons with stock icons on Linux

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

Did you test the override with moz-icon:// ? I'm not 100% sure that works with the chrome registry, but I could be wrong.

Comment 26

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #25)
> Comment on attachment 8587396 [details] [diff] [review]
> part 4: replace some Windows icons with stock icons on Linux
> 
> Review of attachment 8587396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you test the override with moz-icon:// ? I'm not 100% sure that works
> with the chrome registry, but I could be wrong.

Yep, tested and works well.

Comment 27

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/c2dfc44c2b51
https://hg.mozilla.org/integration/fx-team/rev/82bb0e158d53
https://hg.mozilla.org/integration/fx-team/rev/5b77f598a784
https://hg.mozilla.org/integration/fx-team/rev/dca65796f8ab

Comment 28

[leichixian]

(In reply to Dão Gottwald [:dao] from comment #21)
> (In reply to :Gijs Kruitbosch from comment #17)
> > ::: toolkit/themes/windows/mozapps/jar.mn
> > @@ +21,1 @@
> > >          skin/classic/mozapps/extensions/category-experiments.png   (extensions/category-experiments.png)
> > 
> > AFAICT this can go now, and the file can also be removed?
> 
> indeed
> 
> (In reply to leichixian from comment #18)
> > I want to point out such big changes will cause problem to 3rd party theme
> > writer, as they don't know the way how your CSS file is served.
> 
> I don't understand what this means.

I mean this: https://github.com/louischan/simplewhite/issues/32. I'm a co-worker of this theme and I test this theme on nightly. After update to nightly build 20150402xxxxxx, this theme can't work properly, and the repo owener gived the reason.

Comment 29

[[e-mail]]

(In reply to Gijs Kruitbosch from comment #12)
> This looks OK, though it's confusing that HG logs deletions for some files
> which it then has as renames from a different file. For instance, the patch
> says:
> 
> category-experiments.png got deleted
> experimentGeneric.png got renamed to category-experiments-XP.png
> experimentGeneric.png got copied to experimentGeneric-XP.png
> 
> which is counterintuitive but, I guess, net the same as category-experiments
> being copied/moved to the -XP variant? I thought HG logged moves explicitly
> and therefore wouldn't have this problem... :-\

FYI It does if you tell it to (explicit hg mv). If you rely on hg addremove (I guess git-to-hg conversion would also have the same problem) then it will do this.

Comment 30

[Dão Gottwald [:dao]]

Backed out part 4 since it seems to break toolkit/components/places/tests/unit/test_bookmarks_html.js, a test that does fancy stuff with chrome://global/skin/icons/information-16.png.

https://hg.mozilla.org/integration/fx-team/rev/71338877d1dc

Comment 31

[Dão Gottwald [:dao]]

Landed part 4 without the overrides:

https://hg.mozilla.org/integration/fx-team/rev/250e629c9afb

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cba8cbff84d2
https://hg.mozilla.org/mozilla-central/rev/c2dfc44c2b51
https://hg.mozilla.org/mozilla-central/rev/82bb0e158d53
https://hg.mozilla.org/mozilla-central/rev/5b77f598a784
https://hg.mozilla.org/mozilla-central/rev/250e629c9afb

Comment 33

[Alfred Kayser]

(In reply to :Gijs Kruitbosch from comment #24)
> AIUI Dão will file another followup bug and we'll move
> the overrides (and potentially the rest of the theme stuff, not sure how
> easy it is to split them apart) into a separate chrome.manifest which will
> only be loaded for the default theme instead of for omni.jar.

Is this allready registred in bugzilla somewhere?

Comment 34

[:Gijs (out for now; he/him)]

(In reply to Alfred Kayser from comment #33)
> (In reply to :Gijs Kruitbosch from comment #24)
> > AIUI Dão will file another followup bug and we'll move
> > the overrides (and potentially the rest of the theme stuff, not sure how
> > easy it is to split them apart) into a separate chrome.manifest which will
> > only be loaded for the default theme instead of for omni.jar.
> 
> Is this allready registred in bugzilla somewhere?

Ironically, I just morphed your bug for this purpose: bug 1150417.