669028 - DeCOMtaminate nsILookAndFeel

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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?

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Or, could we get rid of virtual calls if we would make nsXPLookAndFeel be another singleton class and nsLookAndFeel would access it?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Part.1 Remove nsIObserver from nsXPLookAndFeel

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Why not mozilla::widget::LookAndFeel?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(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.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Part.1 Remove nsIObserver from nsXPLookAndFeel

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Part.2 Make nsLookAndFeel be singleton

This patch makes the nsLookAndFeel be singleton class. I think that this is the simplest behavior after DeCOMtaminated.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Part.3 Implement mozilla::LookAndFeel which has static members

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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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"

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Part.2 Make nsLookAndFeel be singleton

Sure.

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(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

[Mats Palmgren (inactive)]

Great, I didn't notice that.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Sure. Thank you for your sr.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Part.3 Implement mozilla::LookAndFeel which has static members

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.4 Rename nsILookAndFeel.h to LookAndFeel.h and widget should use mozilla::LookAndFeel rather than nsILookAndFeel

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.5 accessible should use mozilla::LookAndFeel rather than nsILookAndFeel

If nsLookAndFeel::GetInt() failed, mozilla::widget::LookAndFeel::GetInt() would return zero.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.6 chrome should use mozilla::LookAndFeel rather than nsILookAndFeel

If nsLookAndFeel::GetMetric() failed, the new API would return 0 (the second parameter). I'll post a -w patch for you.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: -w patch of part.6

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.7 content should use mozilla::LookAndFeel rather than nsILookAndFeel

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.8 dom should use mozilla::LookAndFeel rather than nsILookAndFeel

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch Bug 669028 part.9 editor should use mozilla::LookAndFeel rather than nsILookAndFeel

> -    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.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.10 embedding should use mozilla::LookAndFeel rather than nsILookAndFeel

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.11 layout/style should use mozilla;LookAndFeel rather than nsILookAndFeel

I used "replace" of my editor for changing kColorKTable.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.12 layout/xul should use mozilla::LookAndFeel rather than nsILookAndFeel

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.13 layout should use mozilla::LookAndFeel rather than nsILookAndFeel

We can remove nsPresContext::mLookAndFeel :-)

Comment 29

[Boris Zbarsky [:bzbarsky]]

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...

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.14 toolkit should use mozilla::LookAndFeel rather than nsILookAndFeel

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch part.15 Remove nsILookAndFeel

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.

Comment 32

[David Baron :dbaron:]

Comment on attachment 558714 [details] [diff] [review]
Patch part.11 layout/style should use mozilla;LookAndFeel rather than nsILookAndFeel

r=dbaron

Comment 33

[Mats Palmgren (inactive)]

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)

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 35

[Mats Palmgren (inactive)]

(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 ...

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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.

Comment 38

[(no longer active)]

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

Comment 39

[[e-mail]]

Bah, I can't find out the alert notification origin from outside libxul :-(

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

If there are some needs to get metrics from outside libxul, we should make a new XPCOM service which would use widget::LookAndFeel.