Closed
Bug 669028
Opened 12 years ago
Closed 12 years ago
DeCOMtaminate nsILookAndFeel
Categories
(Core :: Widget, enhancement)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
Attachments
(16 files, 3 obsolete files)
I think that we can deCOMtaminate nsILookAndFeel because it looks like all users are in libxul. nsXPLookAndFeel inherits nsILookAndFeel and nsIObserver. The observer is used for observing pref change. This can be replaced with observer function of mozilla::Preferences. And nsLookAndFeel instance can be singleton. Therefore, I think we can deCOMtaminate it. I think we should rename nsLookAndFeel and nsILookAndFeel to nsLookAndFeelImpl and nsLookAndFeel (or mozilla::widget::LookAndFeelImpl and mozilla::widget::LookAndFeel?). I.e., nsLookAndFeelImpl inherits nsXPLookAndFeel. nsXPLookAndFeel inherits nsLookAndFeel. How do you think, roc?
Assignee | ||
Comment 1•12 years ago
|
||
Or, could we get rid of virtual calls if we would make nsXPLookAndFeel be another singleton class and nsLookAndFeel would access it?
Blocks: deCOM
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #543719 -
Flags: review?(roc)
Comment on attachment 543719 [details] [diff] [review] Part.1 Remove nsIObserver from nsXPLookAndFeel Review of attachment 543719 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/src/xpwidgets/nsXPLookAndFeel.cpp @@ +379,5 @@ > + // XXX If we could reorganize the pref names, we should separate the branch > + // for each types. Then, we could reduce the unnecessary loop from > + // nsXPLookAndFeel::OnPrefChanged(). > + > + Preferences::RegisterCallback((PrefChangedFunc)OnPrefChanged, "ui.", this); Instead of doing this cast, it's better to make OnPrefChanged use the standard signature and cast the instance parameter internally.
Attachment #543719 -
Flags: review?(roc) → review+
Comment on attachment 543719 [details] [diff] [review] Part.1 Remove nsIObserver from nsXPLookAndFeel Review of attachment 543719 [details] [diff] [review]: ----------------------------------------------------------------- oops, didn't mean to r+ that
Attachment #543719 -
Flags: review+ → review?
Comment 5•12 years ago
|
||
Why not mozilla::widget::LookAndFeel?
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Ms2ger from comment #5) > Why not mozilla::widget::LookAndFeel? I'm still unsure about that. "LookAndFeel" is enough name to be understood. And I think that the meaning of "widget" isn't related with LookAndFeel (in source code level, it's not so, though). I'd like to hear other people suggestions too.
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #543719 -
Attachment is obsolete: true
Attachment #556445 -
Flags: review?(roc)
Attachment #543719 -
Flags: review?
Assignee | ||
Comment 8•12 years ago
|
||
This patch makes the nsLookAndFeel be singleton class. I think that this is the simplest behavior after DeCOMtaminated.
Attachment #556446 -
Flags: review?(roc)
Assignee | ||
Comment 9•12 years ago
|
||
This patch makes mozilla::LookAndFeel for new APIs. The new methods have same name as mozilla::Preferences and the params are also similar. The new IDs are defined as ColorID, IntID and FloatID. And these values are defined as beginning with "eColorID_", "eIntID_" and "eFloatID_". I made all patches for this bug (from part.1 to part.15), but I want to finish review of this first before I post the others. You can see all patched in: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=a9c55d044bb7
Attachment #556448 -
Flags: review?(roc)
Attachment #556445 -
Flags: review?(roc) → review+
Comment on attachment 556446 [details] [diff] [review] Part.2 Make nsLookAndFeel be singleton Review of attachment 556446 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/src/xpwidgets/nsXPLookAndFeel.cpp @@ +446,4 @@ > > nsXPLookAndFeel::~nsXPLookAndFeel() > { > + if (sInstance == this) { Can't we just assert "sInstance == this"?
Comment on attachment 556448 [details] [diff] [review] Part.3 Implement mozilla::LookAndFeel which has static members Review of attachment 556448 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/public/nsILookAndFeel.h @@ +869,5 @@ > + static PRBool GetEchoPassword(); > + > + /** > + * When system look and feel is changed, Refresh() must be called. Then, > + * cached data would be released. "will be"
Attachment #556448 -
Flags: superreview?(matspal)
Attachment #556448 -
Flags: review?(roc)
Attachment #556448 -
Flags: review+
Assignee | ||
Comment 12•12 years ago
|
||
Sure.
Attachment #556446 -
Attachment is obsolete: true
Attachment #556477 -
Flags: review?(roc)
Attachment #556446 -
Flags: review?(roc)
Attachment #556477 -
Flags: review?(roc) → review+
Comment 13•12 years ago
|
||
Comment on attachment 556448 [details] [diff] [review] Part.3 Implement mozilla::LookAndFeel which has static members >widget/public/nsILookAndFeel.h >+class LookAndFeel >+{ >+public: >+ // When modifying this list, also modify nsXPLookAndFeel::sColorPrefs >+ // in widget/src/xpwidgts/nsXPLookAndFeel.cpp. You should also point out that nsILookAndFeel::nsColorID needs to be modified. That is, comments in all three places should mention the other two places. Also, I think we should assert it like so: PR_STATIC_ASSERT(eColor_LAST_COLOR == NS_ARRAY_LENGTH(sColorPrefs)); PR_STATIC_ASSERT(PRUint32(eColor_LAST_COLOR) == PRUint32(LookAndFeel::eColorID_LAST_COLOR)); >+ typedef enum { ... >+ } ColorID; While you're cleaning up these enum types, please declare all the new enums in LookAndFeel using this form: enum ColorID { ... }; >+ eIntID_WindowsThemeIdentifier >+ } IntID; It might be worth adding eIntID_LAST / eMetric_LAST and PR_STATIC_ASSERT that they are equal. Same for the other enums. FYI, an alternative to having separate enums that must be kept in sync is using "static const" like I did in layout/generic/nsIFrame.h for kPrincipalList et al. >+#ifdef NS_DEBUG >+ typedef enum { >+ eNavWidgetID_TextField = 0, ... >+#endif // NS_DEBUG Do we really need to introduce this DEBUG stuff in LookAndFeel? I can't find any consumers of nsILookAndFeel::GetNavSize in the tree. >widget/src/xpwidgets/nsXPLookAndFeel.cpp >+LookAndFeel::GetColor(LookAndFeel::ColorID aID, nscolor* aResult) You can just use ColorID there, no need for the class prefix. >+{ >+ NS_ENSURE_ARG_POINTER(aResult); >+ nsLookAndFeel* lookAndFeel = nsLookAndFeel::GetInstance(); >+ NS_ENSURE_TRUE(lookAndFeel, NS_ERROR_NOT_AVAILABLE); >+ return lookAndFeel->GetColor((nsILookAndFeel::nsColorID)aID, *aResult); >+} Why not just: return nsLookAndFeel::GetInstance()-> GetColor((nsILookAndFeel::nsColorID)aID, *aResult); If I understand it correctly mozilla::LookAndFeel is intended as an internal API and thus we can just crash safely if someone gives us a null-pointer. I for one would much rather have an obvious crash if I make an error than a console warning. Finally, to reflect reality, please make this change as well: > widget/public/nsILookAndFeel.h -/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ sr=mats
Attachment #556448 -
Flags: superreview?(matspal) → superreview+
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Mats Palmgren [:mats] from comment #13) > Comment on attachment 556448 [details] [diff] [review] > Part.3 Implement mozilla::LookAndFeel which has static members > > >widget/public/nsILookAndFeel.h > >+class LookAndFeel > >+{ > >+public: > >+ // When modifying this list, also modify nsXPLookAndFeel::sColorPrefs > >+ // in widget/src/xpwidgts/nsXPLookAndFeel.cpp. > > You should also point out that nsILookAndFeel::nsColorID needs to be > modified. That is, comments in all three places should mention the > other two places. Also, I think we should assert it like so: > PR_STATIC_ASSERT(eColor_LAST_COLOR == NS_ARRAY_LENGTH(sColorPrefs)); > PR_STATIC_ASSERT(PRUint32(eColor_LAST_COLOR) == > PRUint32(LookAndFeel::eColorID_LAST_COLOR)); It's not necessary because nsILookAndFeel will be removed by part.15 of the series of patch for this bug. And all of them would be landed at same time. See comment 9's tryserver URL.
Comment 15•12 years ago
|
||
Great, I didn't notice that.
Assignee | ||
Comment 16•12 years ago
|
||
Sure. Thank you for your sr.
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #556448 -
Attachment is obsolete: true
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #558703 -
Flags: review?(roc)
Assignee | ||
Comment 19•12 years ago
|
||
If nsLookAndFeel::GetInt() failed, mozilla::widget::LookAndFeel::GetInt() would return zero.
Attachment #558704 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 20•12 years ago
|
||
If nsLookAndFeel::GetMetric() failed, the new API would return 0 (the second parameter). I'll post a -w patch for you.
Attachment #558705 -
Flags: review?(benjamin)
Assignee | ||
Comment 21•12 years ago
|
||
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #558707 -
Flags: review?(roc)
Assignee | ||
Comment 23•12 years ago
|
||
Attachment #558709 -
Flags: review?(enndeakin)
Updated•12 years ago
|
Attachment #558704 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 24•12 years ago
|
||
> - nsCOMPtr<nsILookAndFeel> look = do_GetService(kLookAndFeelCID);
> - NS_ASSERTION(look, "Look and feel service must be implemented for this toolkit");
> -
> - PRInt32 xThreshold=1, yThreshold=1;
> - look->GetMetric(nsILookAndFeel::eMetric_DragThresholdX, xThreshold);
> - look->GetMetric(nsILookAndFeel::eMetric_DragThresholdY, yThreshold);
> + PRInt32 xThreshold =
> + LookAndFeel::GetInt(LookAndFeel::eIntID_DragThresholdX, 1);
> + PRInt32 yThreshold =
> + LookAndFeel::GetInt(LookAndFeel::eIntID_DragThresholdY, 1);
This changes the logic. nsLookAndFeel::GetMetric() returns 0 even when the metric isn't supported (note that the metrics are supported on Windows, Mac and GTK2 only). So, initializing the variables doesn't make sense. I just respect the intention of the original author. I.e., the new API returns 1 (the second parameter) if the metrics are not supported on the platform. If you don't like to change the behavior, let me know.
Attachment #558710 -
Flags: review?(ehsan)
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #558712 -
Flags: review?(benjamin)
Assignee | ||
Comment 26•12 years ago
|
||
I used "replace" of my editor for changing kColorKTable.
Attachment #558714 -
Flags: review?(dbaron)
Attachment #558703 -
Flags: review?(roc) → review+
Attachment #558707 -
Flags: review?(roc) → review+
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #558715 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 28•12 years ago
|
||
We can remove nsPresContext::mLookAndFeel :-)
Attachment #558717 -
Flags: review?(roc)
Attachment #558717 -
Flags: review?(roc) → review+
![]() |
||
Comment 29•12 years ago
|
||
Comment on attachment 558715 [details] [diff] [review] Patch part.12 layout/xul should use mozilla::LookAndFeel rather than nsILookAndFeel It might be ~2 weeks before I get to this...
Assignee | ||
Comment 30•12 years ago
|
||
Attachment #558719 -
Flags: review?(neil)
Attachment #558715 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 31•12 years ago
|
||
Removing nsILookAndFeel interface and replacing all metric IDs from old name to new one. The each virtual methods of nsLookAndFeel renamed with "Impl" suffix because the names conflict with mozilla::LookAndFeel's static methods. And also this patch removes the debug methods (GetNavSize()) which are not used on current tree.
Attachment #558722 -
Flags: superreview?(matspal)
Attachment #558722 -
Flags: review?(roc)
Attachment #558722 -
Flags: review?(roc) → review+
Comment 32•12 years ago
|
||
Comment on attachment 558714 [details] [diff] [review] Patch part.11 layout/style should use mozilla;LookAndFeel rather than nsILookAndFeel r=dbaron
Attachment #558714 -
Flags: review?(dbaron) → review+
Comment 33•12 years ago
|
||
Comment on attachment 558722 [details] [diff] [review] Patch part.15 Remove nsILookAndFeel > widget/src/xpwidgets/nsXPLookAndFeel.cpp > nsLookAndFeel* > nsXPLookAndFeel::GetInstance() > { > if (sInstance) { > return static_cast<nsLookAndFeel*>(sInstance); > } > > NS_ENSURE_TRUE(!sShutdown, nsnull); > >- NS_ADDREF(sInstance = new nsLookAndFeel()); >+ sInstance = new nsLookAndFeel(); > return static_cast<nsLookAndFeel*>(sInstance); > } Can we make nsXPLookAndFeel::sInstance have the type nsLookAndFeel* and get rid of the static_casts? > LookAndFeel::GetColor(ColorID aID, nscolor* aResult) > { > NS_ENSURE_ARG_POINTER(aResult); I think we should remove this error handling and just crash if someone calls it with a null pointer. > LookAndFeel::GetInt(IntID aID, PRInt32* aResult) > LookAndFeel::GetFloat(FloatID aID, float* aResult) Same. >widget/src/xpwidgets/nsXPLookAndFeel.h > // > // All these routines will return NS_OK if they have a value, > // in which case the nsLookAndFeel should use that value; > // otherwise we'll return NS_ERROR_NOT_AVAILABLE, in which case, the > // platform-specific nsLookAndFeel should use its own values instead. > // >- NS_IMETHOD GetColor(const nsColorID aID, nscolor &aColor); >- NS_IMETHOD GetMetric(const nsMetricID aID, PRInt32 & aMetric); >- NS_IMETHOD GetMetric(const nsMetricFloatID aID, float & aMetric); >+ nsresult GetColorImpl(ColorID aID, nscolor &aResult); >+ virtual nsresult GetIntImpl(IntID aID, PRInt32 &aResult); >+ virtual nsresult GetFloatImpl(FloatID aID, float &aResult); I think we can simplify the code further by changing the return type to bool and return true where it now returns NS_OK. (not necessarily in this bug though)
Attachment #558722 -
Flags: superreview?(matspal) → superreview+
(In reply to Mats Palmgren [:mats] from comment #33) > I think we can simplify the code further by changing the return type > to bool and return true where it now returns NS_OK. I actually recommend against that. An nsresult is more suitable than a bool for indicating success/failure. For one thing, it's always possible to get confused about whether true means success or failure.
Updated•12 years ago
|
Attachment #558719 -
Flags: review?(neil) → review+
Comment 35•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34) > An nsresult is more suitable than a bool for indicating success/failure. But these methods can't really "fail". Perhaps a different method name could clarify the meaning of the returned bool, such as HasColor, FindColor ...
OK.
Updated•12 years ago
|
Attachment #558709 -
Flags: review?(enndeakin) → review+
Updated•12 years ago
|
Attachment #558710 -
Flags: review?(ehsan) → review+
Updated•12 years ago
|
Attachment #558705 -
Flags: review?(benjamin) → review+
Updated•12 years ago
|
Attachment #558712 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 37•12 years ago
|
||
try before landing: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=2de156ad9647 http://hg.mozilla.org/integration/mozilla-inbound/rev/71486524f1ed http://hg.mozilla.org/integration/mozilla-inbound/rev/7e1031d213ba http://hg.mozilla.org/integration/mozilla-inbound/rev/19b106d56f8a http://hg.mozilla.org/integration/mozilla-inbound/rev/f84313aa98a0 http://hg.mozilla.org/integration/mozilla-inbound/rev/4a86d875f876 http://hg.mozilla.org/integration/mozilla-inbound/rev/a2cae0d42d02 http://hg.mozilla.org/integration/mozilla-inbound/rev/99e282d11ef8 http://hg.mozilla.org/integration/mozilla-inbound/rev/7c9eaa95408c http://hg.mozilla.org/integration/mozilla-inbound/rev/d25a04e9a7bf http://hg.mozilla.org/integration/mozilla-inbound/rev/4442c70c63b8 http://hg.mozilla.org/integration/mozilla-inbound/rev/b81bce473c10 http://hg.mozilla.org/integration/mozilla-inbound/rev/36da7ff7039c http://hg.mozilla.org/integration/mozilla-inbound/rev/8f20dc5f4c8f http://hg.mozilla.org/integration/mozilla-inbound/rev/4a5d378339d9 http://hg.mozilla.org/integration/mozilla-inbound/rev/fd52770098ca > I think we can simplify the code further by changing the return type > to bool and return true where it now returns NS_OK. > (not necessarily in this bug though) Okay, I'll file it.
Flags: in-testsuite-
Whiteboard: [inbound]
Comment 38•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/71486524f1ed http://hg.mozilla.org/mozilla-central/rev/7e1031d213ba http://hg.mozilla.org/mozilla-central/rev/19b106d56f8a http://hg.mozilla.org/mozilla-central/rev/f84313aa98a0 http://hg.mozilla.org/mozilla-central/rev/4a86d875f876 http://hg.mozilla.org/mozilla-central/rev/a2cae0d42d02 http://hg.mozilla.org/mozilla-central/rev/99e282d11ef8 http://hg.mozilla.org/mozilla-central/rev/7c9eaa95408c http://hg.mozilla.org/mozilla-central/rev/d25a04e9a7bf http://hg.mozilla.org/mozilla-central/rev/4442c70c63b8 http://hg.mozilla.org/mozilla-central/rev/b81bce473c10 http://hg.mozilla.org/mozilla-central/rev/36da7ff7039c http://hg.mozilla.org/mozilla-central/rev/8f20dc5f4c8f http://hg.mozilla.org/mozilla-central/rev/4a5d378339d9 http://hg.mozilla.org/mozilla-central/rev/fd52770098ca
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Comment 39•12 years ago
|
||
Bah, I can't find out the alert notification origin from outside libxul :-(
Assignee | ||
Comment 40•12 years ago
|
||
If there are some needs to get metrics from outside libxul, we should make a new XPCOM service which would use widget::LookAndFeel.
You need to log in
before you can comment on or make changes to this bug.
Description
•