Bug 121138 – [iOS] Upstream Source/WebCore/rendering/TextAutoSizing.{cpp, h}

• Status: NEW
• Component: WebCore Misc.

[Daniel Bates]

Upstream file Source/WebCore/rendering/TextAutoSizing.{cpp, h}.

[Daniel Bates]

Created attachment 211276 [details]
Patch

[Sam Weinig]

Comment on attachment 211276 [details]
Patch

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

> Source/WebCore/ChangeLog:31
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * rendering/TextAutoSizing.cpp: Added.
> +        (WebCore::cloneRenderStyleWithState):
> +        (WebCore::TextAutoSizingKey::TextAutoSizingKey):
> +        (WebCore::TextAutoSizingKey::~TextAutoSizingKey):
> +        (WebCore::TextAutoSizingKey::operator=):
> +        (WebCore::TextAutoSizingKey::ref):
> +        (WebCore::TextAutoSizingKey::deref):
> +        (WebCore::TextAutoSizingValue::numNodes):
> +        (WebCore::TextAutoSizingValue::addNode):
> +        (WebCore::TextAutoSizingValue::adjustNodeSizes):
> +        (WebCore::TextAutoSizingValue::reset):
> +        * rendering/TextAutoSizing.h: Added.
> +        (WebCore::TextAutoSizingKey::doc):
> +        (WebCore::TextAutoSizingKey::style):
> +        (WebCore::TextAutoSizingKey::isValidDoc):
> +        (WebCore::TextAutoSizingKey::isValidStyle):
> +        (WebCore::TextAutoSizingKey::deletedKeyDoc):
> +        (WebCore::TextAutoSizingKey::deletedKeyStyle):
> +        (WebCore::operator==):
> +        (WebCore::TextAutoSizingHash::hash):
> +        (WebCore::TextAutoSizingHash::equal):
> +        (WebCore::TextAutoSizingValue::create):
> +        (WebCore::TextAutoSizingValue::TextAutoSizingValue):

You don't need all the per-function lines.

> Source/WebCore/rendering/TextAutoSizing.cpp:116
> +    Vector<RefPtr<Node> > nodesForRemoval;

This should probably use Vector<Ref<Node>>.

> Source/WebCore/rendering/TextAutoSizing.cpp:118
> +    HashSet<RefPtr<Node> >::iterator end = m_autoSizedNodes.end();
> +    for (HashSet<RefPtr<Node> >::iterator i = m_autoSizedNodes.begin(); i != end; ++i) {

This should use auto.

> Source/WebCore/rendering/TextAutoSizing.cpp:120
> +        RenderText* text = static_cast<RenderText*>(autoSizingNode->renderer());

toRenderText()?

> Source/WebCore/rendering/TextAutoSizing.cpp:140
> +    end = m_autoSizedNodes.end();
> +    for (HashSet<RefPtr<Node> >::iterator i = m_autoSizedNodes.begin(); i != end; ++i) {

This should use auto.

> Source/WebCore/rendering/TextAutoSizing.cpp:142
> +        RenderText* renderText = static_cast<RenderText*>(autoSizingNode->renderer());

toRenderText()?

> Source/WebCore/rendering/TextAutoSizing.cpp:151
> +    end = m_autoSizedNodes.end();
> +    for (HashSet<RefPtr<Node> >::iterator i = m_autoSizedNodes.begin(); i != end; ++i) {

This should use auto.

> Source/WebCore/rendering/TextAutoSizing.cpp:153
> +        RenderText* text = static_cast<RenderText*>(autoSizingNode->renderer());

toRenderText()?

> Source/WebCore/rendering/TextAutoSizing.cpp:211
> +    HashSet<RefPtr<Node> >::iterator end = m_autoSizedNodes.end();
> +    for (HashSet<RefPtr<Node> >::iterator i = m_autoSizedNodes.begin(); i != end; ++i) {

This should use auto.

> Source/WebCore/rendering/TextAutoSizing.cpp:213
> +        RenderText* text = static_cast<RenderText*>(autoSizingNode->renderer());

This should use toRenderText().

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 211276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211276&action=review
> 
> > Source/WebCore/ChangeLog:31
> > +        * WebCore.xcodeproj/project.pbxproj:
> > +        * rendering/TextAutoSizing.cpp: Added.
>> [...]
> > +        (WebCore::TextAutoSizingHash::hash):
> > +        (WebCore::TextAutoSizingHash::equal):
> > +        (WebCore::TextAutoSizingValue::create):
> > +        (WebCore::TextAutoSizingValue::TextAutoSizingValue):
> 
> You don't need all the per-function lines.
> 

Will remove per-function lines.

> > Source/WebCore/rendering/TextAutoSizing.cpp:116
> > +    Vector<RefPtr<Node> > nodesForRemoval;
> 
> This should probably use Vector<Ref<Node>>.
> 

Will fix.

> > Source/WebCore/rendering/TextAutoSizing.cpp:118
> > +    HashSet<RefPtr<Node> >::iterator end = m_autoSizedNodes.end();
> > +    for (HashSet<RefPtr<Node> >::iterator i = m_autoSizedNodes.begin(); i != end; ++i) {
> 
> This should use auto.
> 

Will fix.

> > Source/WebCore/rendering/TextAutoSizing.cpp:120
> > +        RenderText* text = static_cast<RenderText*>(autoSizingNode->renderer());
> 
> toRenderText()?
> 

Will fix.

> > Source/WebCore/rendering/TextAutoSizing.cpp:140
> > +    end = m_autoSizedNodes.end();
> > +    for (HashSet<RefPtr<Node> >::iterator i = m_autoSizedNodes.begin(); i != end; ++i) {
> 
> This should use auto.
> 

Will fix.

> > Source/WebCore/rendering/TextAutoSizing.cpp:142
> > +        RenderText* renderText = static_cast<RenderText*>(autoSizingNode->renderer());
> 
> toRenderText()?
> 

Will fix.

> > Source/WebCore/rendering/TextAutoSizing.cpp:151
> > +    end = m_autoSizedNodes.end();
> > +    for (HashSet<RefPtr<Node> >::iterator i = m_autoSizedNodes.begin(); i != end; ++i) {
> 
> This should use auto.
> 

Will fix.

> > Source/WebCore/rendering/TextAutoSizing.cpp:153
> > +        RenderText* text = static_cast<RenderText*>(autoSizingNode->renderer());
> 
> toRenderText()?
> 

Will fix.

> > Source/WebCore/rendering/TextAutoSizing.cpp:211
> > +    HashSet<RefPtr<Node> >::iterator end = m_autoSizedNodes.end();
> > +    for (HashSet<RefPtr<Node> >::iterator i = m_autoSizedNodes.begin(); i != end; ++i) {
> 
> This should use auto.
> 

Will fix.

> > Source/WebCore/rendering/TextAutoSizing.cpp:213
> > +        RenderText* text = static_cast<RenderText*>(autoSizingNode->renderer());
> 
> This should use toRenderText().

Will fix.

[Daniel Bates]

Created attachment 211326 [details]
Patch

[Darin Adler]

Comment on attachment 211326 [details]
Patch

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

> Source/WebCore/rendering/TextAutoSizing.h:41
> +class TextAutoSizingKey {

This is way overkill. Should just be a struct with two RefPtr members in it; no need for constructors, and all these functions.

> Source/WebCore/rendering/TextAutoSizing.h:44
> +    TextAutoSizingKey(RenderStyle*, Document*);

Should take PassRefPtr.

> Source/WebCore/rendering/TextAutoSizing.h:48
> +    Document* doc() const { return m_doc; }

Should name this document.

> Source/WebCore/rendering/TextAutoSizing.h:51
> +    inline bool isValidDoc() const { return m_doc && m_doc != deletedKeyDoc(); }
> +    inline bool isValidStyle() const { return m_style && m_style != deletedKeyStyle(); }

These seem like a misunderstanding of some sort. A deleted object should never get any calls on it and so this should not need any handling for deleted objects.

> Source/WebCore/rendering/TextAutoSizing.h:52
> +    static Document* deletedKeyDoc() { return reinterpret_cast<Document*>(-1); }

Don’t need this.

> Source/WebCore/rendering/TextAutoSizing.h:56
> +    void ref() const;
> +    void deref() const;

Should use RefPtr for data members and not have all these explicit ref/deref t all.

> Source/WebCore/rendering/TextAutoSizing.h:58
> +    Document* m_doc;

Should name this m_document.

> Source/WebCore/rendering/TextAutoSizing.h:71
> +    static const bool safeToCompareToEmptyOrDeleted = true;

Should set this to false and then we don’t need all the isValid stuff.

> Source/WebCore/rendering/TextAutoSizing.h:81
> +    void addNode(Node*, float size);

PassRefPtr. Maybe Element instead of Node?

[Darin Adler]

Comment on attachment 211326 [details]
Patch

Consider all of my suggestions as optional improvements, not a necessary part of getting this upstreamed for the first time.

[Andreas Kling]

Comment on attachment 211326 [details]
Patch

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

> Source/WebCore/rendering/TextAutoSizing.cpp:91
> +    if (isValidStyle() && isValidDoc() && m_doc->renderArena())
> +        m_style->deref();

RenderStyle objects are not allocated in the RenderArena (today.)
The m_doc->renderArena() check should be omitted, since otherwise we could leak a ref here.