Closed Bug 1150006 Opened 5 years ago Closed 5 years ago

Get rid of "-aero" file name suffixes

Categories

(Toolkit :: Themes, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox40 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(5 files, 1 obsolete file)

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.
Flags: qe-verify-
Flags: firefox-backlog+
Attachment #8586770 - Flags: review?(gijskruitbosch+bugs)
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?
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 on attachment 8586770 [details] [diff] [review]
part 1: CSS files

+%ifdef XP_WIN
Hmm, afaics this is a windows-only file.
(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
Attachment #8586770 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: leave-open
Attachment #8587354 - Flags: review?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(dao)
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.
Attachment #8587355 - Flags: review?(gijskruitbosch+bugs)
Attachment #8587354 - Attachment is obsolete: true
Attachment #8587354 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(dao)
Attachment #8587356 - Flags: review?(gijskruitbosch+bugs)
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 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... :-\
Attachment #8587356 - Flags: review?(gijskruitbosch+bugs) → review+
Heh, and then I missed the comment where you explained basically just that. OK! :-)
Comment on attachment 8587355 [details] [diff] [review]
part 2b: rename foo-aero.png to foo.png

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

LGTM.
Attachment #8587355 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8587374 - Flags: review?(gijskruitbosch+bugs)
Err, just realized... does this mean Linux will now get the aero icons instead of the XP icons?
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?
Attachment #8587374 - Flags: review?(gijskruitbosch+bugs) → review+
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.
(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.
(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!
(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.
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.
Attachment #8587396 - Flags: review?(gijskruitbosch+bugs)
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.
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 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.
Attachment #8587396 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
(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.
(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.
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
Keywords: leave-open
Landed part 4 without the overrides:

https://hg.mozilla.org/integration/fx-team/rev/250e629c9afb
Keywords: leave-open
(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?
(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.
You need to log in before you can comment on or make changes to this bug.