Closed
Bug 1257121
Opened 8 years ago
Closed 8 years ago
Simplify code around GetMetricsFor
Categories
(Core :: Graphics: Text, defect)
Core
Graphics: Text
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
A refactor bug for GetMetricsFor.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40703/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40703/
Attachment #8731611 -
Flags: review?(jfkthame)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40705/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40705/
Attachment #8731612 -
Flags: review?(jfkthame)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40707/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40707/
Attachment #8731613 -
Flags: review?(jfkthame)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40709/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40709/
Attachment #8731614 -
Flags: review?(jfkthame)
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96d03c0b8bf4
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40949/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40949/
Attachment #8731950 -
Flags: review?(jfkthame)
Assignee | ||
Comment 7•8 years ago
|
||
It seems RefPtr works just fine with incomplete type: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1afc183cb314
Comment 8•8 years ago
|
||
Comment on attachment 8731950 [details] MozReview Request: Bug 1257121 part 5 - Make nsDeviceContext::mFontCache a RefPtr. https://reviewboard.mozilla.org/r/40949/#review37569 ::: gfx/src/nsDeviceContext.cpp:210 (Diff revision 1) > // Note: we use a bare pointer for mFontCache so that nsFontCache > // can be an incomplete type in nsDeviceContext.h. > // Therefore we have to do all the refcounting by hand. Is this comment no longer relevant? If so, it should be removed. Or is it a valid reason to keep things as they are?
Attachment #8731950 -
Flags: review?(jfkthame)
Comment 9•8 years ago
|
||
Comment on attachment 8731611 [details] MozReview Request: Bug 1257121 part 1 - Use struct for passing some params of font metrics. https://reviewboard.mozilla.org/r/40703/#review37575 ::: gfx/src/nsFontMetrics.h:20 (Diff revision 1) > +#include "nsStyleStruct.h" // for nsStyleFont > +#include "nsPresContext.h" // for nsPresContext I'm not totally comfortable making nsFontMetrics.h depend on #includes from layout... seems kind of backward. It looks like you only need these for the Params constructor that takes an nsStyleFont and nsPresContext. What about instead providing an nsLayoutUtils method to create the nsFontMetrics::Params from these when needed, to avoid the layering inversion here?
Attachment #8731611 -
Flags: review?(jfkthame)
Updated•8 years ago
|
Attachment #8731612 -
Flags: review?(jfkthame)
Comment 10•8 years ago
|
||
Comment on attachment 8731612 [details] MozReview Request: Bug 1257121 part 2 - Merge nsFontMetrics::Init to the constructor and remove unused failure handling code. https://reviewboard.mozilla.org/r/40705/#review37577 ::: gfx/src/nsDeviceContext.cpp (Diff revision 1) > - // One reason why Init() fails is because the system is running out of > - // resources. e.g., on Win95/98 only a very limited number of GDI > - // objects are available. Compact the cache and try again. Are we sure this can no longer happen? AFAIK, GDI resources are still not unlimited...
Assignee | ||
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/40703/#review37575 > I'm not totally comfortable making nsFontMetrics.h depend on #includes from layout... seems kind of backward. > > It looks like you only need these for the Params constructor that takes an nsStyleFont and nsPresContext. What about instead providing an nsLayoutUtils method to create the nsFontMetrics::Params from these when needed, to avoid the layering inversion here? Making it an nsLayoutUtils method probably make sense. The issue is that I hope this function could be inlinable, so that we would not add the cost of an additional function call for this refactor. It seems nsLayoutUtils.h doesn't have access to the definition of nsStyleFont and nsPresContext as well. Probably we can add an "nsFontMetrics-inl.h" file which includes the definition of the constructor, and include it everywhere we need it. Alternatively, we can also just remove the constructor and initialize it explicitly everywhere.
Assignee | ||
Comment 12•8 years ago
|
||
https://reviewboard.mozilla.org/r/40705/#review37577 > Are we sure this can no longer happen? AFAIK, GDI resources are still not unlimited... The reason is that, nsFontMetrics::Init() just never returns any value other than NS_OK. How does it make sense to check its return value at all? If Init() returns any other value, it won't be able to merge the method into the constructor by just removing the last line.
Assignee | ||
Updated•8 years ago
|
Attachment #8731612 -
Flags: review?(jfkthame)
Assignee | ||
Updated•8 years ago
|
Attachment #8731950 -
Flags: review?(jfkthame)
Comment 13•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #12) > https://reviewboard.mozilla.org/r/40705/#review37577 > > > Are we sure this can no longer happen? AFAIK, GDI resources are still not unlimited... > > The reason is that, nsFontMetrics::Init() just never returns any value other > than NS_OK. Ah, yes.... we must have removed whatever failure cases it cared about, long ago.
Updated•8 years ago
|
Attachment #8731612 -
Flags: review?(jfkthame) → review+
Comment 14•8 years ago
|
||
Comment on attachment 8731612 [details] MozReview Request: Bug 1257121 part 2 - Merge nsFontMetrics::Init to the constructor and remove unused failure handling code. https://reviewboard.mozilla.org/r/40705/#review37603 ::: gfx/src/nsFontMetrics.cpp:146 (Diff revision 1) > + if (mDeviceContext) > + mDeviceContext->FontMetricsDeleted(this); Maybe add the missing braces while you're here?
Assignee | ||
Comment 15•8 years ago
|
||
https://reviewboard.mozilla.org/r/40705/#review37603 > Maybe add the missing braces while you're here? What I did is actually moving the code inside Init() to the constructor... but since I do not change code inside, diff thinks I'm moving the destructor... Anyway, okay, I can do it.
Comment 16•8 years ago
|
||
Comment on attachment 8731613 [details] MozReview Request: Bug 1257121 part 3 - Add GetInflatedFontMetricsForFrame function to simplify a common use pattern. https://reviewboard.mozilla.org/r/40707/#review37609 AFAICS, there are only two ways we ever call GetFontMetricsForFrame: either with the FontSizeInflationFor result, or with 1.0. So how about instead of having an overloaded name or a default param (which in effect you're swapping here, which changes the meaning of any callsite that didn't explicitly pass it -- I assume you've checked mozilla-central carefully, but this could cause obscure breakage for thunderbird or seamonkey, for example), how about providing a separate API, GetInflatedFontMetricsForFrame()? The inflation param could then be removed altogether from the existing (uninflated) version (meaning any comm-central code that's depending on this will get a compilation failure, which is better than a silent behavior change), and the implementations of both methods would forward directly to GetFontMetricsForStyleContext().
Attachment #8731613 -
Flags: review?(jfkthame)
Assignee | ||
Comment 17•8 years ago
|
||
https://reviewboard.mozilla.org/r/40707/#review37609 This won't break them silently... because the next patch would change its signature. But yeah, adding another method does make sense. However, we still need the one with inflation params, because some of the callsites do pass in some value neither returned from FontSizeInflationFor nor 1.0. And some of the places can actually take advantage of getting result of FontSizeInflationFor once and reuse it multiple times. FontSizeInflationFor doesn't seem to be as cheap as it may look like.
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8731611 [details] MozReview Request: Bug 1257121 part 1 - Use struct for passing some params of font metrics. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40703/diff/1-2/
Attachment #8731611 -
Flags: review?(jfkthame)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8731612 [details] MozReview Request: Bug 1257121 part 2 - Merge nsFontMetrics::Init to the constructor and remove unused failure handling code. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40705/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8731613 -
Attachment description: MozReview Request: Bug 1257121 part 3 - Make the default value of inflation param of GetFontMetricsForFrame be the result of FontSizeInflationFor. → MozReview Request: Bug 1257121 part 3 - Add GetInflatedFontMetricsForFrame function to simplify a common use pattern.
Attachment #8731613 -
Flags: review?(jfkthame)
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8731613 [details] MozReview Request: Bug 1257121 part 3 - Add GetInflatedFontMetricsForFrame function to simplify a common use pattern. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40707/diff/1-2/
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8731614 [details] MozReview Request: Bug 1257121 part 4 - Use return value rather than out param to return font metrics. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40709/diff/1-2/
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8731950 [details] MozReview Request: Bug 1257121 part 5 - Make nsDeviceContext::mFontCache a RefPtr. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40949/diff/1-2/
Assignee | ||
Comment 23•8 years ago
|
||
https://reviewboard.mozilla.org/r/40949/#review37569 > Is this comment no longer relevant? If so, it should be removed. Or is it a valid reason to keep things as they are? I'm not sure whether it is no longer relevant, or it is never relevant at all. At least, it seems to me changing it to RefPtr can compile successfully (and on all platforms), which indicates that RefPtr accepts incomplete type. So I think it should be fine.
Updated•8 years ago
|
Attachment #8731611 -
Flags: review?(jfkthame) → review+
Comment 24•8 years ago
|
||
Comment on attachment 8731611 [details] MozReview Request: Bug 1257121 part 1 - Use struct for passing some params of font metrics. https://reviewboard.mozilla.org/r/40703/#review37837 I pulled this into my local tree, and ran into one small problem; r=me with that fixed. ::: gfx/src/nsDeviceContext.cpp:130 (Diff revision 2) > - gfxFont::Orientation aOrientation, > - gfxUserFontSet* aUserFontSet, > - gfxTextPerfMetrics* aTextPerf, > nsFontMetrics*& aMetrics) > { > - if (!aLanguage) > + nsIAtom* language = aParams.language ? aParams.language : mLocaleLanguage; This line failed to compile for me locally (on OS X 10.9.5): 0:33.16 /Users/jkew/mozdev/mozilla-central/gfx/src/nsDeviceContext.cpp:128:42: error: conditional expression is ambiguous; 'nsIAtom *const' can be converted to 'nsCOMPtr<nsIAtom>' and vice versa 0:33.16 nsIAtom* language = aParams.language ? aParams.language : mLocaleLanguage; 0:33.16 ^ ~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~ 0:33.27 1 error generated. Adding .get() to mLocaleLanguage fixes it for me.
Comment 25•8 years ago
|
||
Comment on attachment 8731613 [details] MozReview Request: Bug 1257121 part 3 - Add GetInflatedFontMetricsForFrame function to simplify a common use pattern. https://reviewboard.mozilla.org/r/40707/#review37841 r=me, with bonus points for considering the suggested addition. ::: layout/base/nsLayoutUtils.h:1229 (Diff revision 2) > static nsresult GetFontMetricsForFrame(const nsIFrame* aFrame, > nsFontMetrics** aFontMetrics, > float aSizeInflation = 1.0f); > > + static nsresult GetInflatedFontMetricsForFrame(const nsIFrame* aFrame, > + nsFontMetrics** aFontMetrics) > + { > + return GetFontMetricsForFrame(aFrame, aFontMetrics, > + FontSizeInflationFor(aFrame)); > + } In addition to moving many callers to the new GetInflated... version, I wonder if it'd be good to *remove* the default value for aSizeInflation from the existing method? It won't affect a huge number of callers -- looks to me like less than a dozen, in a handful of classes -- and I think there's some benefit to making it more explicit that they're specifically choosing *not* to apply an inflation factor.
Attachment #8731613 -
Flags: review?(jfkthame) → review+
Updated•8 years ago
|
Attachment #8731614 -
Flags: review?(jfkthame) → review+
Comment 26•8 years ago
|
||
Comment on attachment 8731614 [details] MozReview Request: Bug 1257121 part 4 - Use return value rather than out param to return font metrics. https://reviewboard.mozilla.org/r/40709/#review37843
Comment 27•8 years ago
|
||
Comment on attachment 8731950 [details] MozReview Request: Bug 1257121 part 5 - Make nsDeviceContext::mFontCache a RefPtr. https://reviewboard.mozilla.org/r/40949/#review37845
Attachment #8731950 -
Flags: review?(jfkthame) → review+
Comment 28•8 years ago
|
||
Thanks for cleaning this up -- the old code here has been annoying me for a long time!
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8731611 [details] MozReview Request: Bug 1257121 part 1 - Use struct for passing some params of font metrics. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40703/diff/2-3/
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8731612 [details] MozReview Request: Bug 1257121 part 2 - Merge nsFontMetrics::Init to the constructor and remove unused failure handling code. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40705/diff/2-3/
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8731613 [details] MozReview Request: Bug 1257121 part 3 - Add GetInflatedFontMetricsForFrame function to simplify a common use pattern. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40707/diff/2-3/
Assignee | ||
Comment 32•8 years ago
|
||
Comment on attachment 8731614 [details] MozReview Request: Bug 1257121 part 4 - Use return value rather than out param to return font metrics. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40709/diff/2-3/
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8731950 [details] MozReview Request: Bug 1257121 part 5 - Make nsDeviceContext::mFontCache a RefPtr. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40949/diff/2-3/
Comment 34•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af78cb01749 https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a24f3d0e6e https://hg.mozilla.org/integration/mozilla-inbound/rev/d3761d63e160 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae38f8e1def https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b6ae5094f8
Comment 35•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4af78cb01749 https://hg.mozilla.org/mozilla-central/rev/c3a24f3d0e6e https://hg.mozilla.org/mozilla-central/rev/d3761d63e160 https://hg.mozilla.org/mozilla-central/rev/8ae38f8e1def https://hg.mozilla.org/mozilla-central/rev/c8b6ae5094f8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•