Bug 116783 – Use Vector instead of custom linked list for font families

• Component: Text
• Status: RESOLVED FIXED

[Antti Koivisto]

And kill a few classes.

[Antti Koivisto]

Created attachment 202915 [details]
patch

[Andreas Kling]

Comment on attachment 202915 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

OOPS!

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1450
>  static PassRefPtr<CSSValueList> fontFamilyFromStyle(RenderStyle* style)
> -{
> -    const FontFamily& firstFamily = style->fontDescription().family();
> +    {

Strange indentation here.

> Source/WebCore/rendering/RenderText.cpp:211
> -    } else if (oldStyle->font().needsTranscoding() != newStyle->font().needsTranscoding() || (newStyle->font().needsTranscoding() && oldStyle->font().family().family() != newStyle->font().family().family())) {
> +    } else if (oldStyle->font().needsTranscoding() != newStyle->font().needsTranscoding() || (newStyle->font().needsTranscoding() && oldStyle->font().fontDescription().families() != newStyle->font().fontDescription().families())) {

You're now comparing all families here instead of just the first one. Looks wrong.

[Early Warning System Bot]

Comment on attachment 202915 [details]
patch

Attachment 202915 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/677274

[Early Warning System Bot]

Comment on attachment 202915 [details]
patch

Attachment 202915 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/677273

[EFL EWS Bot]

Comment on attachment 202915 [details]
patch

Attachment 202915 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/665163

[EFL EWS Bot]

Comment on attachment 202915 [details]
patch

Attachment 202915 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/744168

[Build Bot]

Comment on attachment 202915 [details]
patch

Attachment 202915 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/744167

[Antti Koivisto]

Created attachment 202916 [details]
patch 2

[EFL EWS Bot]

Comment on attachment 202916 [details]
patch 2

Attachment 202916 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/655189

[Build Bot]

Comment on attachment 202916 [details]
patch 2

Attachment 202916 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/744171

[EFL EWS Bot]

Comment on attachment 202916 [details]
patch 2

Attachment 202916 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/730153

[Antti Koivisto]

Created attachment 202918 [details]
more port fixes

[Build Bot]

Comment on attachment 202918 [details]
more port fixes

Attachment 202918 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/652347

[Antti Koivisto]

Created attachment 202920 [details]
another

[Andreas Kling]

Comment on attachment 202920 [details]
another

r=me

[Build Bot]

Comment on attachment 202920 [details]
another

Attachment 202920 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/744186

[Antti Koivisto]

http://trac.webkit.org/changeset/150716

[Darin Adler]

Comment on attachment 202920 [details]
another

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

> Source/WebCore/platform/graphics/FontDescription.h:144
> +    void setOneFamily(const AtomicString& family) { ASSERT(m_families.size() == 1); m_families[0] = family; }

I am a little confused by this. How do callers know the families list already happens to be one family long in all the cases where this is used?

The name “set one family” also seems a little confusing to me. It sounds like this sets one of the families and leaves the rest alone. I would have maybe called this setSingleFamily. But really I am mystified by the way the code can assume there is only one family.

> Source/WebCore/platform/graphics/FontDescription.h:173
> -    FontFamily m_familyList; // The list of font families to be used.
> +    Vector<AtomicString, 1> m_families;

The old thing was two pointers. Isn’t Vector<AtomicString, 1> much bigger than that?

[Antti Koivisto]

(In reply to comment #18)
> (From update of attachment 202920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202920&action=review
> 
> > Source/WebCore/platform/graphics/FontDescription.h:144
> > +    void setOneFamily(const AtomicString& family) { ASSERT(m_families.size() == 1); m_families[0] = family; }
> 
> I am a little confused by this. How do callers know the families list already happens to be one family long in all the cases where this is used?

FontDescription is not meant to be mutated after the initial setup. The size is set to one in constructor to match the old behaviour and limit the refactoring. This could actually assert that the family is null too.

> The name “set one family” also seems a little confusing to me. It sounds like this sets one of the families and leaves the rest alone. I would have maybe called this setSingleFamily. But really I am mystified by the way the code can assume there is only one family.

Yeah, setSingleFamily would be a better name. Removing the whole thing in favour of constructor parameter would be better.

> The old thing was two pointers. Isn’t Vector<AtomicString, 1> much bigger than that?

This is not meant to be the end state. A simplification step will hopefully be helpful to get bigger gains.

[Andreas Kling]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 202920 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202920&action=review
> > The old thing was two pointers. Isn’t Vector<AtomicString, 1> much bigger than that?
> 
> This is not meant to be the end state. A simplification step will hopefully be helpful to get bigger gains.

FWIW, the new object is one pointer bigger, but has much better heap layout with >1 family.