1302759 - missing customize-titleBar-toggle.png referenced from browser.css on Linux

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Florian Quèze [:florian]]

The test I'm adding in bug 1302570 shows this on Linux only:

missing chrome://browser/skin/customizableui/customize-titleBar-toggle.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/customizableui/[e-mail] referenced from chrome://browser/skin/browser.css

This was added in bug 940093 and these files aren't shipped on Linux because CAN_DRAW_IN_TITLEBAR isn't defined there.

The options I see to fix this:

- add a CAN_DRAW_IN_TITLEBAR ifdef in browser/themes/shared/customizableui/customizeMode.inc.css
I thought we were removing all ifdefs in chrome files, but I see we still have one such ifdef at http://searchfox.org/mozilla-central/source/browser/base/content/browser.css#197 so maybe it's OK?

- ship move these 2 png files to shared/ and ship them even on Linux where they'll never be used.

Thoughts?

Comment 1

[:Gijs (he/him)]

I think we should go for unlisted option #3: add the list of exceptions in the automated checker. We're going to need it sooner or later because there are legitimate cases where unused CSS (because the element the CSS applies to doesn't exist) is shipped in shared/ and any effort to remedy that isn't worth the engineering expense or complexity or footprint change (ie I wouldn't like to ship the unused images, and adding ifdefs just for this is not worth it).

If we're adamant we don't want to do this, I'd prefer going for unlisted option #4, and unwrap the includes here. So create individual osx/windows/linux versions of customizeMode.inc.css, include the shared one in those, and only have this CSS in the osx/windows ones, and include those from their respective browser.css files. Look ma, no ifdefs! ;-)

Comment 2

[Dão Gottwald [:dao]]

Using the CAN_DRAW_IN_TITLEBAR ifdef seems semantically right and is likely the simplest solution. It's unclear to me why it wouldn't be "worth it". As I understand it, we're not gaining anything by avoiding the ifdef because customizeMode.inc.css needs preprocessing anyway.

Comment 3

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [:dao] from comment #2)
> Using the CAN_DRAW_IN_TITLEBAR ifdef seems semantically right and is likely
> the simplest solution. It's unclear to me why it wouldn't be "worth it". As
> I understand it, we're not gaining anything by avoiding the ifdef because
> customizeMode.inc.css needs preprocessing anyway.

I agree. That said, I wanted to move forward with bug 1302570, so I landed it with Gijs' option #3. So if you want to fix this (with an ifdef or whatever solution gets an r+), think about removing the exception from the browser_parsable_css.js test file.

Comment 4

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #2)
> we're not gaining anything by avoiding the ifdef because
> customizeMode.inc.css needs preprocessing anyway.

The static checking in bug 1302570 shows that the file wouldn't exist if we ever tried to load it, but the selector will never apply on Linux so that doesn't actually happen in practice. So it doesn't actually change runtime behaviour or cause problems to have the selector be present on Linux. All we'd do is shut up the warning in the test by adding the %ifdef. However, we'd be keeping a number of other rules that also don't apply on Linux because they apply to the same nonexistent element, but they don't "need" an ifdef because there isn't a test that verifies all the selectors in our CSS apply to "something" (which would be a hard test to write, to be sure).

So, if all we're doing is getting rid of the warning in the test, keeping that list in the test seems more sane than adding otherwise useless ifdefs in the source, unless we're about to start getting really serious about removing rules from shared code that are unused on some platforms, in which case there is more work to be done than just that single ifdef.

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #4)
> So it doesn't actually change runtime
> behaviour or cause problems to have the selector be present on Linux. All
> we'd do is shut up the warning in the test by adding the %ifdef.

The same is true for the exception added to the test file, except that that adds maintenance overhead. So even if we were to use the ifdef only to silence the warning, I'd still prefer that over the exception. However, the ifdef is reasonable on its own, because right now we're effectively shipping dead code that (while cheap) isn't free. It might also be a helpful hint e.g. for somebody reading that stylesheet and looking at the actual UI on Linux and wondering what button the stylesheet refers to.

> So, if all we're doing is getting rid of the warning in the test, keeping
> that list in the test seems more sane than adding otherwise useless ifdefs
> in the source, unless we're about to start getting really serious about
> removing rules from shared code that are unused on some platforms, in which
> case there is more work to be done than just that single ifdef.

I'm not aware of other cases where we ship shared rules that don't apply on all platforms. There might be a few, but I don't think it's very common, nor do we have to fix all cases just because we're fixing this one.

Comment 6

[Dão Gottwald [:dao]]

Precedent: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#197

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > So it doesn't actually change runtime
> > behaviour or cause problems to have the selector be present on Linux. All
> > we'd do is shut up the warning in the test by adding the %ifdef.
> 
> The same is true for the exception added to the test file, except that that
> adds maintenance overhead.

Sorry, how does that add maintenance overhead where the ifdef doesn't?

> So even if we were to use the ifdef only to
> silence the warning, I'd still prefer that over the exception. However, the
> ifdef is reasonable on its own, because right now we're effectively shipping
> dead code that (while cheap) isn't free. It might also be a helpful hint
> e.g. for somebody reading that stylesheet and looking at the actual UI on
> Linux and wondering what button the stylesheet refers to.

By the "help the reader" logic we should at least ifdef out *all* the rules relating to this button in that file, though (or it'll be even more confusing - why would one ifdef out the image source but keep the image region change for [checked], etc.), which is messy because some are in shared rules.

I also note that you've just added a bunch of other platform ifdefs to this same file in an orthogonal styling bug that I r+'d.

From that perspective, it seems that if we want to change source code rather than having the test exception, it'd be easier to read/understand if we separate out the can_draw_in_titlebar/win+osx stuff into its own file(s), which is closer to #4 in comment #1.

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #7)
> Sorry, how does that add maintenance overhead where the ifdef doesn't?

It's a separate file that needs to be kept in sync.

> By the "help the reader" logic we should at least ifdef out *all* the rules
> relating to this button in that file, though (or it'll be even more
> confusing - why would one ifdef out the image source but keep the image
> region change for [checked], etc.), which is messy because some are in
> shared rules.

One of them is in a shared rule. Not that messy.

> I also note that you've just added a bunch of other platform ifdefs to this
> same file in an orthogonal styling bug that I r+'d.

Indeed. Is this supposed to be an argument against using another ifdef here?

> From that perspective, it seems that if we want to change source code rather
> than having the test exception, it'd be easier to read/understand if we
> separate out the can_draw_in_titlebar/win+osx stuff into its own file(s),
> which is closer to #4 in comment #1.

We've added CAN_DRAW_IN_TITLEBAR because we wanted to abstract away what platforms this applies to. If I remember correctly, CAN_DRAW_IN_TITLEBAR initially meant XP_WIN, and then we added OS X support. Bug 513159 is still open. This is why using CAN_DRAW_IN_TITLEBAR is preferable over moving the code to platform-specific files.

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd630bd1ebd8
Use CAN_DRAW_IN_TITLEBAR ifdef for customization-titlebar-visibility-button. r=gijs

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd630bd1ebd8