Closed Bug 241070 Opened 20 years ago Closed 20 years ago

Refactor nsNativeThemeGTK


(Core Graveyard :: GFX: Gtk, defect)

Windows 2000
Not set


(Not tracked)



(Reporter: neil, Assigned: npeninguy)



(Keywords: helpwanted, Whiteboard: [good first bug])


(1 file, 4 obsolete files)

Spun off from bug 237138 comment 18 but see also bug 232175.
Assignee: themes → blizzard
Component: Themes → GFX: Gtk
QA Contact: ian
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.
Blocks: 213747
Whiteboard: [good first bug]
Attached patch Patch (obsolete) — Splinter Review
First mozilla bug and minimal refactoring. Is it ok or should I put more in
nsNativeTheme to keep nsNativeTheme::CheckBooleanAttr() and others private ?
Attached patch Patch (obsolete) — Splinter Review
Same patch but with "cvs diff -u8pN"
Attachment #151625 - Attachment is obsolete: true
Comment on attachment 152353 [details] [diff] [review]

Requesting review
Attachment #152353 - Flags: review?
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...
Attached patch new patch (obsolete) — Splinter Review
Use IsWidgetStyled() like other implementations. Thanks Boris I missed that
one. Now HTML widgets use native theme! :)
Attachment #152353 - Attachment is obsolete: true
Comment on attachment 156090 [details] [diff] [review]
new patch

Requesting review. Note I won't be very responsive for a week.
Attachment #156090 - Flags: superreview?(blizzard)
Attachment #156090 - Flags: review?(bryner)
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().
Attachment #156090 - Flags: review?(bryner) → review+
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...
Attached patch adress review comments (obsolete) — Splinter Review
Boris, Brian, you are right. I've seen some problems on checkbox (cannot see if
it's selected or not)...
Attachment #156090 - Attachment is obsolete: true
Attachment #157106 - Flags: superreview?(blizzard)
Attachment #157106 - Flags: review?(bryner)
Attachment #157106 - Flags: superreview?(blizzard) → superreview+
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?
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.
So should I modify nsNativeTheme's GetPrimaryPresShell() implementation and
rename the method (To GetBestPresShell() ?) in my patch or address this in
another bug ?
I'd say use the copy in nsNativeThemeGTK and call it GetPresShell.
Attached patch new versionSplinter Review
Updated to latest trunk, and follow dbaron advice. Can someone with cvs access
commit it please ? (or should it be re-reviewed ?)
Attachment #157106 - Attachment is obsolete: true
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
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
Checking in gfx/src/shared/;
/cvsroot/mozilla/gfx/src/shared/,v  <--
new revision: 1.9; previous revision: 1.8
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
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
Assignee: blizzard → npeninguy
Thanks! Marking as FIXED...
Closed: 20 years ago
Resolution: --- → FIXED
Blocks: 264164
This caused bug 264164.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.