deCOMtaminate nsIRenderingContext

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: smontagu, Assigned: zwol)

Tracking

(Blocks: 1 bug, {footprint, perf})

Trunk
footprint, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 17 obsolete attachments)

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
(Reporter)

Description

13 years ago
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?
Yes it can.

Comment 2

13 years ago
Who would be best to take this bug?
(Reporter)

Updated

13 years ago
Assignee: general → Kelly_G_Walker

Updated

12 years ago
Blocks: 139986

Comment 3

10 years ago
People on bug 105431.

Comment 4

10 years ago
This sounds like a duplicate of bug 186293.

Updated

10 years ago
Assignee: Kelly_G_Walker → nobody
QA Contact: ian → general
Product: Core → Core Graveyard
(Assignee)

Updated

7 years ago
Blocks: 105431
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

7 years ago
Go for it.

Comment 8

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

Comment 9

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

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

Comment 12

7 years ago
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)?
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.

Comment 14

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

7 years ago
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.
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!)

Comment 17

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

7 years ago
Created attachment 443351 [details] [diff] [review]
3. Patch that contains the renaming

Updated

7 years ago
Attachment #443350 - Flags: review?(vladimir)

Updated

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

Comment 22

7 years ago
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?
Attachment #444955 - Flags: review?(vladimir)
oh wow, yes, other bug
Attachment #443189 - Flags: review?(vladimir) → review+
Attachment #443350 - Flags: review?(vladimir) → review+
Attachment #443351 - Flags: review?(vladimir) → review+
Attachment #444955 - Flags: review?(vladimir) → review+

Comment 24

7 years ago
Created attachment 444966 [details] [diff] [review]
4. Change nsCOMPtr<...> to nsRefPtr<...> (2. try)

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+

Comment 26

7 years ago
Could somebody please commit the patches? (I don't have commit access)

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Assignee: nobody → jan
(Assignee)

Comment 27

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

Updated

7 years ago
Attachment #439502 - Flags: review?(vladimir)
(Assignee)

Comment 28

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

Comment 29

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

Comment 30

7 years ago
Yes, that's the right order. Sorry for the mess...

I'll add the checkin-needed keyword when the first patch is reviewed.

Updated

7 years ago
Attachment #439502 - Attachment description: nsIRenderingContext deCOM → 1. nsIRenderingContext deCOM

Updated

7 years ago
Attachment #443350 - Attachment description: Patch containing the actual merge → 2. Patch containing the actual merge

Updated

7 years ago
Attachment #443351 - Attachment description: Patch that contains the renaming → 3. Patch that contains the renaming

Comment 31

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

Comment 33

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

Comment 34

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

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

Comment 36

7 years ago
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.
Attachment #439502 - Attachment is obsolete: true
Attachment #439502 - Flags: review?(vladimir)
(Assignee)

Comment 37

7 years ago
Created attachment 448790 [details] [diff] [review]
2. Patch containing the actual merge (merged up)
Attachment #443350 - Attachment is obsolete: true
(Assignee)

Comment 38

7 years ago
Created attachment 448791 [details] [diff] [review]
3. Patch that contains the renaming (merged up)
Attachment #443351 - Attachment is obsolete: true
(Assignee)

Comment 39

7 years ago
Created attachment 448792 [details] [diff] [review]
4. Change nsCOMPtr<...> to nsRefPtr<...> (merged up)
(Assignee)

Updated

7 years ago
Attachment #444966 - Attachment is obsolete: true
(Assignee)

Comment 40

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

7 years ago
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
Attachment #448984 - Flags: review?(vladimir)
(Assignee)

Comment 42

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

Updated

7 years ago
Attachment #448984 - Flags: review?(vladimir)

Comment 43

7 years ago
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.
Attachment #449031 - Flags: review?(vladimir)
Attachment #449031 - Flags: review?(vladimir) → review+
(Assignee)

Updated

7 years ago
Attachment #448984 - Attachment is obsolete: true
(Assignee)

Comment 44

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

Comment 45

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

Comment 46

7 years ago
Changed bug title to make the target here clearer.
Summary: deCOMtaminate some GFX methods → deCOMtaminate nsIRenderingContext
Sounds reasonable
(Assignee)

Updated

7 years ago
Depends on: 571989
(Assignee)

Updated

6 years ago
Attachment #443189 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #443350 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #443351 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #444955 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #444966 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #449031 - Flags: review+
(Assignee)

Comment 48

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

Comment 49

6 years ago
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.
Attachment #523587 - Flags: review?(roc)
(Assignee)

Comment 50

6 years ago
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.
Attachment #523588 - Flags: review?(roc)
(Assignee)

Comment 51

6 years ago
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.
Attachment #523590 - Flags: review?(roc)
(Assignee)

Comment 52

6 years ago
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.
Attachment #523593 - Flags: review?(roc)
(Assignee)

Comment 53

6 years ago
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.
Attachment #523595 - Flags: review?(roc)
(Assignee)

Comment 54

6 years ago
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.
Attachment #523596 - Flags: review?(roc)
(Assignee)

Comment 55

6 years ago
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.
Attachment #523598 - Flags: review?(roc)
(Assignee)

Comment 56

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

Updated

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

Comment 57

6 years ago
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.
Attachment #523599 - Attachment is obsolete: true
Attachment #524538 - Flags: review?(roc)
Attachment #523599 - Flags: review?(roc)
(Assignee)

Comment 58

6 years ago
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.
Attachment #524538 - Attachment is obsolete: true
Attachment #524556 - Flags: review?(roc)
Attachment #524538 - Flags: review?(roc)
Attachment #523585 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Blocks: 174055
Attachment #523587 - Flags: review?(roc) → review+
Attachment #523588 - Flags: review?(roc) → review+
Attachment #523590 - Flags: review?(roc) → 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)
Attachment #523593 - Flags: review?(roc) → review+
Attachment #523595 - Flags: review?(roc) → review+
Attachment #523596 - Flags: review?(roc) → review+
(Assignee)

Comment 60

6 years ago
(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.
Attachment #523598 - Flags: review?(roc) → review+
Attachment #524556 - Flags: review?(roc) → review+
(Assignee)

Comment 61

6 years ago
http://hg.mozilla.org/mozilla-central/rev/faeb9fecfc94
http://hg.mozilla.org/mozilla-central/rev/1c1bfa98f600
http://hg.mozilla.org/mozilla-central/rev/f54747d3a908
http://hg.mozilla.org/mozilla-central/rev/2723b4329877
http://hg.mozilla.org/mozilla-central/rev/793e5d7b26c5
http://hg.mozilla.org/mozilla-central/rev/cf8bfa6a4073
http://hg.mozilla.org/mozilla-central/rev/5fb27ef09b9b
http://hg.mozilla.org/mozilla-central/rev/3eba1138ea6d
http://hg.mozilla.org/mozilla-central/rev/3a3109b0d39d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 652361
Depends on: 653179
You need to log in before you can comment on or make changes to this bug.