Last Comment Bug 96971 - Move GetSystemFont from nsIDeviceContext to nsILookAndFeel (?)
: Move GetSystemFont from nsIDeviceContext to nsILookAndFeel (?)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla13
Assigned To: Zack Weinberg (:zwol)
:
Mentors:
Depends on:
Blocks: 651016 33313
  Show dependency treegraph
 
Reported: 2001-08-25 09:45 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2012-02-21 09:03 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Changing GetSystemAttribute to GetSystemFont (83.31 KB, patch)
2001-09-20 20:06 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
updated patch (85.10 KB, patch)
2001-09-28 16:51 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bryner: review+
waterson: superreview+
Details | Diff | Review
move GetSystemFont to LookAndFeel (130.10 KB, patch)
2012-02-19 22:27 PST, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Review
move GetSystemFont to LookAndFeel (r2) (130.09 KB, patch)
2012-02-20 09:30 PST, Zack Weinberg (:zwol)
no flags Details | Diff | Review
move GetSystemFont to LookAndFeel (r3) (130.99 KB, patch)
2012-02-20 15:03 PST, Zack Weinberg (:zwol)
zackw: review+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-08-25 09:45:33 PDT
I'd like to remove nsIDeviceContext::GetSystemAttribute (and replace it with
some additional code in nsILookAndFeel implementations) a few reasons:
 * It makes sense to have everything relating to system look&feel in one place
 * There's already a bit of duplicated code (metrics, colors)
 * It would make it much easier (well, I would be able to avoid duplicating a
lot more code) to implement system fonts correctly on GTK, since for GTK we have
to create native widgets and poke at their style information, and I'd rather not
duplicate all the creation/destruction code that already exists in
nsLookAndFeelGTK for the system colors implementation.
 * the interface of nsILookAndFeel is cleaner -- you don't have to use a
structure that contains a bunch of different things and then pass a pointer to
it to get one of them
 * It's easier for anyone to get an nsILookAndFeel, since it's a service.  If
performance is an issue, they can cache it (as the pres context does).

There are currently only three callers of nsIDeviceContext::GetSystemAttribute,
and one of the three creates a device context just to call it.

Does this seem reasonable?
Comment 1 Pierre Saslawsky 2001-08-25 11:38:02 PDT
Very reasonable.  I didn't notice it was mostly deprecated.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-08-25 11:51:56 PDT
I think the history looks like this:

July 29, 1998 (scullin) - original nsLookAndFeel implementations checked in

January 21, 1999 (rods) - duplicated some nsLookAndFeel stuff on / moved some
nsLookAndFeel stuff (very recently added) to nsDeviceContext, presumably because
it was hard to get hold of an nsLookAndFeel, since it was not a service, but had
to be retrieved from a pres context

July 28, 2000, bug 46445 (anthonyd) - changed nsLookAndFeel to be a service
rather than a normal component, so you wouldn't have to get it from the pres
context.

I think what anthonyd did is probably a better solution to the problem that
originally led these methods to be moved to nsDeviceContext (although I can only
guess at what that problem was).  So it's not really that the stuff on
nsDeviceContext is deprecated -- the nsLookAndFeel stuff was actually there
first.  It's just that it would be easier, both mentally, and for the GTK
implementation, to have all this stuff consolidated in one place.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-08-25 11:52:41 PDT
s/be a service/be used as a service/
(since being a service is enforced by the user rather than the component itself)
Comment 4 rbs 2001-08-26 04:25:35 PDT
It would be great if you could possibly smash bug 77941 while wandering over 
there in nsLookAndFeel.
Comment 5 rods (gone) 2001-08-27 08:11:01 PDT
Sounds fine to me and it seems like it should probably go there
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-09-20 13:23:38 PDT
I just had a look at this in a little more detail.  It's not quite as easy as I
thought, since some of the GetSystemAttribute implementations use member
variables of the device context -- in particular, BeOS and GTK use
mPixelsToTwips, and Windows also uses mDC.

So a first step, at least, would be shrinking the implementations only to deal
with system fonts.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-09-20 20:06:54 PDT
Created attachment 50193 [details] [diff] [review]
Changing GetSystemAttribute to GetSystemFont
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-09-28 16:51:12 PDT
Created attachment 51343 [details] [diff] [review]
updated patch
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-09-30 15:33:02 PDT
In that patch, I need to add |#include "nsSize.h"| to nsLookAndFeel.cpp on the mac.
Comment 10 Brian Ryner (not reading) 2001-09-30 17:26:42 PDT
Comment on attachment 51343 [details] [diff] [review]
updated patch

r=bryner
Comment 11 Chris Waterson 2001-10-01 18:06:32 PDT
Comment on attachment 51343 [details] [diff] [review]
updated patch

sr=waterson
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-10-01 20:43:48 PDT
That patch is checked in 2001-10-01 20:10 PDT.

The issue still remains of moving things from nsIDeviceContext to nsILookAndFeel.
Comment 13 Zack Weinberg (:zwol) 2011-04-18 19:48:48 PDT
This is on my list of things that remain to be done to get rid of the device context, so claiming.
Comment 14 Zack Weinberg (:zwol) 2012-02-19 22:27:12 PST
Created attachment 598771 [details] [diff] [review]
move GetSystemFont to LookAndFeel

As promised.  Guessing at reviewer - please feel free to reassign.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-19 23:22:03 PST
Comment on attachment 598771 [details] [diff] [review]
move GetSystemFont to LookAndFeel

Review of attachment 598771 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that fixed.

::: widget/android/nsLookAndFeel.cpp
@@ +478,5 @@
> +{
> +    // XXXzw This formerly hardwired Droid Sans for everything, with
> +    // garbage style parameters.  Until/unless it makes sense to
> +    // actually query the Android theme for the font, I think we're
> +    // better off just letting the style system use its default.

I think it would be better to leave this behavior alone.

::: widget/gonk/nsLookAndFeel.cpp
@@ +323,5 @@
> +{
> +    // XXXzw This formerly hardwired Droid Sans for everything, with
> +    // garbage style parameters.  Until/unless it makes sense to
> +    // actually query the Android theme for the font, I think we're
> +    // better off just letting the style system use its default.

Let's not change this.
Comment 16 Zack Weinberg (:zwol) 2012-02-20 09:30:46 PST
Created attachment 598897 [details] [diff] [review]
move GetSystemFont to LookAndFeel (r2)

Ok, I'll land this revision if the try server likes it.
Comment 17 :Ms2ger 2012-02-20 09:51:16 PST
Comment on attachment 598897 [details] [diff] [review]
move GetSystemFont to LookAndFeel (r2)

>--- a/layout/style/nsRuleNode.cpp
>+++ b/layout/style/nsRuleNode.cpp
>   // -moz-system-font: enum (never inherit!)
>-  nsFont systemFont;
>+  PR_STATIC_ASSERT(

MOZ_STATIC_ASSERT, please

>--- a/widget/cocoa/nsLookAndFeel.mm
>+++ b/widget/cocoa/nsLookAndFeel.mm
>+
>+// copied from gfxQuartzFontCache.mm, maybe should go in a Cocoa utils file somewhere
>+static void GetStringForNSString(const NSString *aSrc, nsAString& aDest)
>+{
>+    aDest.SetLength([aSrc length]);
>+    [aSrc getCharacters:aDest.BeginWriting()];

Is this code safe if the SetCapacity call in SetLength's implementation fails on OOM?

>+bool
>+nsLookAndFeel::GetFontImpl(FontID aID, nsString &aFontName,
>+                           gfxFontStyle &aFontStyle)
>+{
>+    switch (aID) {
>+        default:
>+            // should never hit this
>+            return NS_ERROR_FAILURE;

false, I think.
Comment 18 Zack Weinberg (:zwol) 2012-02-20 12:22:05 PST
(In reply to Ms2ger from comment #17)
> >+  PR_STATIC_ASSERT(
> 
> MOZ_STATIC_ASSERT, please

There are several other uses of PR_STATIC_ASSERT in this file.  I think it would be better to change all of them to MOZ_STATIC_ASSERT in a separate patch.  I have no objection to adding that patch to this bug, as long as dbaron also has no objection.

> >+    aDest.SetLength([aSrc length]);
> >+    [aSrc getCharacters:aDest.BeginWriting()];
> 
> Is this code safe if the SetCapacity call in SetLength's implementation
> fails on OOM?

It was like this already, I just moved it.

> >+bool
> >+nsLookAndFeel::GetFontImpl(FontID aID, nsString &aFontName,
> >+                           gfxFontStyle &aFontStyle)
> >+{
> >+    switch (aID) {
> >+        default:
> >+            // should never hit this
> >+            return NS_ERROR_FAILURE;
> 
> false, I think.

Thanks.  Fixed in my copy.
Comment 19 Zack Weinberg (:zwol) 2012-02-20 15:03:27 PST
Created attachment 598981 [details] [diff] [review]
move GetSystemFont to LookAndFeel (r3)

Here's another revision, which is all-green on try.  I'm not entirely comfortable checking this in, though.  While making the Android changes that roc asked for, I noticed that the Android implementation of GetSystemFont can't ever have worked -- it was returning default-initialized gfxFontStyle objects; in particular, ->size was set to *zero*.  Despite comments in various places to the contrary, calling code was *not* prepared to cope with that.

This revision of the patch causes the Android/XUL and Gonk implementations of (what is now) LookAndFeel::GetFont to return a gfxFontStyle that is how it appears to have been meant to be all along, but I do not have suitable hardware for testing it, and wouldn't really know where to look for a problem in any case.
Comment 20 Doug Turner (:dougt) 2012-02-20 15:08:08 PST
just confirming that this does look like it works fine on android native.  I didn't see anything obviously busted.  thanks for the heads up.
Comment 21 Zack Weinberg (:zwol) 2012-02-20 15:21:27 PST
Comment on attachment 598981 [details] [diff] [review]
move GetSystemFont to LookAndFeel (r3)

(In reply to Doug Turner (:dougt) from comment #20)
> just confirming that this does look like it works fine on android native.  I
> didn't see anything obviously busted.  thanks for the heads up.

OK, that's good enough for me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/027f56e65a84
Comment 22 Zack Weinberg (:zwol) 2012-02-20 15:22:25 PST
(Sheriff: Go ahead and close this bug on merge.  The follow-up work mentioned in comment 18 will happen in a new bug.)
Comment 23 Ed Morley [:emorley] 2012-02-21 08:45:12 PST
> (Sheriff: Go ahead and close this bug on merge.  The follow-up work
> mentioned in comment 18 will happen in a new bug.)

Thank you :-)

https://hg.mozilla.org/mozilla-central/rev/027f56e65a84
Comment 24 Zack Weinberg (:zwol) 2012-02-21 09:03:36 PST
That follow-up bug is bug 729142, for the record.  I'm not marking a dependency as there is no technical relationship between the two changes.

Note You need to log in before you can comment on or make changes to this bug.