Closed Bug 266236 Opened 20 years ago Closed 13 years ago

deCOMtaminate nsIRenderingContext

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smontagu, Assigned: zwol)

References

(Blocks 1 open bug)

Details

(Keywords: memory-footprint, perf)

Attachments

(9 files, 17 obsolete files)

68.40 KB, patch
roc
: review+
Details | Diff | Splinter Review
116.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
578.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
76.02 KB, patch
roc
: review+
Details | Diff | Splinter Review
26.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
42.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
16.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
37.12 KB, patch
roc
: review+
Details | Diff | Splinter Review
67.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
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?
Who would be best to take this bug?
Assignee: general → Kelly_G_Walker
Blocks: 139986
People on bug 105431.
This sounds like a duplicate of bug 186293.
Assignee: Kelly_G_Walker → nobody
QA Contact: ian → general
Product: Core → Core Graveyard
Blocks: deCOM
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.
No longer blocks: 139986
Component: GFX → Graphics
Product: Core Graveyard → Core
QA Contact: general → thebes
(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.
Go for it.
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.
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.
(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!
(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.
Attached patch 1. nsIRenderingContext deCOM (obsolete) — Splinter Review
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)?
Attachment #439226 - Attachment is obsolete: true
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.
Attached patch Merged nsRenderingContext (obsolete) — Splinter Review
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.
Attached patch Merged nsRenderingContext (obsolete) — Splinter Review
Removed some commented-out code. 
This patch should merge fine with any changes from the last days.
Attachment #440166 - Attachment is obsolete: true
Attachment #443189 - Flags: review?(vladimir)
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!)
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.
Attachment #443350 - Flags: review?(vladimir)
Attachment #443351 - Flags: review?(vladimir)
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.
(Just doing an additional patch that changes the nsCOMPtr<nsRenderingContext> to nsRefPtr<nsRenderingContext> would be fine, no need to change the previous patches)
+      // 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.
Alright, this patch changes all nsCOMPtr<nsRenderingContext> to nsRefPtr<...>s.

Your last comment was meant for an other bug, right?
Attachment #444955 - Flags: review?(vladimir)
Missed one file.
Attachment #444955 - Attachment is obsolete: true
Attachment #444966 - Flags: review?(vladimir)
Comment on attachment 444966 [details] [diff] [review]
4. Change nsCOMPtr<...> to nsRefPtr<...> (2. try)

could've sworn I already r+'d this, sorry!
Attachment #444966 - Flags: review?(vladimir) → review+
Could somebody please commit the patches? (I don't have commit access)
Keywords: checkin-needed
Assignee: nobody → jan
Comment on attachment 443189 [details] [diff] [review]
Merged nsRenderingContext

The "Merged nsRenderingContext" patch appears to have been superseded by the next two patches.
Attachment #443189 - Attachment is obsolete: true
Attachment #439502 - Flags: review?(vladimir)
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.
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.
Keywords: checkin-needed
Yes, that's the right order. Sorry for the mess...

I'll add the checkin-needed keyword when the first patch is reviewed.
Attachment #439502 - Attachment description: nsIRenderingContext deCOM → 1. nsIRenderingContext deCOM
Attachment #443350 - Attachment description: Patch containing the actual merge → 2. Patch containing the actual merge
Attachment #443351 - Attachment description: Patch that contains the renaming → 3. Patch that contains the renaming
Comment on attachment 444966 [details] [diff] [review]
4. Change nsCOMPtr<...> to nsRefPtr<...> (2. try)

Added sequence numbers to the patches.
Attachment #444966 - Attachment description: Change nsCOMPtr<...> to nsRefPtr<...> (2. try) → 4. Change nsCOMPtr<...> to nsRefPtr<...> (2. try)
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 :)
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. ;-)
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.
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?
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.
Attachment #439502 - Attachment is obsolete: true
Attachment #439502 - Flags: review?(vladimir)
Attachment #443350 - Attachment is obsolete: true
Attachment #443351 - Attachment is obsolete: true
Attachment #444966 - Attachment is obsolete: true
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
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
Attachment #448984 - Flags: review?(vladimir)
> -    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
Attachment #448984 - Flags: review?(vladimir)
Added that to the previous patch, the ID was used nowhere else.
Attachment #449031 - Flags: review?(vladimir)
Attachment #448984 - Attachment is obsolete: true
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.
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.
Changed bug title to make the target here clearer.
Summary: deCOMtaminate some GFX methods → deCOMtaminate nsIRenderingContext
Depends on: 571989
Attachment #443189 - Flags: review+
Attachment #443350 - Flags: review+
Attachment #443351 - Flags: review+
Attachment #444955 - Flags: review+
Attachment #444966 - Flags: review+
Attachment #449031 - Flags: review+
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.
Assignee: jan → zackw
Attachment #448789 - Attachment is obsolete: true
Attachment #448790 - Attachment is obsolete: true
Attachment #448791 - Attachment is obsolete: true
Attachment #448792 - Attachment is obsolete: true
Attachment #449031 - Attachment is obsolete: true
Attachment #449051 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #523585 - Flags: review?(roc)
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.
Attachment #523587 - Flags: review?(roc)
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.
Attachment #523588 - Flags: review?(roc)
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.
Attachment #523590 - Flags: review?(roc)
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.
Attachment #523593 - Flags: review?(roc)
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.
Attachment #523595 - Flags: review?(roc)
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.
Attachment #523596 - Flags: review?(roc)
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.
Attachment #523598 - Flags: review?(roc)
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.
Attachment #523599 - Attachment description: move nsBoundingMetrics to nsIThebesFontMetrics.h, prune includes of nsRenderingContext.h → 9/9: move nsBoundingMetrics to nsIThebesFontMetrics.h, prune includes of nsRenderingContext.h
Attachment #523599 - Flags: review?(roc)
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.
Attachment #523599 - Attachment is obsolete: true
Attachment #524538 - Flags: review?(roc)
Attachment #523599 - Flags: review?(roc)
Brown paper bag time: in the previous iteration of this patch I forgot to 'hg add' the new file.
Attachment #524538 - Attachment is obsolete: true
Attachment #524556 - Flags: review?(roc)
Attachment #524538 - Flags: review?(roc)
Blocks: 174055
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)
Attachment #523593 - Flags: review?(roc) → review+
(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.
Depends on: 652361
Depends on: 653179
You need to log in before you can comment on or make changes to this bug.