The default bug view has changed. See this FAQ.

Status

()

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

People

(Reporter: zwol, Assigned: zwol)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Assignee)

Description

6 years ago
Didn't I just say (bug 651016) that nsIDeviceContext should be totally removed?  Yes, but deCOMming it first makes it easier to chainsaw out huge chunks (only one header to update, no worries about IIDs, etc).  Besides, I have that part done and it can go in now. :)
(Assignee)

Comment 1

6 years ago
Created attachment 526907 [details] [diff] [review]
part 1: include minimization

In addition to mechanical removal of #include "nsIDeviceContext.h" from as many files as possible, this moves the printing error codes to nsIDeviceContextSpec.h, removes some stale declarations from a few places, and removes some logic in os2/nsWindow.cpp that exactly duplicated the BaseCreate method provided by xpwidgets.
Attachment #526907 - Flags: review?(roc)
(Assignee)

Comment 2

6 years ago
Created attachment 526908 [details] [diff] [review]
part 2: better initialization for the gfx module

We shortly won't be able to count on XPCOM objects from the gfx module being instantiated anymore, so create a dummy service and register it for the app-startup category to ensure that gfxPlatform::Init() gets called early enough.

It might not be necessary to do_GetService this dummy service in libpr0n's module constructor, but I couldn't prove it to myself.
Attachment #526908 - Flags: review?(roc)
(Assignee)

Comment 3

6 years ago
Created attachment 526909 [details] [diff] [review]
part 3: make nsThebesDeviceContext not an nsIObserver

nsThebesDeviceContext objects register themselves as memory pressure observers so they can tell their font metrics caches to compact.  Instead, have the font metrics caches do that themselves.

Also flushes out the last remnants of gThebesGFXLog and a totally pointless call to ScriptRecordDigitSubstitution on Windows.
Attachment #526909 - Flags: review?(roc)
(Assignee)

Comment 4

6 years ago
Created attachment 526911 [details] [diff] [review]
part 4: deCOM and tidy

This does four things at once - quoting the patch header:

deCOM nsIDeviceContext; 
include-minimization on nsDeviceContext.h itself;
merge GetPrintDC() into Windows/OS/2 specific callers;
merge Shutdown() with ClearCachedSystemFonts().
Most references outside gfx not fixed up.
(Assignee)

Comment 5

6 years ago
Created attachment 526912 [details] [diff] [review]
part 5: mechanical fixups

perl -pi -e 's/\bns(?:I|Thebes)(?=DeviceContext\b/ns/g/' \
    $(grep -rEw 'ns(I|Thebes)DeviceContext' *)

perl -pi -e 's/\bnsCOMPtr(?=<nsDeviceContext>)/nsRefPtr/g' \
    $(grep -rE '\<nsCOMPtr<nsDeviceContext>' *)
Attachment #526912 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #526911 - Flags: review?(roc)
(Assignee)

Comment 6

6 years ago
Created attachment 526913 [details] [diff] [review]
part 6: prune unused/unimplemented methods, push some code down to nsFontCache

Only the font cache cares about the locale language, so the device context doesn't need to hold that pointer.
Attachment #526913 - Flags: review?(roc)
(Assignee)

Comment 7

6 years ago
Created attachment 526914 [details] [diff] [review]
part 7: simplify rendering context creation

Users of CreateRenderingContextInstance can just do 'new nsRenderingContext()' now.  The CreateRenderingContext(nsIDeviceContext*, nsIWidget*) overload was folded into its sole caller.
Attachment #526914 - Flags: review?(roc)
(Assignee)

Comment 8

6 years ago
And that's it for this bug.
Attachment #526907 - Flags: review?(roc) → review+
Attachment #526908 - Flags: review?(roc) → review+
Comment on attachment 526909 [details] [diff] [review]
part 3: make nsThebesDeviceContext not an nsIObserver

+    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();

add "using namespace mozilla::services;" so we don't get this cruft inline.

+    if (obs)
+        obs->AddObserver(this, "memory-pressure", PR_FALSE);
+    if (obs)
+        obs->RemoveObserver(this, "memory-pressure");
+    if (!nsCRT::strcmp(aTopic, "memory-pressure"))
+        Compact();
+    if (!mFontCache)
+        CreateFontCache();

{}
Attachment #526909 - Flags: review?(roc) → review+
Attachment #526911 - Flags: review?(roc) → review+
Attachment #526913 - Flags: review?(roc) → review+
Attachment #526912 - Flags: review?(roc) → review+
Comment on attachment 526913 [details] [diff] [review]
part 6: prune unused/unimplemented methods, push some code down to nsFontCache

+    if (!aLanguage)
+        aLanguage = mLocaleLanguage;
+    if (mFontCache)
+        mFontCache->Flush();

{}
Attachment #526914 - Flags: review?(roc) → review+
Great stuff!
(Assignee)

Comment 12

6 years ago
(In reply to comment #9)
> 
> +    nsCOMPtr<nsIObserverService> obs =
> mozilla::services::GetObserverService();
> 
> add "using namespace mozilla::services;" so we don't get this cruft inline.

Will do.

> {}

Inconsistent with file style.
The file itself isn't consistent, but OK.
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/mozilla-central/rev/a06a2b7fda72
http://hg.mozilla.org/mozilla-central/rev/b4efed6c459c
http://hg.mozilla.org/mozilla-central/rev/a353b9086b47
http://hg.mozilla.org/mozilla-central/rev/3487552eaf91
http://hg.mozilla.org/mozilla-central/rev/3237cf9bc6f8
http://hg.mozilla.org/mozilla-central/rev/2afcb1f14fad
http://hg.mozilla.org/mozilla-central/rev/1764e405eb02
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 651498

Comment 15

6 years ago
This appears to have broken pref handling at startup (*sigh*).

Steps (on Windows):

1. Enter about:config
2. Set 'gfx.direct2d.disabled' to true
3. Restart the browser
4. Enter http://people.mozilla.org/~jdaggett/tests/decimalfontwaterfalls-default.html in the URL bar

Result: waterfall shows continuous size increments, indicative of DirectWrite rendering.

Expected result: step-effect with several lines rendering with the same size until the next full-pixel size up, indicative of GDI rendering.

Works: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:6.0a1) Gecko/20110419 Firefox/6.0a1

Doesn't work: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:6.0a1) Gecko/20110420 Firefox/6.0a1

I traced into the prefs code and it looks like it's not picking up a value but not reporting any error.

I have a feeling we're going to get into a "all prefs should be live" argument here.  My 1-second take on that is that these base level switches dictate some fairly big switches within gfx code, ones that affect hardware/driver checks and initialization and involve significant costs in startup time.  These also affect how text is measured and rendered, so having those prefs live means figuring out how to flush *all* text measurements.  Great flexibility but at a big cost in time and effort.  I think the real question is why is it important to make load order of components like gfx and the pref system be independent ("that's the way it's always work" doesn't count as a reason).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
See bug 651498.
John - you reopened in comment 15, but I don't think you backed out, right? If not, I'm going to call this fixed again, and the remaining work can go on in bug 651498.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 18

6 years ago
(In reply to comment #17)
> John - you reopened in comment 15, but I don't think you backed out, right? If
> not, I'm going to call this fixed again, and the remaining work can go on in
> bug 651498.

Yes, that's fine, sorry for the noise.
You need to log in before you can comment on or make changes to this bug.