Move GetSystemFont from nsIDeviceContext to nsILookAndFeel (?)

RESOLVED FIXED in mozilla13

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: dbaron, Assigned: zwol)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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?
(Reporter)

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5

Comment 1

16 years ago
Very reasonable.  I didn't notice it was mostly deprecated.
(Reporter)

Comment 2

16 years ago
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.
(Reporter)

Comment 3

16 years ago
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

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

Comment 5

16 years ago
Sounds fine to me and it seems like it should probably go there
(Reporter)

Updated

16 years ago
Blocks: 33313
(Reporter)

Comment 6

16 years ago
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.
(Reporter)

Comment 7

16 years ago
Created attachment 50193 [details] [diff] [review]
Changing GetSystemAttribute to GetSystemFont
(Reporter)

Comment 8

16 years ago
Created attachment 51343 [details] [diff] [review]
updated patch
(Reporter)

Comment 9

16 years ago
In that patch, I need to add |#include "nsSize.h"| to nsLookAndFeel.cpp on the mac.
Comment on attachment 51343 [details] [diff] [review]
updated patch

r=bryner
Attachment #51343 - Flags: review+

Comment 11

16 years ago
Comment on attachment 51343 [details] [diff] [review]
updated patch

sr=waterson
Attachment #51343 - Flags: superreview+
(Reporter)

Comment 12

16 years ago
That patch is checked in 2001-10-01 20:10 PDT.

The issue still remains of moving things from nsIDeviceContext to nsILookAndFeel.
(Reporter)

Updated

16 years ago
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Reporter)

Updated

16 years ago
Summary: Remove nsIDeviceContext::GetSystemAttribute → Move GetSystemFont from nsIDeviceContext to nsILookAndFeel (?)
Target Milestone: mozilla0.9.6 → Future
(Reporter)

Updated

10 years ago
Assignee: dbaron → nobody
Status: ASSIGNED → NEW
QA Contact: ian → style-system
(Assignee)

Comment 13

6 years ago
This is on my list of things that remain to be done to get rid of the device context, so claiming.
Assignee: nobody → zackw
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Blocks: 651016
(Assignee)

Comment 14

5 years ago
Created attachment 598771 [details] [diff] [review]
move GetSystemFont to LookAndFeel

As promised.  Guessing at reviewer - please feel free to reassign.
Attachment #598771 - Flags: review?(roc)
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.
Attachment #598771 - Flags: review?(roc) → review+
(Assignee)

Comment 16

5 years ago
Created attachment 598897 [details] [diff] [review]
move GetSystemFont to LookAndFeel (r2)

Ok, I'll land this revision if the try server likes it.
Attachment #598771 - Attachment is obsolete: true
Attachment #598897 - Flags: review+
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.
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
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.
Attachment #598981 - Flags: review?(doug.turner)
(Assignee)

Updated

5 years ago
Attachment #598897 - Attachment is obsolete: true
Attachment #598897 - Flags: review+
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.
(Assignee)

Comment 21

5 years ago
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
Attachment #598981 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 22

5 years ago
(Sheriff: Go ahead and close this bug on merge.  The follow-up work mentioned in comment 18 will happen in a new bug.)
> (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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla13
(Assignee)

Comment 24

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.