DeCOMtaminate nsILookAndFeel

RESOLVED FIXED in mozilla9

Status

()

Core
Widget
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 3 obsolete attachments)

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
bsmedberg
: 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
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
6.57 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
2.23 KB, patch
bsmedberg
: 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@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
182.68 KB, patch
roc
: review+
Mats Palmgren (vacation - back in August)
: 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?
Blocks: 105431
Created attachment 543719 [details] [diff] [review]
Part.1 Remove nsIObserver from nsXPLookAndFeel
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?
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.
Created attachment 556445 [details] [diff] [review]
Part.1 Remove nsIObserver from nsXPLookAndFeel
Attachment #543719 - Attachment is obsolete: true
Attachment #556445 - Flags: review?(roc)
Attachment #543719 - Flags: review?
Created attachment 556446 [details] [diff] [review]
Part.2 Make nsLookAndFeel be singleton

This patch makes the nsLookAndFeel be singleton class. I think that this is the simplest behavior after DeCOMtaminated.
Attachment #556446 - Flags: review?(roc)
Created attachment 556448 [details] [diff] [review]
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
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+
Created attachment 556477 [details] [diff] [review]
Part.2 Make nsLookAndFeel be singleton

Sure.
Attachment #556446 - Attachment is obsolete: true
Attachment #556477 - Flags: review?(roc)
Attachment #556446 - Flags: review?(roc)
Attachment #556477 - Flags: review?(roc) → review+
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.
Created attachment 558702 [details] [diff] [review]
Part.3 Implement mozilla::LookAndFeel which has static members
Attachment #556448 - Attachment is obsolete: true
Created attachment 558703 [details] [diff] [review]
Patch part.4 Rename nsILookAndFeel.h to LookAndFeel.h and widget should use mozilla::LookAndFeel rather than nsILookAndFeel
Attachment #558703 - Flags: review?(roc)
Created attachment 558704 [details] [diff] [review]
Patch part.5 accessible should use mozilla::LookAndFeel rather than nsILookAndFeel

If nsLookAndFeel::GetInt() failed, mozilla::widget::LookAndFeel::GetInt() would return zero.
Attachment #558704 - Flags: review?(surkov.alexander)
Created attachment 558705 [details] [diff] [review]
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.
Attachment #558705 - Flags: review?(benjamin)
Created attachment 558706 [details] [diff] [review]
-w patch of part.6
Created attachment 558707 [details] [diff] [review]
Patch part.7 content should use mozilla::LookAndFeel rather than nsILookAndFeel
Attachment #558707 - Flags: review?(roc)
Created attachment 558709 [details] [diff] [review]
Patch part.8 dom should use mozilla::LookAndFeel rather than nsILookAndFeel
Attachment #558709 - Flags: review?(enndeakin)

Updated

6 years ago
Attachment #558704 - Flags: review?(surkov.alexander) → review+
Created attachment 558710 [details] [diff] [review]
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.
Attachment #558710 - Flags: review?(ehsan)
Created attachment 558712 [details] [diff] [review]
Patch part.10 embedding should use mozilla::LookAndFeel rather than nsILookAndFeel
Attachment #558712 - Flags: review?(benjamin)
Created attachment 558714 [details] [diff] [review]
Patch part.11 layout/style should use mozilla;LookAndFeel rather than nsILookAndFeel

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+
Created attachment 558715 [details] [diff] [review]
Patch part.12 layout/xul should use mozilla::LookAndFeel rather than nsILookAndFeel
Attachment #558715 - Flags: review?(bzbarsky)
Created attachment 558717 [details] [diff] [review]
Patch part.13 layout should use mozilla::LookAndFeel rather than nsILookAndFeel

We can remove nsPresContext::mLookAndFeel :-)
Attachment #558717 - Flags: review?(roc)
Attachment #558717 - Flags: review?(roc) → review+

Comment 29

6 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...
Created attachment 558719 [details] [diff] [review]
Patch part.14 toolkit should use mozilla::LookAndFeel rather than nsILookAndFeel
Attachment #558719 - Flags: review?(neil)
Attachment #558715 - Flags: review?(bzbarsky) → review+
Created attachment 558722 [details] [diff] [review]
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.
Attachment #558722 - Flags: superreview?(matspal)
Attachment #558722 - Flags: review?(roc)
Attachment #558722 - Flags: review?(roc) → review+
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.

Updated

6 years ago
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 ...
OK.
Attachment #558709 - Flags: review?(enndeakin) → review+
Attachment #558710 - Flags: review?(ehsan) → review+
Attachment #558705 - Flags: review?(benjamin) → review+
Attachment #558712 - Flags: review?(benjamin) → review+
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]
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 686257
Whiteboard: [inbound]
Target Milestone: --- → mozilla9

Updated

6 years ago
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.