Encapsulate FloatingObject's constructor inside create
Created attachment 188967 [details] Cleanup
Comment on attachment 188967 [details] Cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=188967&action=review > Source/WebCore/rendering/RenderBlock.cpp:7714 > +void RenderBlock::ensureFloatingObjects() I’m not such a big fan of the naming here. I understand, though, that we’re using the word “ensure” for this in many places in WebKit now. But I have two thoughts: 1) We should find a better name or short phrase for this that is less like jargon if we can. 2) In other places, I think this is only used when the function has a return value. To me this is just createFloatingObjects. The fact that it's smart enough not to recreate it if it already exists seems like a detail that need not be in its name. > Source/WebCore/rendering/RenderBlock.cpp:7717 > + if (!m_floatingObjects) > + m_floatingObjects = FloatingObjects::create(this, isHorizontalWritingMode()); I like early return when we already have it better than nesting like this, but it's just a matter of taste. > Source/WebCore/rendering/RenderBlock.cpp:7720 > +inline PassOwnPtr<RenderBlock::FloatingObjects> RenderBlock::FloatingObjects::create(const RenderBlock* renderer, bool horizontalWritingMode) With older compilers, the inline needs to before the code that uses it. Thus, I suggest reordering these three things in exactly the reverse order they currently appear.
Committed r143289: <http://trac.webkit.org/changeset/143289>
Apparently this patch doesn't work on Windows :( Build fix landed in http://trac.webkit.org/changeset/143293.
Still doesn't work: 6>C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\include\private\wtf/OwnPtrCommon.h(63) : error C2248: 'WebCore::RenderBlock::FloatingObjects' : cannot access protected class declared in class 'WebCore::RenderBlock' 6> c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderBlock.h(1140) : see declaration of 'WebCore::RenderBlock::FloatingObjects' 6> c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderBlock.h(83) : see declaration of 'WebCore::RenderBlock' 6> C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\include\private\wtf/OwnPtr.h(63) : see reference to function template instantiation 'void WTF::deleteOwnedPtr<WebCore::RenderBlock::FloatingObjects>(T *)' being compiled 6> with 6> [ 6> T=WebCore::RenderBlock::FloatingObjects 6> ] 6> C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\include\private\wtf/OwnPtr.h(63) : while compiling class template member function 'WTF::OwnPtr<T>::~OwnPtr(void)' 6> with 6> [ 6> T=WebCore::RenderBlock::FloatingObjects 6> ] 6> c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderBlock.h(1180) : see reference to class template instantiation 'WTF::OwnPtr<T>' being compiled 6> with 6> [ 6> T=WebCore::RenderBlock::FloatingObjects 6> ]
I made the class public: http://trac.webkit.org/changeset/143294 This is why I hate nested classes. They never work.
Reopening to attach new patch.
Created attachment 188998 [details] Cleanup after build fixes
Comment on attachment 188998 [details] Cleanup after build fixes Attachment 188998 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16627369
Comment on attachment 188998 [details] Cleanup after build fixes Attachment 188998 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16613540
Comment on attachment 188998 [details] Cleanup after build fixes Attachment 188998 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16627371
Comment on attachment 188998 [details] Cleanup after build fixes Attachment 188998 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16627370
Comment on attachment 188998 [details] Cleanup after build fixes Attachment 188998 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16627382
Comment on attachment 188998 [details] Cleanup after build fixes Attachment 188998 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16612585
Created attachment 189009 [details] Upload the right file
Comment on attachment 189009 [details] Upload the right file Attachment 189009 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16621547
Comment on attachment 189009 [details] Upload the right file Attachment 189009 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16628344
Comment on attachment 189009 [details] Upload the right file Attachment 189009 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16608735
Comment on attachment 189009 [details] Upload the right file Attachment 189009 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16622002
Comment on attachment 189009 [details] Upload the right file Attachment 189009 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16632001
Comment on attachment 189009 [details] Upload the right file Attachment 189009 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16620003
Created attachment 189024 [details] Fixed builds
Comment on attachment 189024 [details] Fixed builds Attachment 189024 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16629325
Comment on attachment 189024 [details] Fixed builds Attachment 189024 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16610700
Comment on attachment 189024 [details] Fixed builds Attachment 189024 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16627433
Comment on attachment 189024 [details] Fixed builds Attachment 189024 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16614661
Comment on attachment 189024 [details] Fixed builds Attachment 189024 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16610717
Comment on attachment 189024 [details] Fixed builds Attachment 189024 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16653297
Comment on attachment 189024 [details] Fixed builds View in context: https://bugs.webkit.org/attachment.cgi?id=189024&action=review > Source/WebCore/rendering/RenderBlock.cpp:7752 > +inline PassOwnPtr<RenderBlock::FloatingObjects> RenderBlock::FloatingObjects::create(const RenderBlock* renderer, bool horizontalWritingMode) > +{ > + return adoptPtr(new FloatingObjects(renderer, horizontalWritingMode)); > +} For better encapsulation, remove horizontalWritingMode and do?: return adoptPtr(new FloatingObjects(renderer, renderer->isHorizontalWritingMode())); > Source/WebCore/rendering/RenderBlock.h:1178 > friend void RenderBlock::createFloatingObjects(); You should remove this. > Source/WebCore/rendering/RenderBlock.h:1181 > + friend OwnPtr<FloatingObjects>; > + friend PassOwnPtr<FloatingObjects>; friend class xxx For which scope was this needed?