Clean up / modernize iOS text autosizing code

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Clean up / modernize iOS text autosizing code.

[Chris Dumez]

Created attachment 280115 [details]
Patch

[Darin Adler]

Comment on attachment 280115 [details]
Patch

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

Should change the header file to use #pragma once.

> Source/WebCore/ChangeLog:25
> +        - Rename node to textNode for clarity.

Not sure we need this change.

> Source/WebCore/dom/Document.cpp:5290
> +    auto addResult = m_textAutoSizedNodes.ensure(TextAutoSizingKey(textNode.renderer()->style()), [] {
> +        return std::make_unique<TextAutoSizingValue>();
>      });

We only need to clone the RenderStyle if we are actually allocating a new entry in the table, but the way the code works, we always make and destroy a TextAutoSizingKey every time even if it’s already in the table; that’s a lot of extra cloning! Instead of ensure, we should use the add with a hash translator to accomplish this. And we should get rid of the TextAutoSizingKey entirely and have the keys just be unique_ptr<RenderStyle>.

> Source/WebCore/dom/Document.cpp:5294
>  void Document::validateAutoSizingNodes()

This is not a great use of the name "validate". I would call this updateAutoSizingNodes, I think.

> Source/WebCore/dom/Document.h:1697
> +    typedef HashMap<TextAutoSizingKey, std::unique_ptr<TextAutoSizingValue>, TextAutoSizingHash, TextAutoSizingTraits> TextAutoSizingMap;

Is there a reason the values need to be heap allocated instead of stored in the map? I understand that these are sets, but can’t we modify them in place inside the map?

> Source/WebCore/rendering/TextAutoSizing.cpp:93
>      float averageSize = roundf(cumulativeSize / m_autoSizedNodes.size());

Should be std::round instead of roundf.

> Source/WebCore/rendering/TextAutoSizing.cpp:106
> +            averageSize = roundf(specifiedSize * maxScaleIncrease);

std::round

> Source/WebCore/rendering/TextAutoSizing.cpp:166
> +        auto* renderText = node->renderer();

I think a good name for this is renderer. No need to name it after the class.

> Source/WebCore/rendering/TextAutoSizing.h:52
>      std::unique_ptr<RenderStyle> m_style;

Seems like TextAutoSizingKey is actually std::unique_ptr<RenderStyle>. Not sure it should be a class at all. Maybe just hash table traits.

> Source/WebCore/rendering/TextAutoSizing.h:96
>      HashSet<RefPtr<Text>> m_autoSizedNodes;

This should be HashSet<Ref> rather than HashSet<RefPtr>.

[Chris Dumez]

Comment on attachment 280115 [details]
Patch

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

>> Source/WebCore/ChangeLog:25
>> +        - Rename node to textNode for clarity.
> 
> Not sure we need this change.

Ok.

>> Source/WebCore/dom/Document.cpp:5290
>>      });
> 
> We only need to clone the RenderStyle if we are actually allocating a new entry in the table, but the way the code works, we always make and destroy a TextAutoSizingKey every time even if it’s already in the table; that’s a lot of extra cloning! Instead of ensure, we should use the add with a hash translator to accomplish this. And we should get rid of the TextAutoSizingKey entirely and have the keys just be unique_ptr<RenderStyle>.

Great idea, I will add a HashTranslator.

>> Source/WebCore/dom/Document.cpp:5294
>>  void Document::validateAutoSizingNodes()
> 
> This is not a great use of the name "validate". I would call this updateAutoSizingNodes, I think.

Ok

>> Source/WebCore/dom/Document.h:1697
>> +    typedef HashMap<TextAutoSizingKey, std::unique_ptr<TextAutoSizingValue>, TextAutoSizingHash, TextAutoSizingTraits> TextAutoSizingMap;
> 
> Is there a reason the values need to be heap allocated instead of stored in the map? I understand that these are sets, but can’t we modify them in place inside the map?

Ok.

>> Source/WebCore/rendering/TextAutoSizing.cpp:93
>>      float averageSize = roundf(cumulativeSize / m_autoSizedNodes.size());
> 
> Should be std::round instead of roundf.

Ok

>> Source/WebCore/rendering/TextAutoSizing.cpp:166
>> +        auto* renderText = node->renderer();
> 
> I think a good name for this is renderer. No need to name it after the class.

This seemed to be the convention in rendering code but OK.

>> Source/WebCore/rendering/TextAutoSizing.h:52
>>      std::unique_ptr<RenderStyle> m_style;
> 
> Seems like TextAutoSizingKey is actually std::unique_ptr<RenderStyle>. Not sure it should be a class at all. Maybe just hash table traits.

Sounds like a good idea but I'd rather not take on this task in this patch.

>> Source/WebCore/rendering/TextAutoSizing.h:96
>>      HashSet<RefPtr<Text>> m_autoSizedNodes;
> 
> This should be HashSet<Ref> rather than HashSet<RefPtr>.

Sadly, Ref<> does not play well with HashSet and HashMap AFAIK. It does not have an operator==() or a default constructor.

[Chris Dumez]

Created attachment 280119 [details]
Patch

[Darin Adler]

Comment on attachment 280119 [details]
Patch

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

A few last comments.

> Source/WebCore/dom/Document.cpp:5288
> +    auto addResult = m_textAutoSizedNodes.add<TextAutoSizingHashTranslator>(node.renderer()->style(), TextAutoSizingValue());

A little sad that we have to create/destroy the empty HashSet every time here. I wonder what the best trick for avoiding that is. I can think of three tricks off hand:

1) Add a version of HashMap::add with the hash translator that uses an empty value for the new hash map entry if any and does not require a value argument be passed in.
2) Add a version of HashMap::ensure with the hash translator.
3) Instead of being a map, m_textAutoSizedNodes could be a HashSet with a mutable HashSet inside the set values. We would combine TextAutoSizingKey and TextAutoSizingValue into a single class. Would probably have to add HashSet::removeIf too.

> Source/WebCore/dom/Document.h:49
> +#include "TextAutoSizing.h"

Always sad for build times when we add another include to Document.h.

> Source/WebCore/dom/Document.h:1697
> +    typedef HashMap<TextAutoSizingKey, TextAutoSizingValue, TextAutoSizingHash, TextAutoSizingTraits> TextAutoSizingMap;

Could switch to using; we prefer that to typedef now in every case, I think.

> Source/WebCore/dom/Document.h:1698
>      TextAutoSizingMap m_textAutoSizedNodes;

Kind of strange that all the functions and the typedef call these "auto sizing nodes", but the data member is named "auto sized nodes".

> Source/WebCore/rendering/TextAutoSizing.cpp:52
> +    new (NotNull, std::addressof(m_style)) std::unique_ptr<RenderStyle>(reinterpret_cast<RenderStyle*>(-1));

I would have preferred to write this:

    HashTraits<std::unique_ptr<RenderStyle>>::constructDeletedValue(m_style);

> Source/WebCore/rendering/TextAutoSizing.h:50
> +    bool isDeleted() const { return m_style.get() == reinterpret_cast<RenderStyle*>(-1); }

I’d rather see:

    bool isDeleted() const { return HashTraits<std::unique_ptr<RenderStyle>>::isDeletedValue(m_style); }

> Source/WebCore/rendering/TextAutoSizing.h:103
> +struct TextAutoSizingHashTranslator {
> +    static unsigned hash(const RenderStyle& style)
> +    {
> +        return style.hashForTextAutosizing();
> +    }
> +
> +    static bool equal(const TextAutoSizingKey& key, const RenderStyle& style)
> +    {
> +        if (key.isDeleted() || !key.style())
> +            return false;
> +        return key.style()->equalForTextAutosizing(style);
> +    }
> +
> +    static void translate(TextAutoSizingKey& key, const RenderStyle& style, unsigned hash)
> +    {
> +        key = TextAutoSizingKey(style, hash);
> +    }
> +};

This does not need to be in a header. How about moving it inside TextAutoSizing.cpp?

Also, I think you could use initializer list style when setting the value of the key in the translate function.

I would have written the equal function as an && expression instead of an if statement.

[Darin Adler]

Comment on attachment 280119 [details]
Patch

None of my comments are "don’t land this yet" comments; all just ideas to make the code even better.

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 280119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280119&action=review
> 
> A few last comments.
> 
> > Source/WebCore/dom/Document.cpp:5288
> > +    auto addResult = m_textAutoSizedNodes.add<TextAutoSizingHashTranslator>(node.renderer()->style(), TextAutoSizingValue());
> 
> A little sad that we have to create/destroy the empty HashSet every time
> here. I wonder what the best trick for avoiding that is. I can think of
> three tricks off hand:
> 
> 1) Add a version of HashMap::add with the hash translator that uses an empty
> value for the new hash map entry if any and does not require a value
> argument be passed in.
> 2) Add a version of HashMap::ensure with the hash translator.
> 3) Instead of being a map, m_textAutoSizedNodes could be a HashSet with a
> mutable HashSet inside the set values. We would combine TextAutoSizingKey
> and TextAutoSizingValue into a single class. Would probably have to add
> HashSet::removeIf too.

4) Go back to using a std::unique_ptr<>() for values as in the original code and call add() with nullptr?

This seems the simplest.

[Chris Dumez]

Comment on attachment 280119 [details]
Patch

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

>> Source/WebCore/dom/Document.h:49
>> +#include "TextAutoSizing.h"
> 
> Always sad for build times when we add another include to Document.h.

The issue is that we need the hash traits for text autosizing in Document.h but I thought, those traits should really not be in Document.h.
To decrease build time, I guess we could move those traits to their own header (e.g. TextAutoSizingHashTraits.h) and include this header here instead? What do you think?

>> Source/WebCore/dom/Document.h:1697
>> +    typedef HashMap<TextAutoSizingKey, TextAutoSizingValue, TextAutoSizingHash, TextAutoSizingTraits> TextAutoSizingMap;
> 
> Could switch to using; we prefer that to typedef now in every case, I think.

Ok.

>> Source/WebCore/dom/Document.h:1698
>>      TextAutoSizingMap m_textAutoSizedNodes;
> 
> Kind of strange that all the functions and the typedef call these "auto sizing nodes", but the data member is named "auto sized nodes".

Ok, I can harmonize.

>> Source/WebCore/rendering/TextAutoSizing.cpp:52
>> +    new (NotNull, std::addressof(m_style)) std::unique_ptr<RenderStyle>(reinterpret_cast<RenderStyle*>(-1));
> 
> I would have preferred to write this:
> 
>     HashTraits<std::unique_ptr<RenderStyle>>::constructDeletedValue(m_style);

Good idea.

>> Source/WebCore/rendering/TextAutoSizing.h:50
>> +    bool isDeleted() const { return m_style.get() == reinterpret_cast<RenderStyle*>(-1); }
> 
> I’d rather see:
> 
>     bool isDeleted() const { return HashTraits<std::unique_ptr<RenderStyle>>::isDeletedValue(m_style); }

Good idea.

>> Source/WebCore/rendering/TextAutoSizing.h:103
>> +};
> 
> This does not need to be in a header. How about moving it inside TextAutoSizing.cpp?
> 
> Also, I think you could use initializer list style when setting the value of the key in the translate function.
> 
> I would have written the equal function as an && expression instead of an if statement.

If it is not in the header, how can we use it from Document.cpp? Did you want to move it to Document.cpp instead? Or did you want me to keep the struct in the header and move the implementation out of line in TextAutoSizing.cpp?

[Chris Dumez]

Created attachment 280175 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 280175 [details]
Patch

Clearing flags on attachment: 280175

Committed r201534: <http://trac.webkit.org/changeset/201534>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.