Closed
Bug 241070
Opened 21 years ago
Closed 20 years ago
Refactor nsNativeThemeGTK
Categories
(Core Graveyard :: GFX: Gtk, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: neil, Assigned: npeninguy)
References
Details
(Keywords: helpwanted, Whiteboard: [good first bug])
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Spun off from bug 237138 comment 18 but see also bug 232175.
Updated•21 years ago
|
Assignee: themes → blizzard
Component: Themes → GFX: Gtk
QA Contact: ian
Comment 1•21 years ago
|
||
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.
Updated•21 years ago
|
Whiteboard: [good first bug]
Assignee | ||
Comment 2•21 years ago
|
||
First mozilla bug and minimal refactoring. Is it ok or should I put more in nsNativeTheme to keep nsNativeTheme::CheckBooleanAttr() and others private ?
Assignee | ||
Comment 3•21 years ago
|
||
Same patch but with "cvs diff -u8pN"
Attachment #151625 -
Attachment is obsolete: true
Assignee | ||
Comment 4•21 years ago
|
||
Comment on attachment 152353 [details] [diff] [review] Patch Requesting review
Attachment #152353 -
Flags: review?
Comment 5•20 years ago
|
||
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...
Assignee | ||
Comment 6•20 years ago
|
||
Use IsWidgetStyled() like other implementations. Thanks Boris I missed that one. Now HTML widgets use native theme! :)
Attachment #152353 -
Attachment is obsolete: true
Assignee | ||
Comment 7•20 years ago
|
||
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 8•20 years ago
|
||
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+
Comment 9•20 years ago
|
||
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...
Assignee | ||
Comment 10•20 years ago
|
||
Boris, Brian, you are right. I've seen some problems on checkbox (cannot see if it's selected or not)...
Assignee | ||
Updated•20 years ago
|
Attachment #156090 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #157106 -
Flags: superreview?(blizzard)
Attachment #157106 -
Flags: review?(bryner)
Updated•20 years ago
|
Attachment #157106 -
Flags: superreview?(blizzard) → superreview+
Assignee | ||
Comment 11•20 years ago
|
||
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?
Reporter | ||
Comment 12•20 years ago
|
||
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.
Assignee | ||
Comment 13•20 years ago
|
||
So should I modify nsNativeTheme's GetPrimaryPresShell() implementation and rename the method (To GetBestPresShell() ?) in my patch or address this in another bug ?
Assignee | ||
Comment 15•20 years ago
|
||
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
Comment 16•20 years ago
|
||
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
Updated•20 years ago
|
Attachment #156090 -
Flags: superreview?(blizzard)
Updated•20 years ago
|
Attachment #152353 -
Flags: review?
Updated•20 years ago
|
Attachment #157106 -
Flags: review?(bryner)
Updated•20 years ago
|
Assignee: blizzard → npeninguy
Assignee | ||
Comment 17•20 years ago
|
||
Thanks! Marking as FIXED...
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•