Closed Bug 669028 Opened 13 years ago Closed 13 years ago

DeCOMtaminate nsILookAndFeel

Categories

(Core :: Widget, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(16 files, 3 obsolete files)

4.83 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.16 KB, patch
roc
: review+
Details | Diff | Splinter Review
18.66 KB, patch
Details | Diff | Splinter Review
16.29 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.03 KB, patch
surkov
: review+
Details | Diff | Splinter Review
2.94 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
2.67 KB, patch
Details | Diff | Splinter Review
5.21 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.60 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
6.57 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
2.23 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
27.20 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
20.56 KB, patch
roc
: review+
Details | Diff | Splinter Review
42.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.00 KB, patch
neil
: review+
Details | Diff | Splinter Review
182.68 KB, patch
roc
: review+
MatsPalmgren_bugz
: superreview+
Details | Diff | Splinter Review
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?
Or, could we get rid of virtual calls if we would make nsXPLookAndFeel be another singleton class and nsLookAndFeel would access it?
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?
Why not mozilla::widget::LookAndFeel?
(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.
Attachment #543719 - Attachment is obsolete: true
Attachment #556445 - Flags: review?(roc)
Attachment #543719 - Flags: review?
This patch makes the nsLookAndFeel be singleton class. I think that this is the simplest behavior after DeCOMtaminated.
Attachment #556446 - Flags: review?(roc)
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)
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+
Sure.
Attachment #556446 - Attachment is obsolete: true
Attachment #556477 - Flags: review?(roc)
Attachment #556446 - Flags: review?(roc)
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+
(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.
Great, I didn't notice that.
Sure. Thank you for your sr.
If nsLookAndFeel::GetInt() failed, mozilla::widget::LookAndFeel::GetInt() would return zero.
Attachment #558704 - Flags: review?(surkov.alexander)
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)
Attachment #558704 - Flags: review?(surkov.alexander) → review+
> -    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)
I used "replace" of my editor for changing kColorKTable.
Attachment #558714 - Flags: review?(dbaron)
We can remove nsPresContext::mLookAndFeel :-)
Attachment #558717 - Flags: review?(roc)
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...
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)
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 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.
Attachment #558719 - Flags: review?(neil) → review+
(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 ...
Attachment #558709 - Flags: review?(enndeakin) → review+
Attachment #558710 - Flags: review?(ehsan) → review+
Attachment #558705 - Flags: review?(benjamin) → review+
Attachment #558712 - Flags: review?(benjamin) → review+
Depends on: 686257
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Depends on: 687070
Bah, I can't find out the alert notification origin from outside libxul :-(
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.

Attachment

General

Created:
Updated:
Size: