Closed Bug 1257121 Opened 8 years ago Closed 8 years ago

Simplify code around GetMetricsFor

Categories

(Core :: Graphics: Text, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(5 files)

A refactor bug for GetMetricsFor.
It seems RefPtr works just fine with incomplete type:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1afc183cb314
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 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)
Attachment #8731612 - Flags: review?(jfkthame)
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...
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.
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.
Attachment #8731612 - Flags: review?(jfkthame)
Attachment #8731950 - Flags: review?(jfkthame)
(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.
Attachment #8731612 - Flags: review?(jfkthame) → review+
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?
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 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)
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.
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)
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/
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)
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/
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/
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/
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.
Attachment #8731611 - Flags: review?(jfkthame) → review+
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 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+
Attachment #8731614 - Flags: review?(jfkthame) → review+
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 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+
Thanks for cleaning this up -- the old code here has been annoying me for a long time!
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/
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/
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/
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/
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/
Blocks: 1097499
You need to log in before you can comment on or make changes to this bug.