96971 - Move GetSystemFont from nsIDeviceContext to nsILookAndFeel (?)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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]

Very reasonable.  I didn't notice it was mostly deprecated.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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]

It would be great if you could possibly smash bug 77941 while wandering over 
there in nsLookAndFeel.

Comment 5

[rods (gone)]

Sounds fine to me and it seems like it should probably go there

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: Changing GetSystemAttribute to GetSystemFont

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: updated patch

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In that patch, I need to add |#include "nsSize.h"| to nsLookAndFeel.cpp on the mac.

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 51343 [details] [diff] [review]
updated patch

r=bryner

Comment 11

[Chris Waterson]

Comment on attachment 51343 [details] [diff] [review]
updated patch

sr=waterson

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)]

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)]

Attachment: move GetSystemFont to LookAndFeel

As promised.  Guessing at reviewer - please feel free to reassign.

Comment 15

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

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)]

Attachment: move GetSystemFont to LookAndFeel (r2)

Ok, I'll land this revision if the try server likes it.

Comment 17

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

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)]

(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)]

Attachment: 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)]

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)]

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)]

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

> (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)]

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.