Last Comment Bug 651017 - DeCOM nsIDeviceContext
: DeCOM nsIDeviceContext
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Zack Weinberg (:zwol)
:
Mentors:
Depends on: 651498
Blocks: 651016
  Show dependency treegraph
 
Reported: 2011-04-18 19:58 PDT by Zack Weinberg (:zwol)
Modified: 2011-05-08 07:40 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: include minimization (56.15 KB, patch)
2011-04-18 20:27 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 2: better initialization for the gfx module (7.88 KB, patch)
2011-04-18 20:29 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 3: make nsThebesDeviceContext not an nsIObserver (12.46 KB, patch)
2011-04-18 20:32 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 4: deCOM and tidy (90.01 KB, patch)
2011-04-18 20:35 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 5: mechanical fixups (101.26 KB, patch)
2011-04-18 20:38 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 6: prune unused/unimplemented methods, push some code down to nsFontCache (19.67 KB, patch)
2011-04-18 20:40 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 7: simplify rendering context creation (13.64 KB, patch)
2011-04-18 20:42 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review

Description Zack Weinberg (:zwol) 2011-04-18 19:58:13 PDT
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. :)
Comment 1 Zack Weinberg (:zwol) 2011-04-18 20:27:16 PDT
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.
Comment 2 Zack Weinberg (:zwol) 2011-04-18 20:29:55 PDT
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.
Comment 3 Zack Weinberg (:zwol) 2011-04-18 20:32:35 PDT
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.
Comment 4 Zack Weinberg (:zwol) 2011-04-18 20:35:53 PDT
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.
Comment 5 Zack Weinberg (:zwol) 2011-04-18 20:38:00 PDT
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>' *)
Comment 6 Zack Weinberg (:zwol) 2011-04-18 20:40:37 PDT
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.
Comment 7 Zack Weinberg (:zwol) 2011-04-18 20:42:40 PDT
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.
Comment 8 Zack Weinberg (:zwol) 2011-04-18 20:43:32 PDT
And that's it for this bug.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-18 21:37:24 PDT
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();

{}
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-18 21:44:35 PDT
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();

{}
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-18 21:46:59 PDT
Great stuff!
Comment 12 Zack Weinberg (:zwol) 2011-04-18 21:50:55 PDT
(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.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-18 22:23:03 PDT
The file itself isn't consistent, but OK.
Comment 15 John Daggett (:jtd) 2011-04-25 23:14:48 PDT
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).
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-25 23:21:16 PDT
See bug 651498.
Comment 17 Joe Drew (not getting mail) 2011-04-26 08:16:51 PDT
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.
Comment 18 John Daggett (:jtd) 2011-04-26 16:30:34 PDT
(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.

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