Font::primaryFontData() should return a reference

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antti Koivisto]

It is not null

[Antti Koivisto]

Created attachment 243977 [details]
patch

[Antti Koivisto]

Created attachment 243978 [details]
patch2

[Darin Adler]

Comment on attachment 243978 [details]
patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=243978&action=review

A tiny bit sad that something used this often got a little longer, but I suppose it’s strange that a font has a function that returns “a font”.

> Source/WebCore/platform/graphics/FontGlyphs.h:77
> +    const FontData& primaryFontData(const FontDescription& description) { return *realizeFontDataAt(description, 0); }

Do we even need a function for this?

[Antti Koivisto]

(In reply to comment #3)
> Comment on attachment 243978 [details]
> patch2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243978&action=review
> 
> A tiny bit sad that something used this often got a little longer, but I
> suppose it’s strange that a font has a function that returns “a font”.

Yeah. We might even want to rename Font to something like StyleFont (since it is essentially CSS font definition) and FontData to Font.

> Do we even need a function for this?

Not really. I'll get rid of it.

[Myles C. Maxfield]

See https://bugs.webkit.org/show_bug.cgi?id=135290

[Myles C. Maxfield]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 243978 [details]
> > patch2
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=243978&action=review
> > 
> > A tiny bit sad that something used this often got a little longer, but I
> > suppose it’s strange that a font has a function that returns “a font”.
> 
> Yeah. We might even want to rename Font to something like StyleFont (since
> it is essentially CSS font definition) and FontData to Font.
> 
> > Do we even need a function for this?
> 
> Not really. I'll get rid of it.

I actually think that this is a good function to have. It lightens the cognitive load of code that calls this function, and I don't think there is any information lost between just using the name.

[Antti Koivisto]

https://trac.webkit.org/r177955

[Antti Koivisto]

(In reply to comment #6)
> I actually think that this is a good function to have. It lightens the
> cognitive load of code that calls this function, and I don't think there is
> any information lost between just using the name.

There were two functions with similar names. This one was used only in a few places internally in FontGlyphs and was really just creating confusion. Now "primary font data" is always a SimpleFontData.

[Ahmad Saleem]

As per Comment 07, I did landed and there was no backout, I searched with bug number on Webkit Github.

Can this be marked as "RESOLVED FIXED"? Thanks!

[Ryosuke Niwa]

Yup, fixed.

[Radar WebKit Bug Importer]

<rdar://problem/97684797>