[webkit-reviews] review granted: [Bug 121130] Unexpected word wrapping for wrapped content then raw content : [Attachment 212592] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zalan Bujtas <zalan at apple.com>'s
request for review:
Bug 121130: Unexpected word wrapping for wrapped content then raw content
https://bugs.webkit.org/show_bug.cgi?id=121130

Attachment 212592: Patch
https://bugs.webkit.org/attachment.cgi?id=212592&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=212592&action=review


> Source/WebCore/rendering/LineWidth.cpp:158
> +    UncommittedWidthMap::const_iterator it =
m_uncommittedWidthMap.find(&object);
> +    if (it != m_uncommittedWidthMap.end())
> +	   return it->value;
> +    return -1;

If this was returning 0 we could just use get, which would result in easier to
read code. But I assume there’s a good reason we need to return -1 rather than
0.

> Source/WebCore/rendering/LineWidth.cpp:169
> +    UncommittedWidthMap::iterator it = m_uncommittedWidthMap.find(&current);

> +    if (it == m_uncommittedWidthMap.end())
> +	   m_uncommittedWidthMap.add(&current, delta);
> +    else
> +	   it->value += delta;

This is the way to write this efficiently:

    auto result = m_uncommittedWidthMap.add(&current, delta);
    if (!result.isNewEntry)
	result.iterator->value += delta;

Writing it this way does only a single hash table lookup, even the first time.

> Source/WebCore/rendering/LineWidth.h:95
> +    typedef HashMap<const RenderObject*, float> UncommittedWidthMap;
> +    UncommittedWidthMap m_uncommittedWidthMap;

Our more modern style is to not use a typedef, and use “auto” instead when
defining iterators. I suggest following that style instead.

Is it really OK to add a map to every one of these? Even empty maps are
relatively big objects.