241070 - Refactor nsNativeThemeGTK

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[[e-mail]]

Spun off from bug 237138 comment 18 but see also bug 232175.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that bug 237138 was only fixed in the nsNativeTheme code on trunk, NOT in
nsNativeThemeGTK.   So GTK will not pick up this fix till this bug is fixed.

Comment 2

[Nicolas PENINGUY]

Attachment: Patch

First mozilla bug and minimal refactoring. Is it ok or should I put more in
nsNativeTheme to keep nsNativeTheme::CheckBooleanAttr() and others private ?

Comment 3

[Nicolas PENINGUY]

Attachment: Patch

Same patch but with "cvs diff -u8pN"

Comment 4

[Nicolas PENINGUY]

Comment on attachment 152353 [details] [diff] [review]
Patch

Requesting review

Comment 5

[Boris Zbarsky [:bzbarsky]]

Nicolas, would you mind also changing ThemeSupportsWidget to call
IsWidgetStyled() like the other implementations do?

Also, for future reference, you need to ask for review from someone in
particular; otherwise no one gets the review request mail...

Comment 6

[Nicolas PENINGUY]

Attachment: new patch

Use IsWidgetStyled() like other implementations. Thanks Boris I missed that
one. Now HTML widgets use native theme! :)

Comment 7

[Nicolas PENINGUY]

Comment on attachment 156090 [details] [diff] [review]
new patch

Requesting review. Note I won't be very responsive for a week.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 156090 [details] [diff] [review]
new patch

Turning on native theming for HTML controls is somewhat risky and not
well-tested.  I'm not comfortable with doing that until I've at least had a
chance to make sure everything looks ok (I suspect there are problems).  r=me
on everything except the few lines in ThemeSupportsWidget().

Comment 9

[Boris Zbarsky [:bzbarsky]]

If anyone cares about my $0.02, I'd also says that turning on native theming of
HTML controls should be a separate bug, and that it's very regression-prone. 
The ThemeSupportsWidget() code should return !IsWidgetStyled() where it
currently returns PR_TRUE; that will drop native theming on styled widgets but
not enable it for anything that wasn't enabled before...

Comment 10

[Nicolas PENINGUY]

Attachment: adress review comments

Boris, Brian, you are right. I've seen some problems on checkbox (cannot see if
it's selected or not)...

Comment 11

[Nicolas PENINGUY]

Brian, you recently changed GetPrimaryPresShell(nsIFrame* aFrame)
implementation in nsNativeThemeGTK.cpp, but my patch is going to
throw it away and use the implementation provided by nsNativeTheme.cpp...
Is it ok or not?

Comment 12

[[e-mail]]

dbaron says the version in nsNativeThemeGTK is better, but it is badly named as
it actually finds the best pres shell, which theoretically may not be primary.

Comment 13

[Nicolas PENINGUY]

So should I modify nsNativeTheme's GetPrimaryPresShell() implementation and
rename the method (To GetBestPresShell() ?) in my patch or address this in
another bug ?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd say use the copy in nsNativeThemeGTK and call it GetPresShell.

Comment 15

[Nicolas PENINGUY]

Attachment: new version

Updated to latest trunk, and follow dbaron advice. Can someone with cvs access
commit it please ? (or should it be re-reviewed ?)

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

checked into trunk, please mark fixed if it is

Checking in gfx/src/gtk/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/gfx/src/gtk/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.60; previous revision: 1.59
done
Checking in gfx/src/gtk/nsNativeThemeGTK.h;
/cvsroot/mozilla/gfx/src/gtk/nsNativeThemeGTK.h,v  <--  nsNativeThemeGTK.h
new revision: 1.25; previous revision: 1.24
done
Checking in gfx/src/shared/Makefile.in;
/cvsroot/mozilla/gfx/src/shared/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
Checking in gfx/src/shared/nsNativeTheme.cpp;
/cvsroot/mozilla/gfx/src/shared/nsNativeTheme.cpp,v  <--  nsNativeTheme.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in gfx/src/shared/nsNativeTheme.h;
/cvsroot/mozilla/gfx/src/shared/nsNativeTheme.h,v  <--  nsNativeTheme.h
new revision: 1.10; previous revision: 1.9
done

Comment 17

[Nicolas PENINGUY]

Thanks! Marking as FIXED...

Comment 18

[Reed Loden [:reed]]

This caused bug 264164.