Closed Bug 948946 Opened 7 years ago Closed 4 years ago

Themes should be consistent between different GTK platforms

Categories

(Firefox :: Theme, defect)

x86_64
FreeBSD
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox29 --- verified
firefox30 --- fixed
firefox31 --- verified

People

(Reporter: jbeich, Assigned: jbeich)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P5-])

Attachments

(2 files, 4 obsolete files)

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.
Attachment #8345895 - Flags: review?(mconley)
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.
Attachment #8345895 - Flags: review?(mconley) → review?(dao)
Component: Build Config → Theme
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.
Assignee: nobody → jbeich
Status: NEW → ASSIGNED
Summary: Australis should be consistent between different GTK platforms → Themes should be consistent between different GTK platforms
(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.
Attachment #8345895 - Attachment is obsolete: true
Attachment #8345895 - Flags: review?(dao)
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).
Attachment #8346619 - Attachment is patch: false
Attachment #8346619 - Attachment is obsolete: true
Attachment #8346619 - Attachment is patch: true
Attachment #8346640 - Flags: review?(dao)
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.
Attachment #8346640 - Flags: review?(dao) → review-
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
Attachment #8346619 - Attachment is obsolete: false
Attachment #8346619 - Flags: review+
Attachment #8346619 - Attachment is obsolete: true
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.
Keywords: checkin-needed
Whiteboard: [leave open]
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.
Attachment #8347190 - Flags: checkin?
Attachment #8346640 - Attachment is obsolete: true
Attachment #8347190 - Flags: checkin? → checkin+
https://hg.mozilla.org/integration/fx-team/rev/b54bf98cb176
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b54bf98cb176
Whiteboard: [leave open][fixed-in-fx-team] → [leave open]
Target Milestone: --- → Firefox 29
Blocking bug 963576 as it's now in ff29.
Blocks: 963576
(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?
Flags: needinfo?(jbeich)
Attached patch bug963576 followup (obsolete) — Splinter Review
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.
Attachment #8400251 - Flags: review?(MattN+bmo)
Flags: needinfo?(jbeich)
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.
Attachment #8400251 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8400251 - Attachment is obsolete: true
Keywords: checkin-needed
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
Attachment #8401173 - Flags: approval-mozilla-beta?
Attachment #8401173 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/fx-team/rev/47ba4bce872f
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][fixed-in-fx-team]
Keywords: leave-open
Whiteboard: [leave open][fixed-in-fx-team] → [Australis:P5-][fixed-in-fx-team]
Attachment #8401173 - Flags: approval-mozilla-beta?
Attachment #8401173 - Flags: approval-mozilla-beta+
Attachment #8401173 - Flags: approval-mozilla-aurora?
Attachment #8401173 - Flags: approval-mozilla-aurora+
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
Attachment #8401173 - Flags: approval-mozilla-beta?
Attachment #8401173 - Flags: approval-mozilla-beta+
Attachment #8401173 - Flags: approval-mozilla-aurora?
Attachment #8401173 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/47ba4bce872f
Whiteboard: [Australis:P5-][fixed-in-fx-team] → [Australis:P5-]
Attachment #8401173 - Flags: approval-mozilla-beta?
Attachment #8401173 - Flags: approval-mozilla-beta+
Attachment #8401173 - Flags: approval-mozilla-aurora?
Attachment #8401173 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Needs to land in Beta as well because of bug 963576 comment 43.
Keywords: checkin-needed
Blocks: 992593
(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
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.
Flags: needinfo?(jbeich)
(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.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1329895
Blocks: 1365026
Blocks: 1386812
You need to log in before you can comment on or make changes to this bug.