XP_LINUX is reserved for code that's specific to Linux kernel or (less often) glibc. Popular X11 toolkits like Gtk and Qt do exist on other platforms as well. So, let's not diverge look&feel for other XP_UNIX, say, Solaris and various BSDs. $ fgrep -r XP_LINUX browser/ browser/base/content/browser.xul:#ifdef XP_LINUX browser/themes/shared/devtools/highlighter.inc.css:%ifndef XP_LINUX browser/themes/shared/devtools/common.css:%ifdef XP_LINUX browser/themes/shared/devtools/common.css:%ifdef XP_LINUX browser/themes/shared/tab-selected.svg:%elifdef XP_LINUX

Comment on attachment 8345895 [details] [diff] [review] Share Linux theme on every GTK platform. How about #ifdef MOZ_WIDGET_GTK? It's used by both cairo-gtk2 and cairo-gtk3 builds.

Comment on attachment 8345895 [details] [diff] [review] Share Linux theme on every GTK platform. Heh, nice catch - i can only support the de-XP_LINUX'ification of those code areas :)

Comment on attachment 8345895 [details] [diff] [review] Share Linux theme on every GTK platform. Hm, I'm not 100% confident that I'm the best reviewer for this. Tentatively redirecting to Dao.

If this is how we should be writing new ifdef's to detect GTK platforms then there should be an email to firefox-dev and dev-developer-tools to let developers know for the future.

(This time after actually confirming preprocessor.py accepts cpp(1)-like syntax.) Other XP_* ifdefs are left alone for now. Not sure if there're Cocoa Firefox users on GNUstep/Linux. This is somewhat in line with browser/themes/moz.build toolkit = CONFIG['MOZ_WIDGET_TOOLKIT'] if toolkit == 'cocoa': DIRS += ['osx'] elif toolkit in ('gtk2', 'gtk3', 'qt'): DIRS += ['linux'] else: DIRS += ['windows'] where 'linux' was 'gnomestripe' before bug 842913. Looking at current XP_LINUX ifdefs (few in number) they were introduced after the rename.

Comment on attachment 8346619 [details] [diff] [review] Test toolkit, not platform, for linux theme. v1 Nevermind, I can just define XP_LINUX in moz.build and avoid having to deal with maillists (require working email).

Comment on attachment 8346640 [details] [diff] [review] Define XP_LINUX to have consistent "linux" theme on non-Linux platforms. browser/base/moz.build overriding XP_LINUX doesn't feel right to me... seems like asking for trouble.

One of those reasons why I think bug 842913 shouldn't have called the GTK theme "linux".

Comment on attachment 8346619 [details] [diff] [review] Test toolkit, not platform, for linux theme. v1 >--- a/browser/themes/shared/devtools/common.css >+++ b/browser/themes/shared/devtools/common.css > .devtools-monospace { > %ifdef XP_MACOSX > font-family: Menlo, monospace; > %endif >-%ifdef XP_LINUX >+%if defined(MOZ_WIDGET_GTK) || defined(MOZ_WIDGET_QT) > font-family: monospace; > font-size: 80%; > %endif > %ifdef XP_WIN > font-family: Consolas, monospace; > %endif Please reorganize this as follows: > %ifdef XP_MACOSX > font-family: Menlo, monospace; > %elifdef XP_WIN > font-family: Consolas, monospace; > %else > font-family: monospace; > %endif > %if defined(MOZ_WIDGET_GTK) || defined(MOZ_WIDGET_QT) > font-size: 80%; > %endif r=me with that change

Leave open for now until comment 5 is addressed. If it doesn't work out, blocking bugs that regress this one until everyone learns can be a workaround.

Comment on attachment 8347190 [details] [diff] [review] Test toolkit, not platform, for linux theme. v1.1 To avoid confusion, only previously r+ diff needs checkin.

Blocking bug 963576 as it's now in ff29.

(In reply to Jan Beich from comment #17) > Blocking bug 963576 as it's now in ff29. Please file a new bug and submit a patch. Also, could you send the email mentioned in comment 5?

Attaching here because there's time to re-use Target Milestone. browser.xul already contains the same ifdef a few lines above.

Comment on attachment 8400251 [details] [diff] [review] bug963576 followup (In reply to Matthew N. [:MattN] from comment #18) > Please file a new bug and submit a patch. Obsolete the patch if you want to insist despite comment 19. > Also, could you send the email mentioned in comment 5? That'd take some time as I still don't have working email.

Comment on attachment 8400251 [details] [diff] [review] bug963576 followup Please give a more descriptive commit message mentioning "private-browsing-indicator" or "private browsing indicator" and in the future use a new bug once an existing bug has been resolved and you're not just fixing fallout.

just new commit message, carry over r=MattN (In reply to Matthew N. [:MattN] from comment #21) > in the future use a new bug once an existing bug has been resolved and you're not > just fixing fallout. OK, mistake admitted especially since (In reply to Jan Beich from comment #13) > ... blocking bugs that regress this one until everyone learns can be a > workaround.

Comment on attachment 8401173 [details] [diff] [review] bug963576 followup [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 963576 regressing this one User impact if declined: no private-browsing icon in top-left corner showing on non-Linux Tier3 platforms Testing completed (on m-c, etc.): soon Risk to taking this patch (and alternatives if risky): None, the file already uses the same ifdef String or IDL/UUID changes made by this patch: None, syncing appearance with Linux

Comment on attachment 8401173 [details] [diff] [review] bug963576 followup Sorry, not yet in central. Btw, Jan, "None" is longer a "valid risk" ;) https://wiki.mozilla.org/Release_Management/Uplift_rules#Guidelines_on_approval_comments

Needs to land in Beta as well because of bug 963576 comment 43.

(In reply to Jan Beich from comment #28) > Needs to land in Beta as well because of bug 963576 comment 43. Yes, I was just waiting to make sure there were no failures on Aurora before pushing to Beta. https://hg.mozilla.org/releases/mozilla-beta/rev/f7faeaf19dfa

Jan could you verify the fix on latest versions? http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/29.0b6-candidates/ http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/ http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/

florin: just so you know, it's pointless to blindlessly point people at repositories where builds affected by that bug are not present, since they're not tier1.... get some context first :)

My bad... I wanted to mention that 29.0b6 (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/29.0b6-candidates/) is not yet available (should be available in a few hours), but for some reason it slipped :).

What i meant was that those dirs on the FTP _dont_ ship build/binaries for the platforms targeted by this fix.

Confirm to work as expected on self-built ff31 and ff29, also on linux ff29 from ftp.m.o.

(In reply to Matthew N. [:MattN] from comment #5) > If this is how we should be writing new ifdef's to detect GTK platforms then > there should be an email to firefox-dev and dev-developer-tools to let > developers know for the future. ENOTIME to write such an email, and my mailhost blocked SMTP for Tor users. Anyway, this is not a technical but a social issue, so not appropriate to block closing the bug. I'll probably just file bugs for regressions in future.