Themes should be consistent between different GTK platforms

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Theme
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: Jan Beich, Assigned: Jan Beich)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 29
x86_64
FreeBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 fixed, firefox31 verified)

Details

(Whiteboard: [Australis:P5-])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 8345895 [details] [diff] [review]
Share Linux theme on every GTK platform.
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 6

5 years ago
Created attachment 8346619 [details] [diff] [review]
Test toolkit, not platform, for linux theme. v1

(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)
(Assignee)

Comment 7

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #8346619 - Attachment is obsolete: true
Attachment #8346619 - Attachment is patch: true
(Assignee)

Comment 8

5 years ago
Created attachment 8346640 [details] [diff] [review]
Define XP_LINUX to have consistent "linux" theme on non-Linux platforms.
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 12

5 years ago
Created attachment 8347190 [details] [diff] [review]
Test toolkit, not platform, for linux theme. v1.1
Attachment #8346619 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
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]
(Assignee)

Comment 14

5 years ago
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]
(Assignee)

Updated

4 years ago
Target Milestone: --- → Firefox 29
(Assignee)

Comment 17

4 years ago
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)
(Assignee)

Comment 19

4 years ago
Created attachment 8400251 [details] [diff] [review]
bug963576 followup

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

Comment 20

4 years ago
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+
(Assignee)

Comment 22

4 years ago
Created attachment 8401173 [details] [diff] [review]
bug963576 followup

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
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 23

4 years ago
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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 28

4 years ago
Needs to land in Beta as well because of bug 963576 comment 43.
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
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
status-firefox29: --- → fixed
status-firefox31: --- → fixed
Keywords: checkin-needed
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.
(Assignee)

Comment 34

4 years ago
Confirm to work as expected on self-built ff31 and ff29, also on linux ff29 from ftp.m.o.
status-firefox29: fixed → verified
status-firefox31: fixed → verified
Flags: needinfo?(jbeich)
(Assignee)

Comment 35

2 years ago
(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
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Blocks: 1329895
(Assignee)

Updated

a year ago
Blocks: 1365026
(Assignee)

Updated

10 months ago
Blocks: 1386812
You need to log in before you can comment on or make changes to this bug.