Last Comment Bug 266236 - deCOMtaminate nsIRenderingContext
: deCOMtaminate nsIRenderingContext
Status: RESOLVED FIXED
: footprint, perf
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Zack Weinberg (:zwol)
:
Mentors:
Depends on: 571989 652361 653179
Blocks: deCOM 174055
  Show dependency treegraph
 
Reported: 2004-10-27 00:53 PDT by Simon Montagu :smontagu
Modified: 2014-04-26 03:10 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Preliminary patch to deCOMtaminate nsIRenderingContext (103.53 KB, patch)
2010-04-15 06:57 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
1. nsIRenderingContext deCOM (110.59 KB, patch)
2010-04-16 04:26 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
Merged nsRenderingContext (693.65 KB, patch)
2010-04-20 01:28 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
Merged nsRenderingContext (693.39 KB, patch)
2010-05-03 14:28 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
2. Patch containing the actual merge (177.75 KB, patch)
2010-05-04 08:09 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
3. Patch that contains the renaming (579.42 KB, patch)
2010-05-04 08:10 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
Change nsCOMPtr<...> to nsRefPtr<...> (18.96 KB, patch)
2010-05-12 12:54 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
4. Change nsCOMPtr<...> to nsRefPtr<...> (2. try) (19.69 KB, patch)
2010-05-12 13:40 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
1. nsIRenderingContext deCOM (merged up) (111.64 KB, patch)
2010-06-02 11:01 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
2. Patch containing the actual merge (merged up) (172.98 KB, patch)
2010-06-02 11:01 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
3. Patch that contains the renaming (merged up) (581.43 KB, patch)
2010-06-02 11:02 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
4. Change nsCOMPtr<...> to nsRefPtr<...> (merged up) (19.19 KB, patch)
2010-06-02 11:02 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
5. Remove unused nsViewManager::gCleanupContext (assertion fix) (2.92 KB, patch)
2010-06-03 04:08 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
5. Remove unused nsViewManager::gCleanupContext and any occurence of NS_?RENDERING_CONTEXT_?ID (4.88 KB, patch)
2010-06-03 09:24 PDT, Jan Küchler
no flags Details | Diff | Splinter Review
6. attempt to make non-libxul builds work (51.84 KB, patch)
2010-06-03 11:28 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
1/9: preliminary API cleanup (68.40 KB, patch)
2011-04-01 08:04 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
2/9: deCOM nsIRenderingContext and combine with sole implementation (116.32 KB, patch)
2011-04-01 08:07 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
3/9: mechanical update of all in-tree uses of ns*RenderingContext (578.38 KB, patch)
2011-04-01 08:12 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
4/9: aggressively prune unused methods and dead #ifdefs from nsRenderingContext (76.02 KB, patch)
2011-04-01 08:20 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
5/9: pointless nsresults (no out-params) (26.38 KB, patch)
2011-04-01 08:24 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
6/9: pointless nsresults (out-params) (42.33 KB, patch)
2011-04-01 08:29 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
7/9: simplify calling convention of SetClipRect and SetClipRegion (16.92 KB, patch)
2011-04-01 08:31 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
8/9: remove some unnecessary refcounting (37.12 KB, patch)
2011-04-01 08:35 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
9/9: move nsBoundingMetrics to nsIThebesFontMetrics.h, prune includes of nsRenderingContext.h (65.23 KB, patch)
2011-04-01 08:41 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
9/9: Move nsBoundingMetrics to its own header and prune inclusions of nsRenderingContext.h. (61.93 KB, patch)
2011-04-07 17:00 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
9/9: Move nsBoundingMetrics to its own header and prune inclusions of nsRenderingContext.h. (67.03 KB, patch)
2011-04-07 19:12 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review

Description Simon Montagu :smontagu 2004-10-27 00:53:57 PDT
Bug 139986 comment 22:
The right way to fix this kind of problem is to deCOMtaminate methods like
GetDeviceContext() so it just returns the device context as a raw pointer
directly, without adding a ref.

but see Bug 194385 comment 6:
deCOMtamination involves several things and some of them can be done
across library boundaries. But some changes require this merge [of gfx, widget
and layout]

roc, do you think the fix described in the first comment be done without the
library merging?
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-27 04:31:13 PDT
Yes it can.
Comment 2 Markus Hübner 2004-10-27 04:32:41 PDT
Who would be best to take this bug?
Comment 3 Cees T. 2007-04-18 02:21:56 PDT
People on bug 105431.
Comment 4 Cees T. 2007-04-18 02:33:36 PDT
This sounds like a duplicate of bug 186293.
Comment 5 Zack Weinberg (:zwol) 2010-02-08 18:37:24 PST
Gfx still has a few classes worth deCOMing and/or merging: I would think nsIDeviceContext and nsIRenderingContext would be the most worthwhile here.  Of course this is also tied up with conversion to direct use of the Thebes API, but that takes more human attention.
Comment 6 Jan Küchler 2010-04-14 09:48:58 PDT
(In reply to comment #5)
> Gfx still has a few classes worth deCOMing and/or merging: I would think
> nsIDeviceContext and nsIRenderingContext would be the most worthwhile here.

If nobody is on this at the moment, I'd like to take it.
Comment 7 Zack Weinberg (:zwol) 2010-04-14 09:58:47 PDT
Go for it.
Comment 8 Jan Küchler 2010-04-15 06:57:01 PDT
Created attachment 439226 [details] [diff] [review]
Preliminary patch to deCOMtaminate nsIRenderingContext

I added a first patch, that includes some changes to the nsIRenderingContext. This is not complete, but I thought I show what I have done so far, and ask if I'm on the right track.
Comment 9 Zack Weinberg (:zwol) 2010-04-15 09:15:21 PDT
I'm not a gfx peer, but I have some comments on your general approach:

In several places you made a change like this:

-NS_IMETHODIMP
-nsThebesRenderingContext::GetDeviceContext(nsIDeviceContext *& aDeviceContext)
-{
-    aDeviceContext = mDeviceContext;
-    NS_IF_ADDREF(aDeviceContext);
-    return NS_OK;
-}
+nsIDeviceContext*
+nsThebesRenderingContext::GetDeviceContext()
+{
+    //aDeviceContext = mDeviceContext;
+    //NS_IF_ADDREF(aDeviceContext);
+    //return NS_OK;
+    return mDeviceContext;
+}

This is wrong on two fronts: you shouldn't leave commented-out code in the tree, and you should not remove the addref here.  The correct rewrite of a function like this is:

already_AddRefed<nsIDeviceContext>
nsThebesRenderingContext::GetDeviceContext()
{
  NS_IF_ADDREF(mDeviceContext);
  return mDeviceContext;
}

(It *can* be the right thing to remove the addref from a function like this, if you can prove that all copies of the returned pointer die before the nsThebesRenderingContext itself does; that allows you to degrade the nsCOMPtrs in the callers to bare pointers, too.  But that needs careful auditing and should be done separately.)

As the only implementation of nsIRenderingContext in the tree is nsThebesRenderingContext, it would be nice to merge the classes (call the result nsRenderingContext) and devirtualize all the methods.  That should probably be done as a separate patch, though.

nsIRenderingContext appears to still have an IID and respond to QueryInterface after this patch.  That should go away too -- again, it's ok if that's a separate patch.
Comment 10 Jan Küchler 2010-04-15 09:27:29 PDT
(In reply to comment #9)
> I'm not a gfx peer, but I have some comments on your general approach:
> 
> In several places you made a change like this:
> 
> -NS_IMETHODIMP
> -nsThebesRenderingContext::GetDeviceContext(nsIDeviceContext *& aDeviceContext)
> -{
> -    aDeviceContext = mDeviceContext;
> -    NS_IF_ADDREF(aDeviceContext);
> -    return NS_OK;
> -}
> +nsIDeviceContext*
> +nsThebesRenderingContext::GetDeviceContext()
> +{
> +    //aDeviceContext = mDeviceContext;
> +    //NS_IF_ADDREF(aDeviceContext);
> +    //return NS_OK;
> +    return mDeviceContext;
> +}
> 
> This is wrong on two fronts: you shouldn't leave commented-out code in the
> tree, and you should not remove the addref here.  The correct rewrite of a
> function like this is:
> 
> already_AddRefed<nsIDeviceContext>
> nsThebesRenderingContext::GetDeviceContext()
> {
>   NS_IF_ADDREF(mDeviceContext);
>   return mDeviceContext;
> }
> 
> (It *can* be the right thing to remove the addref from a function like this, if
> you can prove that all copies of the returned pointer die before the
> nsThebesRenderingContext itself does; that allows you to degrade the nsCOMPtrs
> in the callers to bare pointers, too.  But that needs careful auditing and
> should be done separately.)

As all callers are using a nsCOMPtr<...> to hold the result, I thought that wasn't necessary. I'll change that.

> As the only implementation of nsIRenderingContext in the tree is
> nsThebesRenderingContext, it would be nice to merge the classes (call the
> result nsRenderingContext) and devirtualize all the methods.  That should
> probably be done as a separate patch, though.
> 
> nsIRenderingContext appears to still have an IID and respond to QueryInterface
> after this patch.  That should go away too -- again, it's ok if that's a
> separate patch.

I'll deCOM it first and merge them later. Should the merging/removal of the IID be a sperate bug, or shall I just post them here?

Thanks for the advice!
Comment 11 Bas Schouten (:bas.schouten) 2010-04-15 14:29:36 PDT
(In reply to comment #10)
> (In reply to comment #9)
> 
> As all callers are using a nsCOMPtr<...> to hold the result, I thought that
> wasn't necessary. I'll change that.

As this function is declared already_AddRefed for the return type the assignment operators will not add a reference to the object.
Comment 12 Jan Küchler 2010-04-16 04:26:33 PDT
Created attachment 439502 [details] [diff] [review]
1. nsIRenderingContext deCOM

Proposed patch to deCOM nsIRenderingContext.

Next things to do (I can do them) is removing the IID and merging it with the nsThebesRenderingContext (the only implementation of this interface) to nsRenderingContext.
Shall I base these changes on this patch, or should I create a patch that contains all of the changes (this patch + the new changes)?
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-04-16 04:39:19 PDT
It's better to break your changes up into one patch per change. That makes each patch smaller and easier to review. Each patch should leave the code in a working state.
Comment 14 Jan Küchler 2010-04-20 01:28:51 PDT
Created attachment 440166 [details] [diff] [review]
Merged nsRenderingContext

Proposed patch to merge the nsIRenderingContext with it's implementation. It depends on the deCOM patch, some calls to e.g. GetDeviceContext() are using the new format of that patch. This patch also gets rid of the IID.
Comment 15 Jan Küchler 2010-05-03 14:28:31 PDT
Created attachment 443189 [details] [diff] [review]
Merged nsRenderingContext

Removed some commented-out code. 
This patch should merge fine with any changes from the last days.
Comment 16 Vladimir Vukicevic [:vlad] [:vladv] 2010-05-04 07:30:22 PDT
Could you split up this patch into "places where nsIRenderingContext is renamed to nsRenderingContext" and everything else (places where code is actually merged/changed)?  It's extremely large and hard to review at the moment.  (But the general idea looks great!)
Comment 17 Jan Küchler 2010-05-04 08:09:53 PDT
Created attachment 443350 [details] [diff] [review]
2. Patch containing the actual merge

Here is the splitted version.

These are the actual merge changes, nsIRenderingContext.h and the nsThebesRenderingContext.h/cpp are removed and replaced by nsRenderingContext.h/cpp. 
It also contains the Makefile.in updates and the change to the nsThebesGfxFactory.

The next patch will contain the rename stuff. That was mostly a global search/replace. Just some files that included nsThebesRenderingContext.h directly needed some additional includes that are not in the new nsRenderingContext.h.
Comment 18 Jan Küchler 2010-05-04 08:10:33 PDT
Created attachment 443351 [details] [diff] [review]
3. Patch that contains the renaming
Comment 19 Vladimir Vukicevic [:vlad] [:vladv] 2010-05-12 10:03:37 PDT
Looks fine, but with one question -- this still uses nsCOMPtr<nsRenderingContext> to hold these objects.  I don't understand how that works, since as far as I can see there's no more IID for these objects.  I guess we only ever assign a real nsRenderingContext* to it, so the QI path never gets invoked (if it was, it would probably cause a compile error); but I believe these should all be changed to nsRefPtr.
Comment 20 Vladimir Vukicevic [:vlad] [:vladv] 2010-05-12 10:04:11 PDT
(Just doing an additional patch that changes the nsCOMPtr<nsRenderingContext> to nsRefPtr<nsRenderingContext> would be fine, no need to change the previous patches)
Comment 21 Vladimir Vukicevic [:vlad] [:vladv] 2010-05-12 10:17:25 PDT
+      // initialize some common services, so we don't pay the cost for these at odd times later on;
+      // SetWindowCreator -> ChromeRegistry -> IOService -> SocketTransportService -> (nspr wspm init), Prefs
+      {
+        nsCOMPtr<nsISupports> comp;

in XRE_main, stick this inside #ifdef NS_FUNCTION_TIMER so that we don't change the normal startup path.


in mozJSSubScriptLoader::LoadSubScript (and maybe elsewhere?):

+#ifdef NS_FUNCTION_TIMER
+    NS_ConvertUTF16toUTF8 url__(aURL);
+    NS_TIME_FUNCTION_FMT("%s (line %d) (url: %s)", MOZ_FUNCTION_NAME,
+                         __LINE__, url__.get());

don't declare a stack NS_ConvertUTF16toUTF8 -- use NS_ConvertUTF16toUTF8(aURL).get() in the printf args directly.  Better yet, use NS_LossyConvertUTF16toASCII instead, no need to pay the cost of a UTF8 conversion.

in PresShell, would be much cleaner to have a macro for

+  nsCAutoString docURL__("N/A");
+  nsIURI *uri__ = mDocument->GetDocumentURI();
+  if (uri__) {
+    uri__->GetSpec(docURL__);
+  }

since it's repeated everywhere and looks really ugly; it'll likely bitrot unless it looks clean/easy enough to fix for someone making a change.

Also, everywhere, don't use braces around:

+  if (aLoadData->mURI) {
+    aLoadData->mURI->GetSpec(spec__);
+  }

The single-line if doesn't need it, and we're trying to minimize code impact on existing code here (visual/scanning impact).  I'd even write it on just one line:

if (aLoadData->mURI) aLoadData->mURI->GetSpec(spec__);

I couldn't come up with a good way to declare and initialize in one go; stupid getters :/

+  NS_TIME_FUNCTION_MIN_FMT(5, "%s (ARGH line %d) (cid: %s)", MOZ_FUNCTION_NAME, \
+                           __LINE__, cid_buf__)

get rid of the ARGHs?

Please pull the nsStackWalk/nsThread/nsTimer instrumentation into a separate patch; it's trickier than just the boilerplate stuff elsewhere.
Comment 22 Jan Küchler 2010-05-12 12:54:39 PDT
Created attachment 444955 [details] [diff] [review]
Change nsCOMPtr<...> to nsRefPtr<...>

Alright, this patch changes all nsCOMPtr<nsRenderingContext> to nsRefPtr<...>s.

Your last comment was meant for an other bug, right?
Comment 23 Vladimir Vukicevic [:vlad] [:vladv] 2010-05-12 13:06:46 PDT
oh wow, yes, other bug
Comment 24 Jan Küchler 2010-05-12 13:40:12 PDT
Created attachment 444966 [details] [diff] [review]
4. Change nsCOMPtr<...> to nsRefPtr<...> (2. try)

Missed one file.
Comment 25 Vladimir Vukicevic [:vlad] [:vladv] 2010-05-18 15:53:43 PDT
Comment on attachment 444966 [details] [diff] [review]
4. Change nsCOMPtr<...> to nsRefPtr<...> (2. try)

could've sworn I already r+'d this, sorry!
Comment 26 Jan Küchler 2010-05-31 09:46:07 PDT
Could somebody please commit the patches? (I don't have commit access)
Comment 27 Zack Weinberg (:zwol) 2010-06-01 09:30:26 PDT
Comment on attachment 443189 [details] [diff] [review]
Merged nsRenderingContext

The "Merged nsRenderingContext" patch appears to have been superseded by the next two patches.
Comment 28 Zack Weinberg (:zwol) 2010-06-01 09:31:25 PDT
Comment on attachment 439502 [details] [diff] [review]
1. nsIRenderingContext deCOM

I was going to land this stuff, but it looks like the actual deCOM patch (on which everything else depends, no?) was never reviewed by a gfx peer.
Comment 29 Zack Weinberg (:zwol) 2010-06-01 09:34:12 PDT
Clearing checkin-needed for now.  Jan, please confirm that the patches need to be applied in this order:

"nsIRenderingContext deCOM"
"Patch containing the actual merge"
"Patch that contains the renaming"
"Change nsCOMPtr<...> to nsRefPtr<...> (2. try)"

For future reference, it is good to put sequence numbers on the patches in any bug that might need more than one patch, so that the order of application is clear.
Comment 30 Jan Küchler 2010-06-01 09:43:42 PDT
Yes, that's the right order. Sorry for the mess...

I'll add the checkin-needed keyword when the first patch is reviewed.
Comment 31 Jan Küchler 2010-06-01 09:45:37 PDT
Comment on attachment 444966 [details] [diff] [review]
4. Change nsCOMPtr<...> to nsRefPtr<...> (2. try)

Added sequence numbers to the patches.
Comment 32 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-01 11:16:35 PDT
r=me on the first patch, but please don't mark it or otherwise change it -- its edit page is showing me a very weird layout bug, so would like to preserve it for a bit :)
Comment 33 Zack Weinberg (:zwol) 2010-06-01 23:50:48 PDT
FYI, all four patches required merging, and the result does not compile due to a mixture of trivial and not-so-trivial problems.  I have fixed the simple ones, leaving me with this that I'm not sure is correct:

  // mDeviceContext is a nsCOMPtr
  already_AddRefed<nsIDeviceContext>
  nsRenderingContext::GetDeviceContext()
  {
    NS_IF_ADDREF(mDeviceContext);
    return mDeviceContext.get();
  }

and this error that I don't know how to fix (non-libxul debug build):

nsNativeThemeGTK.o: In function `nsNativeThemeGTK::GetMinimumWidgetSize(nsRenderingContext*, nsIFrame*, unsigned char, nsIntSize*, int*)':
/home/zack/src/mozilla/moz-central/widget/src/gtk2/nsNativeThemeGTK.cpp:1097: undefined reference to `nsRenderingContext::GetDeviceContext()'
/usr/bin/ld: libwidget_gtk2.so: hidden symbol `nsRenderingContext::GetDeviceContext()' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output

Pushed to try server anyway, but I for one will not land something that breaks non-libxul builds. ;-)
Comment 34 Zack Weinberg (:zwol) 2010-06-02 09:25:10 PDT
Total build failure on try server due to a typo in one place and a missed conversion in OSX-specific code.  Fixed, repushed.  Still need help sorting out the non-libxul build.
Comment 35 Jan Küchler 2010-06-02 10:19:29 PDT
I think I found the reason for the non-libxul build break:

While linking a "fake.lib" (in widgets/) the nsRenderingContext object file is missing. With the previous nsIRenderingContext that would be no problem, as all the methods where pure virtual and linked at runtime.
The object file is in gfx/src/thebes/gkgfxthebes.dll.

I think linking to this DLL would fix the error, but I have two problems:
1. No .lib file is created for the missing DLL, and I don't know how what to change in the Makefile.in to get it created and
2. I don't know the right Makefile.in for the fake.lib to add the library (I think it's widget/src/build/Makefile.in).

Would that be the right approach and is there a "HowTo" for the buildsystem to create the missing .lib?
Comment 36 Zack Weinberg (:zwol) 2010-06-02 11:01:30 PDT
Created attachment 448789 [details] [diff] [review]
1. nsIRenderingContext deCOM (merged up)

Here are the patches merged to latest trunk and with most of the compile errors fixed (the non-libxul link errors remain).  Tests on the try server melted down quite thoroughly - that needs investigation.
Comment 37 Zack Weinberg (:zwol) 2010-06-02 11:01:57 PDT
Created attachment 448790 [details] [diff] [review]
2. Patch containing the actual merge (merged up)
Comment 38 Zack Weinberg (:zwol) 2010-06-02 11:02:27 PDT
Created attachment 448791 [details] [diff] [review]
3. Patch that contains the renaming (merged up)
Comment 39 Zack Weinberg (:zwol) 2010-06-02 11:02:54 PDT
Created attachment 448792 [details] [diff] [review]
4. Change nsCOMPtr<...> to nsRefPtr<...> (merged up)
Comment 40 Zack Weinberg (:zwol) 2010-06-02 13:20:31 PDT
Here are links to all of the failed test logs from the try server:

linux Bd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275494940.1275499140.10899.gz
linux Cd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275499111.1275499850.13689.gz
linux Ro: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275499681.1275499860.13697.gz
linux Rd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275499112.1275499746.13179.gz
linux Jd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275499112.1275501525.20303.gz

osx Bd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275494940.1275502213.23510.gz
osx Mdoth: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275501989.1275502872.26275.gz
osx Cd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275501990.1275502633.25055.gz
osx Ro: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275507392.1275507547.14856.gz
osx Rd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275501990.1275502310.23726.gz
osx Jd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275502340.1275504427.851.gz

windows Bd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275494940.1275500345.15711.gz
windows Md5: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275500247.1275500730.17232.gz
windows Cd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275500246.1275500821.17545.gz
windows Ro: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275506794.1275506914.11841.gz
windows Rd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275500246.1275500522.16415.gz
windows Jd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275500246.1275502407.23975.gz

Some of these may be unrelated intermittent oranges; I didn't look at them very closely.  However, a lot of them seem to come down to this new assertion:

ASSERTION: Wasn't able to create a graphics context for cleanup: 'gCleanupContext', file view/src/nsViewManager.cpp, line 160
Comment 41 Jan Küchler 2010-06-03 04:08:26 PDT
Created attachment 448984 [details] [diff] [review]
5. Remove unused nsViewManager::gCleanupContext (assertion fix)

It seems that the gCleanupContext is never actually used. It gets created in the constructor (that's where the assertion fires) and released in the destructor, but it's not referenced anywhere else.
This patch removes it.

I get one other assertion, but I think it is unrelated to the nsRenderingContext:
ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnonymousSubtree()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file g:/Programmieren/src/mozilla-central/content/base/src/nsContentUtils.cpp, line 3537
Comment 42 Zack Weinberg (:zwol) 2010-06-03 08:44:35 PDT
> -    CallCreateInstance(kRenderingContextCID, &gCleanupContext);

You should search the tree for any other uses of kRenderingContextCID, NS_RENDERING_CONTEXT_CID, or NS_IRENDERING_CONTEXT_IID, and zap them as well.

http://mxr.mozilla.org/mozilla-central/search?string=kRenderingContextCID
http://mxr.mozilla.org/mozilla-central/search?string=NS_RENDERING_CONTEXT_CID
http://mxr.mozilla.org/mozilla-central/search?string=NS_IRENDERING_CONTEXT_IID
Comment 43 Jan Küchler 2010-06-03 09:24:51 PDT
Created attachment 449031 [details] [diff] [review]
5. Remove unused nsViewManager::gCleanupContext and any occurence of NS_?RENDERING_CONTEXT_?ID

Added that to the previous patch, the ID was used nowhere else.
Comment 44 Zack Weinberg (:zwol) 2010-06-03 11:28:36 PDT
Created attachment 449051 [details] [diff] [review]
6. attempt to make non-libxul builds work

This is an attempt to make non-libxul builds work.  Fundamentally, the changes required are (1) make GetDeviceContext an inline method, which makes the various uses in widget/ happy; (2) allow gklayout to refer directly to nsRenderingContext.

I was able to do (1) no problem, but I got stuck on (2).  It *almost* suffices to make libgkgfxthebes not a component and then explicitly link libgklayout against it -- the *build* succeeds, but the browser crashes on startup because libgkgfxthebes contains XPCOM components, still, and they don't get registered, so the docshell can't create device contexts, kaboom.  I then tried moving nsRenderingContext into libgkgfx, which seems like it would work (modulo having to mess with the PR_LOG calls) but then libgkgfx fails to link with a pile of undefined references to raw gfxContext APIs, and that's where I gave up again.

There are also some independently-desirable cleanups in here, notably nsRenderingContext no longer inherits from nsISupports and has no virtual methods at all.

Unrelated observation: patch 3 moves some files but the moves are not recorded properly.
Comment 45 Zack Weinberg (:zwol) 2010-06-03 11:31:42 PDT
I'm starting to wonder if we shouldn't first collapse gfx/src/thebes (and maybe also gfx/thebes) into gfx/src before doing this deCOM.  Then there would be only one libgkgfx.so, it would not be in components/, but it would nonetheless register components (this has to be possible somehow) and we wouldn't have these linkage problems.  This would also pave the way to deCOMing nsIDeviceContext, which seems like the next logical step.
Comment 46 Zack Weinberg (:zwol) 2010-06-03 11:32:19 PDT
Changed bug title to make the target here clearer.
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-06-03 14:24:49 PDT
Sounds reasonable
Comment 48 Zack Weinberg (:zwol) 2011-04-01 08:04:06 PDT
Created attachment 523585 [details] [diff] [review]
1/9: preliminary API cleanup

Now that bug 571989 is finally done, here is a new patch series for this bug.  Patches 1-3 are still largely Jan's work; patches 4-9 are further cleanups on top of that.  There was a whole lot of unused junk in this interface, which I have now *mostly* gotten rid of (it's not possible to get rid of all of it without starting in on nsIFontMetrics and nsIDeviceContext - I might do that, but not in this bug).

This patch is entirely Jan's; it simply rearranges the calling convention of various ns(I)RenderingContext APIs to be more convenient for their users.
Comment 49 Zack Weinberg (:zwol) 2011-04-01 08:07:22 PDT
Created attachment 523587 [details] [diff] [review]
2/9: deCOM nsIRenderingContext and combine with sole implementation

This patch merges nsIRenderingContext and nsThebesRenderingContext, devirtualizes all the methods, and strips out all the nsISupports boilerplate.  For ease of review, this does NOT include the large set of mechanical changes required to make things compile again; that's the next patch.
Comment 50 Zack Weinberg (:zwol) 2011-04-01 08:12:44 PDT
Created attachment 523588 [details] [diff] [review]
3/9: mechanical update of all in-tree uses of ns*RenderingContext

This patch  fixes up all uses of nsIRenderingContext and nsThebesRenderingContext in the tree; it is the result of these automated search-and-replace commands:

$ perl -pi~ -e 's/\bns(?:I|Thebes)RenderingContext\b/nsRenderingContext/g' \
    $(grep -rEl '\<ns(I|Thebes)RenderingContext\>' *)
$ perl -pi~ -e 's/nsCOMPtr<nsRenderingContext>/nsRefPtr<nsRenderingContext>/g' \
    $(grep -rEl '\<nsCOMPtr<nsRenderingContext>' *)

It bitrots very fast; if this series is delayed in landing for more than a week or so, it will be easier to regenerate it from scratch with the above commands than to try to merge.
Comment 51 Zack Weinberg (:zwol) 2011-04-01 08:20:05 PDT
Created attachment 523590 [details] [diff] [review]
4/9: aggressively prune unused methods and dead #ifdefs from nsRenderingContext

There were a large number of nsRenderingContext methods that were totally unused, or only used in one place that could easily be changed to do something else.  There were also a few #ifdefs that were never defined under any circumstances.  This patch removes all that junk.  I also removed the PR_LOG calls from all the methods, on the theory that those were largely useful for bring-up of Thebes and are now just wasting space.

Most of the shape-drawing methods exist in two overloads: one that takes  nscoords, and one that takes points or rects.  I kept both of those even if only one was used, for consistency's sake.
Comment 52 Zack Weinberg (:zwol) 2011-04-01 08:24:39 PDT
Created attachment 523593 [details] [diff] [review]
5/9: pointless nsresults (no out-params)

The majority of nsRenderingContext methods have no out-parameters and always return NS_OK.  This makes them return nothing.  There were hardly any places that were bothering to check.

Some of the text methods wind up forwarding to nsIFontMetrics methods. I spot-checked those and it *appears* that they only fail if operator new fails, which it doesn't anymore.
Comment 53 Zack Weinberg (:zwol) 2011-04-01 08:29:07 PDT
Created attachment 523595 [details] [diff] [review]
6/9: pointless nsresults (out-params)

The remainder of the nsRenderingContext methods had one out-parameter which could easily be rotated to the return value; that's this patch.

This is also the point where I noticed that nsThebesFontMetrics doesn't implement the 'aFontID' or 'aSpacing' arguments to any of its methods, and furthermore that *no callers anywhere in the tree* passed non-default arguments for aFontID or aSpacing in calls to nsRenderingContext, so I stripped them.  Arguably some of that should have been done in the previous patch, but it didn't seem worth going back to it.

Several places did care whether GetBoundingMetrics failed.  Again, it appears to me that this can only happen if memory allocation fails, so infallible malloc has that covered.
Comment 54 Zack Weinberg (:zwol) 2011-04-01 08:31:58 PDT
Created attachment 523596 [details] [diff] [review]
7/9: simplify calling convention of SetClipRect and SetClipRegion

nsRenderingContext::SetClipRegion throws an assertion if its second argument isn't nsClipCombine_kReplace.  nsRenderingContext::SetClipRect works if its second argument is nsClipCombine_kReplace or _kIntersect, but all its callers use _kIntersect.  Therefore, I delete the second argument from both methods and rename them SetClip and IntersectClip respectively.  The nsClipCombine enumeration is then unused and can also be deleted.
Comment 55 Zack Weinberg (:zwol) 2011-04-01 08:35:31 PDT
Created attachment 523598 [details] [diff] [review]
8/9: remove some unnecessary refcounting

All users of ->GetFontMetrics and ->GetDeviceContext keep the rendering context around for longer than the nsCOMPtr that they assign the result to. Replace them with inline methods that return bare pointers.

Most users of ->GetDeviceContext only want it to extract the AppUnitsPerDevPixel scale factor, so provide a convenience method for that case.
Comment 56 Zack Weinberg (:zwol) 2011-04-01 08:41:50 PDT
Created attachment 523599 [details] [diff] [review]
9/9: move nsBoundingMetrics to nsIThebesFontMetrics.h, prune includes of nsRenderingContext.h

Several places (most prominently nsHTMLReflowMetrics.h) include nsRenderingContext.h only to get the definition of nsBoundingMetrics.  Move that to nsIThebesFontMetrics.h, which is much more appropriate.  In another place, nsDisplayList.h has an inline method that uses AutoPushTranslation *without* including nsRenderingContext.h; push that down to the method that actually does the drawing.  Semi-mechanically prune all unnecessary inclusions of nsRenderingContext.h.  Also minimize inclusions *by* nsRenderingContext.h and some of the *FontMetrics headers.

I wish I had thought of doing this much earlier in the patch series, it would have saved me a whole bunch of recompile wait time.

I don't *think* I need to bump the nsIPresShell IID for the change I made to a hidden, conditionally-present interface there; please correct me if I'm wrong.
Comment 57 Zack Weinberg (:zwol) 2011-04-07 17:00:55 PDT
Created attachment 524538 [details] [diff] [review]
9/9: Move nsBoundingMetrics to its own header and prune inclusions of nsRenderingContext.h.

Slight revision of part 9: having moved on to decomming nsI*FontMetrics, it became obvious that nsBoundingMetrics really wanted to be in its *own* header, if only so that changes to ns*FontMetrics.h don't rebuild all of layout thanks to the use of nsBoundingMetrics in nsHTMLReflowMetrics.h.
Comment 58 Zack Weinberg (:zwol) 2011-04-07 19:12:46 PDT
Created attachment 524556 [details] [diff] [review]
9/9: Move nsBoundingMetrics to its own header and prune inclusions of nsRenderingContext.h.

Brown paper bag time: in the previous iteration of this patch I forgot to 'hg add' the new file.
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-13 15:01:22 PDT
Comment on attachment 523593 [details] [diff] [review]
5/9: pointless nsresults (no out-params)

+        } else
+            rv = NS_ERROR_FAILURE;

Needs curlies:
 } else {
     rv = NS_ERROR_FAILURE;
 }

(2 occurrences)
Comment 60 Zack Weinberg (:zwol) 2011-04-13 16:48:29 PDT
(In reply to comment #59)
> flag: review?(roc@ocallahan.org) => review+Comment on attachment 523593 [details] [diff] [review]
> 5/9: pointless nsresults (no out-params)
> 
> +        } else
> +            rv = NS_ERROR_FAILURE;
> 
> Needs curlies:
>  } else {
>      rv = NS_ERROR_FAILURE;
>  }
> 
> (2 occurrences)

This file doesn't seem to bother with curlies around single statement dependent clauses, but ok, changed.

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