| Summary: | Encapsulate FloatingObject's constructor inside create | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
| Component: | New Bugs | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
| Status: | REOPENED --- | ||||||||||||
| Severity: | Normal | CC: | benjamin, buildbot, dglazkov, eric, hyatt, jchaffraix, leviw, ojan.autocc, philn, rego+ews, rniwa, simon.fraser, tony, webkit.review.bot, xan.lopez | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ryosuke Niwa
2013-02-18 17:24:16 PST
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? |