Last Comment Bug 174055 - deCOM nsIFontMetrics
: deCOM nsIFontMetrics
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Zack Weinberg (:zwol)
:
Mentors:
Depends on: 266236 648197
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2002-10-11 19:17 PDT by Christopher Blizzard (:blizzard)
Modified: 2011-04-14 10:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: include minimization (30.78 KB, patch)
2011-04-12 17:51 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 2: merge nsIThebesFontMetrics into nsIFontMetrics (31.33 KB, patch)
2011-04-12 17:53 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 3: deCOM ns(I|Thebes)FontMetrics. Code outside gfx/ not fixed up. (68.03 KB, patch)
2011-04-12 17:55 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 4: mechanical fixups (68.62 KB, patch)
2011-04-12 17:58 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 5: prune unused and unimplemented methods. (32.87 KB, patch)
2011-04-12 18:01 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 6: eliminate pointless nsresult return values. (81.62 KB, patch)
2011-04-12 18:03 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 7: remove unnecessary members from nsFontMetrics (7.54 KB, patch)
2011-04-12 18:08 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review
part 8: GetLanguage() doesn't need to addref (3.26 KB, patch)
2011-04-14 09:26 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review

Description Christopher Blizzard (:blizzard) 2002-10-11 19:17:34 PDT
After the checkin for bug 126919 none of these apis are used anywhere.  See that
bug for more details.
Comment 1 Zack Weinberg (:zwol) 2011-04-12 17:14:04 PDT
nsIWidget::SetFont seems to be gone, and I have a patch series deCOMming nsIFontMetrics that nukes GetFontHandle as a side effect, so I'm stealing this bug to carry that patch series.  Hope you don't mind.
Comment 2 Zack Weinberg (:zwol) 2011-04-12 17:51:09 PDT
Created attachment 525578 [details] [diff] [review]
part 1: include minimization

Mostly-mechanical patch to remove all unnecessary #includes of ns*FontMetrics.h.
Comment 3 Zack Weinberg (:zwol) 2011-04-12 17:53:10 PDT
Created attachment 525579 [details] [diff] [review]
part 2: merge nsIThebesFontMetrics into nsIFontMetrics

What it says.  Note I did not bump the nsIFontMetrics IID, because the very next patch makes the IID go away.
Comment 4 Zack Weinberg (:zwol) 2011-04-12 17:55:09 PDT
Created attachment 525580 [details] [diff] [review]
part 3: deCOM ns(I|Thebes)FontMetrics.  Code outside gfx/ not fixed up.

This is the main deCOM patch.  gfx/ can be compiled at this point, but the mechanical fixups needed for the rest of the tree are in the next patch.
Comment 5 Zack Weinberg (:zwol) 2011-04-12 17:58:02 PDT
Created attachment 525581 [details] [diff] [review]
part 4: mechanical fixups

perl -pi -e 's/\bns(?:I|Thebes)(?=FontMetrics\b)/ns/g;
             s/\bnsCOMPtr(?=<nsFontMetrics>)/nsRefPtr/g' [every file in the tree]
Comment 6 Zack Weinberg (:zwol) 2011-04-12 18:01:05 PDT
Created attachment 525582 [details] [diff] [review]
part 5: prune unused and unimplemented methods.

One interesting thing about this patch is that I deleted GetHeight, which was used but documented as "will be removed once callers have been fixed".  It did the same thing as GetMaxHeight, so I changed all remaining callers to use that.  Also there are a couple of technically-unrelated removals of unused variables in here (I was tired of the warnings).
Comment 7 Zack Weinberg (:zwol) 2011-04-12 18:03:22 PDT
Created attachment 525583 [details] [diff] [review]
part 6: eliminate pointless nsresult return values.

Also rearranges code in nsFontMetrics.cpp a little so we can make use of the anonymous namespace for private classes.  The accessors that now return one nscoord also have the "Get" removed from their names.
Comment 8 Zack Weinberg (:zwol) 2011-04-12 18:08:34 PDT
Created attachment 525584 [details] [diff] [review]
part 7: remove unnecessary members from nsFontMetrics

All other places that call CreateFontGroup put the gfxFontStyle argument on the stack, and nothing else in nsFontMetrics uses the gfxFontStyle, so we don't need to heap-allocate that and keep it around for the lifetime of the font metrics.  mFontGroup is a refptr so it doesn't need to be nulled in the constructor.  And there were *two* booleans for whether text runs were right-to-left, one of which was never set to true (but that was the one that was actually getting *used*; I assumed this was just a mistake).

This is as far as this patch series goes.  I have work past this point, trying to disentangle nsFontMetrics from nsThebesDeviceContext, but it's still got bugs in it, and this is enough for one bug.  This set + the set in bug 266236 has passed the try server.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-13 21:39:20 PDT
GetLanguage could return a raw nsIAtom*.
Comment 10 Zack Weinberg (:zwol) 2011-04-13 21:51:52 PDT
(In reply to comment #9)
> GetLanguage could return a raw nsIAtom*.

Good point; I'll tack on another patch to do that tomorrow morning.
Comment 11 Zack Weinberg (:zwol) 2011-04-14 09:26:00 PDT
Created attachment 526019 [details] [diff] [review]
part 8: GetLanguage() doesn't need to addref

Now I remember why I didn't do this in the first place: the only user is nsFontCache::GetMetricsFor, and I was originally going to have part 8 of this bug delete nsFontCache, so it seemed like unnecessary churn.  But that change turns out to expose a bunch of subtle bugs, so this is a better stopping place in case we wind up not being able to delete nsFontCache.

This series is waiting for one more review in bug 648197 before it can land, by the way.

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