Bug 121138

Created attachment 211276 [details] Patch

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().

(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.

Created attachment 211326 [details] Patch

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?

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.

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.